Add instruction to transfer a role

[fedgiac]

Introduce an instruction to immediately transfer an authority on success.
This replaces #107 in that there's no initial transfer proposal step. This differs from the original design discussed earlier, but I decided to go for this given the comments from that PR and the decision to cut corners for the sake of a faster release.

This PR:

• Adds the concept of Roles (authorities)
• Adds a TransferAuthority instruction

Until now manager and reclaim_authority were written once by Initialize and could never change; now the manager (or a role's current holder) can hand a role over in a single instruction.

A generic mechanism to transfer authorities

I made the transfer role-generic rather than writing a SetManager/SetReclaimAuthority pair. A Role enum (Manager = 0, ReclaimAuthority = 1) is specified in the instruction data, and a single authorization rule covers every role. If we need more authorities, it's easy to add them. (For example, the one to reclaim funds in the buffers.)
The manager is special in that it can always propose a role change.

In-place edit of the bytes

Instead of decoding the account to a struct and then reencoding on change, the code is designed to

Tests

I kept the aggressive macros from #107 because the reviewer didn't stop me. 😈

I also made a few improvements in passing that aren't strictly needed for this PR:

• unified PROGRAM_ID for consistency; it can't be generated with a hash (the hashing function isn't const) and so I picked something unused so far (0xc0 from c0w).
• Moved around pubkey_from_seed to be available to the client.
• Used pubkey_from_seed in some places where I could use them for consistency.
• A more ergonomic setup_init.

How to test

I admit this PR is too large, I could have split the introduction of the roles, the new in-place decoding of the state, and the new instruction. However, this would have been confusing for a reviewer coming from #107.

CI, verify that test coverage is satisfactory.

[linear-code]

SC-298

[kaze-cow]

I kept the aggressive macros from #107 because the reviewer didn't stop me. 😈

macro on, dude 😆

[kaze-cow]

I'm super happy with how the state account ref concept turned out. I wonder if we could reduce the construction even more by using lazy access fields through a impl EncodedStateAccount rather than constructing a view struct, but its probably a minor optimization.

From the previous PR

If there's a program upgrade authority, this changes everything. This is definitely a new role, but I'd still let the manager handle rent and withdrawal so that we only need the upgrade authority in exceptional circumstances. It should be able to oust the manager, yes, and then probably all the rest. But an upgrade authority is a much larger discussion point and I wouldn't design that PR based on its introduction.

Yes every program in solana comes with an upgrade authority. However I just dug a little deeper and found out that the way that a program loses its upgradability (through the solana program deploy --final flag) is by effectively setting the upgrade authority to None onchain, so piggybacking off of this wouldn't make a lot of sense once we go immutable.

[kaze-cow]

this is beautiful! thanks for taking my feedback from before. unfortunate that we need to define it twice for mutable/immutable but I can't say I'm complaining.

[fedgiac]

Yeah I don't like the repetition as well but it's the best compromise I found.

I wonder if we could reduce the construction even more by using lazy access fields through a impl EncodedStateAccount rather than constructing a view struct, but its probably a minor optimization.

Not easily. Note that the authority* takes bytes: &mut [u8; Self::SIZE], not self, which would make much more sense. Why? Because then EncodedStateAccount would need to take a (mutable) reference, and this reference would have to be kept alive, which has a big chance to make coding changes harder and overall uglier code. Probably it can be done differently but this is fine enough after all my attempts.

[kaze-cow]

you could macroify this with pastey 🤔 if you don't want to do this now I totally understand

[fedgiac]

Assuming you mentioned the macro for solving the mutable reference issue, I don't think it helps because eventually the reference still has to be managed. If it's about the code repetition, I think it's small and self-contained enough that it doesn't warrant the extra dependency.

[kaze-cow]

I mentioned in the previous PR that I didn't like the kind of wordy syntax. At a glance its not obvious that each line of this macro actually expands to 3 fully fledged functions (though this is kind of a nit opinion). I think it would be better if the macro looked sort of like a function call.

you mentioned not liking having to explicitly define the test name, so I asked around (claude) and it suggested the pastey crate, which allows for concatenation of idents.

So in total I got this vibed thing which I think looks a lot more intuitive:

macro_rules! transfer_authority_test {
    ($actor:ident, $role:ident => ok) => {
        pastey::paste! {
            #[test]
            fn [<transfer_authority_ $actor:lower _to_ $role:lower>]() {
                let result = transfer_authority(Role::$actor, Role::$role);
                assert_eq!(result, Ok(Role::$role));
            }
        }
    };
    ($actor:ident, $role:ident => err($msg:literal)) => {
        pastey::paste! {
            #[test]
            fn [<transfer_authority_ $actor:lower _to_ $role:lower _fails>]() {
                let result = transfer_authority(Role::$actor, Role::$role);
                assert_eq!(result, Err($msg));
            }
        }
    };
}

transfer_authority_test!(Owner, Solver  => ok);            // fn transfer_authority_owner_to_solver
transfer_authority_test!(Manager, Solver  => ok);            // fn transfer_authority_manager_to_solver
transfer_authority_test!(Manager, Owner   => err("unauthorized")); // ..._manager_to_owner_fails
transfer_authority_test!(Solver, Manager => err("unauthorized"));

[fedgiac]

To be honest I'd rather drop the macro than add a dependency to make it nicer, since it's already overkill. 😅
Even then, macros are already magic by themselves, and adding a macro in a macro definition makes things even harder to reason about. I've got no intuition what paste! is supposed to do, thought I admit the syntax is reasonable.

Anyway I see your point now. What do you think about this?

[kaze-cow]

i would strongly be in favor of keeping the macro for sure. you could probably even get rid of the helper function it depends on and have the whole thing be much more self contained.

macros basically always are, in their nature, obfuscating what is happening, buti f the macro is well named and designed, I find them to be much easier to myself as a code reader.

[fedgiac]

Good! I agree with you overall (but I like to keep things out of macros for readability and minimize macro code size; note that many dev tools are worse in macros, a clear example is autoformatting as you can see in my commit history but I expect the same is true for linting and static analysis). Let's keep it like now then!

[kaze-cow]

feedback has been addressed, just a couple followup comments