Repository navigation
CLI: Add --sign-only with durable nonce for transaction submit - #71
mcintyre94 wants to merge 1 commit into
Conversation
joncinque
left a comment
There was a problem hiding this comment.
Looks great overall! Just a few points which I hope will simplify the code
| /// Durable nonce value used as the relay transaction's blockhash. Read from the durable nonce | ||
| /// account when omitted. | ||
| #[clap(long, value_name = "HASH", requires = "durable-nonce")] | ||
| durable_nonce_value: Option<Hash>, |
There was a problem hiding this comment.
nit: value is a bit vague, especially when this is supposed to be the nonce hash. At the very least, let's go with durable_nonce_hash. I do think we might want to prefer blockhash to be consistent with the other tools
There was a problem hiding this comment.
I've switched this to --blockhash. If used without --durable-nonce then it can be used to set the blockhash, instead of fetching the latest one. This aligns with our other tools
| /// `transaction submit --sign-only` also accepts an address, whose signature is collected | ||
| /// separately. Defaults to the configured keypair. |
There was a problem hiding this comment.
I'm torn about whether this comment is needed, but I guess it doesn't hurt. I did get confused about null signers back in the day 😅
06d16f1 to
9d94971
Compare
9d94971 to
de5543c
Compare
de5543c to
9f23ba4
Compare
5920cbf to
5fa4318
Compare
5fa4318 to
1ae35c5
Compare
joncinque
left a comment
There was a problem hiding this comment.
Looks great! Just the point on the TODO really matters, the rest can be handled separately if you choose
| let test = SubmitTest::new(env, &signer).await; | ||
| // The forwarded signer also authorizes the durable nonce. A separate durable nonce authority | ||
| // would push this relay transaction over the transaction size limit. | ||
| // TODO: Test a separate durable nonce authority once the relay transaction is v1 |
There was a problem hiding this comment.
Rather than putting a TODO, let's create an issue for this or add it to an existing issue
There was a problem hiding this comment.
Added it to the tracking issue with a permalink to this test
| #[test] | ||
| fn sign_only_requires_authority_signatures() { | ||
| let env = SubmitTestEnv::new(); | ||
| assert_failure( | ||
| &env.submit_message(&[ | ||
| "--fee-payer", | ||
| &env.keypair_file(&env.fee_payer), | ||
| "--durable-nonce", | ||
| &env.durable_nonce.to_string(), | ||
| "--blockhash", | ||
| &env.durable_nonce_value.to_string(), | ||
| "--sign-only", | ||
| ]), | ||
| &format!( | ||
| "missing signature for authority {}, authorities sign with `transaction sign`", | ||
| env.authority.pubkey() | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Just to make sure I understand, this is check to make sure that all authorities are explicitly passed, even as null signers, to the sign only command. Is that correct?
There was a problem hiding this comment.
Nope this is in submit, so the requirement is that we have a signature for all of the authorities on the authorization message. Ie we have a signature for the authority of all the PDAs that will be promoted. Those signatures from transaction sign become part of the Execute instruction in the relay transaction, so we must have them all before we call transaction submit. If any are missing then the transaction would fail, and if you pass different ones in different calls to transaction submit then you'd get different relay transactions. So this is saying that we must have all those signatures when we call transaction submit, even if it's with --sign-only.
There was a problem hiding this comment.
Ohhh gotcha, that makes sense, thanks for the explanation!
The current version of `transaction submit` requires the fee payer and all forwarded signers to be available as signers at the time of the call. This is unsuitable for migration use cases where, for example, the keypair transferring its authority to a PDA may need to sign offline. This PR adds a `--sign-only` mode similar to other Solana CLI commands. `--blockhash` can be used to set the relay transaction's lifetime. Combined with `--durable-nonce` and `--durable-nonce-authority`, it can be set to the current value of a durable nonce, so that signatures don't expire. Under `--sign-only`, `--fee-payer` and `--durable-nonce-authority` also accept an address, whose signature is reported as absent, and `--dump-transaction-message` prints the relay transaction message so runs can be compared. Without `--sign-only`, `--durable-nonce` submits as usual with all relay signers local, checking the durable nonce value and authority first. The output is `CliSignOnlyData`, matching other Solana CLI `--sign-only` outputs. This PR only addresses offline signing. A follow-up PR will let `submit` accept these relay signatures in place of relay signers. Note some intentional differences from similar CLIs: - we use `--durable-nonce[-authority]` rather than `--nonce` and `--nonce-authority`, because the relay uses a System Program nonce account, distinct from the SPL nonce args (`--nonce-account`, `--nonce-authority`) on other commands
1ae35c5 to
f93818f
Compare
The current version of
transaction submitrequires the fee payer and all forwarded signers to be available as signers at the time of the call.This is unsuitable for migration use cases where, for example, the keypair transferring its authority to a PDA may need to sign offline.
This PR adds a durable nonce option for the relay transaction, and a
--sign-onlymode similar to other Solana CLI commands. Using a durable nonce instead of a recent blockhash as the relay transaction's lifetime lets each signer build and sign the same relay transaction independently, without RPC calls. Under--sign-only,--fee-payerand--durable-nonce-authorityalso accept an address, whose signature is reported as absent, and--dump-transaction-messageprints the relay transaction message so runs can be compared.Without
--sign-only,--durable-noncesubmits as usual with all relay signers local, checking the durable nonce value and authority first.The output is
CliSignOnlyData, matching other Solana CLI--sign-onlyoutputs. This PR only addresses offline signing. A follow-up PR will letsubmitaccept these relay signatures in place of relay signers.Note some intentional differences from similar CLIs:
--fee-payeris required when using--sign-only, instead of defaulting to the configured keypair, which would differ betweenoffline signers
--durable-nonce-authoritydefaults to the fee payer rather than the configured keypair, for the same reason--durable-nonce[-authority|-value]rather than--nonceand--nonce-authority, because the relay uses a System Program nonce account, distinct from the SPL nonce args (--nonce-account,--nonce-authority) on other commands--durable-nonce-valueto pass the nonce value, instead of--blockhash.submithas no--blockhasharg, and nonce value is clearerStack created with GitHub Stacks CLI • Give Feedback 💬