Add stateless signer program#7
Conversation
| solana_instruction::{AccountMeta, Instruction}, | ||
| solana_sdk_ids::sysvar::slot_hashes, | ||
| spl_ed25519_programmatic_signer_interface::instruction::ProgrammaticSignerInstruction, | ||
| spl_ed25519_programmatic_signer_legacy_interface::instruction::ProgrammaticSignerInstruction, |
There was a problem hiding this comment.
Keeping this old crates here until I can get Initialize code moved to the new program in the next PR
| # TODO: pinned to mollusk's `4.1-beta` branch for a friendlier dep graph resolution w/ the new no-std crates | ||
| mollusk-svm = { git = "https://github.com/anza-xyz/mollusk", branch = "4.1-beta" } |
There was a problem hiding this comment.
Found this to solve all of my no-std crate issues given the latest updates. Think acceptable for now and can go back to stable mollusk version when ready.
grod220
force-pushed
the
executor-pt-1
branch
2 times, most recently
from
534687c to
b4dabef
Compare
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Sorry for the lateness, but looks really good to me! A few questions about usability, mostly around hardware wallets, but the guts are definitely solid
| // The signed payload specifies which signer program it authorizes, so an envelope | ||
| // signed for a different signer program cannot be replayed against this one. | ||
| if &payload.signer_program_id != program_id { | ||
| return Err(Error::SignerProgramMismatch.into()); | ||
| } |
There was a problem hiding this comment.
Not sure if this is needed, since execution / replay protection is handled by the executor program.
If someone uses a different signer program, they'll get different PDAs, which shouldn't cause any issues. If someone has PDAs with multiple different signer programs, then the executor should still protect.
Writing this out, it seems like protection against a certain possible class of executor program bugs, which isn't the worst thing in the world, so we can keep it
There was a problem hiding this comment.
I actually think that's a good point. If the cold-wallet is expecting a certain signer program, it's because they need signer privileges it can grant. With the relayer passing it into the wrong one, it arrives without those privileges. The executor program needs to check for .is_signer() regardless.
That said, in the new design, if we wanted to bring back this check, I suppose we could hash the signer program id and put it in the recent blockhash field (currently unused in the inner wrapped tx and used in the inner-inner one as the nonce).
|
|
||
| let (programmatic_signer, bump) = | ||
| ProgrammaticSigner::derive_address_and_bump(program_id, authority); | ||
| authorized.push(AuthorizedSigner { |
There was a problem hiding this comment.
should we reject duplicates (somewhere)?
There was a problem hiding this comment.
Don't see a security reason why to prevent it. Duplicates don't grant any extra authority and the executor handles the account list policy.
There was a problem hiding this comment.
you should check the history of the spl-token multisig functionality 😂
|
A pretty significant refactor on this latest commit given the issues discussed in #7 (comment). Hardware wallet support is critical, so the signed payload changed: Also, now that the signed message contains the account list/flags, the signer program can enforce that the outer accounts mirror the signed keys exactly. |
t-nelson
left a comment
t-nelson
left a comment
There was a problem hiding this comment.
i can get behind this as mvp. just a couple nits
still p sure signer and executor functionality can be fully decoupled, but we can table that for now as most of the work should be in client side transaction construction, which we're handing to consumers a library either way
| let signer_seeds: Vec<[Seed; 3]> = authorized_signers | ||
| .iter() | ||
| .map(|signer| { | ||
| [ | ||
| Seed::from(ProgrammaticSigner::SEED_PREFIX), | ||
| Seed::from(signer.authority.as_ref()), | ||
| Seed::from(&signer.bump_seed), | ||
| ] | ||
| }) | ||
| .collect(); | ||
| let cpi_signers: Vec<Signer> = signer_seeds.iter().map(Signer::from).collect(); |
There was a problem hiding this comment.
seems like these can be combined to save a collect?
There was a problem hiding this comment.
It explodes. Signer does not own the seed array, it only stores a borrow pointing into it. So the first is the storage that keeps those seed arrays alive.
4 participants
Implements the first step of the executor-model decomposition proposed in #4. This new program has a single ix
Submitwhich simply verifies authority ed25519 signatures over an opaque payload, then CPIs the executor program the payload names, promoting each authority'sProgrammaticSignerPDA to a signer.The executor program (in the next PR) is responsible for nonce advancing, parsing the message bytes, and replaying the instructions.
This new signer program is simple and stateless, so the objective is to make this immutable.