feat(standards): Add DelayedExecution (Timelocked Accounts) to AuthMultisigSmart#3044
feat(standards): Add DelayedExecution (Timelocked Accounts) to AuthMultisigSmart#3044onurinanc wants to merge 31 commits into
DelayedExecution (Timelocked Accounts) to AuthMultisigSmart#3044Conversation
…ution' into onur-multisig-smart-delayed-execution
PhilippGackstatter
left a comment
There was a problem hiding this comment.
I left a few questions/comments/nits. Overall, I'm having a hard time wrapping my head around this PR. In particular:
- it seems we have multiple names for the same concept - would be great to unify.
- I find the use of the pending slots hard to follow, and I'm not sure I understand why we need these. I left a question/suggestion to use auth args instead.
- I don't understand why we need the
cancel_and_proposefunctionality when we already have the individualcancelandpropose.
| # Map entries: [TX_HASH] -> [unlock_timestamp, proposal_timestamp, min_cancel_sigs, 1] | ||
| const TX_PROPOSALS_SLOT = word("miden::standards::auth::multisig_smart::tx_proposals") |
There was a problem hiding this comment.
Nit: I'd call this TX_SUMMARY_COMMITMENT instead of TX_HASH for accuracy (everywhere it is used in this PR).
| #! Loads the delayed execution policy configuration from `DELAYED_EXECUTION_SLOT`. | ||
| #! | ||
| #! Inputs: [] | ||
| #! Outputs: [min_delay, propose_expiration_delta, 0, 0] |
There was a problem hiding this comment.
Nit: Returning the zeros is unnecessary.
| proc get_propose_expiration_delta | ||
| exec.get_delayed_execution | ||
| # => [min_delay, propose_expiration_delta, 0, 0] |
There was a problem hiding this comment.
| proc get_propose_expiration_delta | |
| exec.get_delayed_execution | |
| # => [min_delay, propose_expiration_delta, 0, 0] | |
| proc get_propose_expiration_delta | |
| exec.get_delayed_execution_config | |
| # => [min_delay, propose_expiration_delta, 0, 0] |
Nit: Maybe the current procedure name could be a bit clearer.
| proc apply_expiration_delta | ||
| dup neq.0 assert.err=ERR_EXPIRATION_DELTA_ZERO | ||
| # => [expiration_delta] | ||
|
|
||
| exec.tx::update_expiration_block_delta | ||
| # => [] | ||
|
|
||
| exec.tx::get_expiration_block_delta | ||
| # => [tx_expiration_delta] | ||
|
|
||
| dup neq.0 assert.err=ERR_TX_EXPIRATION_DELTA_NOT_SET | ||
| # => [tx_expiration_delta] | ||
|
|
||
| drop | ||
| # => [] | ||
| end |
There was a problem hiding this comment.
I don't understand why we set and then get the delta - this seems redundant. The neq.0 check is already done in tx::update_expiration_block_delta (but it is not documented in a Panics if section - if you don't mind, we could add it in this PR).
So, I think the whole procedure can be removed in favor of calling tx::update_expiration_block_delta directly.
| #! Finalizes a pending cancel action for the current transaction. | ||
| #! | ||
| #! Inputs: [num_verified_signatures, TX_HASH] | ||
| #! Outputs: [num_verified_signatures, TX_HASH] |
There was a problem hiding this comment.
Does this procedure use TX_HASH? If not, can we remove it from the inputs/outputs? I'd also pass in num_verified_signatures but not return it. Caller should dup it.
| #! Marks that this tx intends to execute a proposed action. | ||
| #! | ||
| #! Inputs: [] | ||
| #! Outputs: [] | ||
| #! | ||
| #! Panics if: | ||
| #! - `PENDING_EXECUTE_SLOT` is already non-empty (`ERR_PENDING_ALREADY_SET`). | ||
| #! | ||
| #! Side effects: | ||
| #! - Writes `PENDING_EXECUTE_FLAG` to `PENDING_EXECUTE_SLOT`. | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc execute_proposed_transaction |
There was a problem hiding this comment.
Is the use of this procedure to communicate to the auth procedure that the current transaction executed a proposed action? It seems like this is intended to be called from a tx script, which is arbitrarily provided by the tx executor.
If so, can we not achieve the same by passing a flag as AUTH_ARGS? And if so, can we get rid of all the pending slots by using this pattern?
There was a problem hiding this comment.
I think the same pattern needs to stay for propose/cancel, so it might be good keeping execute on the same pattern for symmetry. I'm not sure if removing all the pending slots by using the AUTH_ARGS pattern works.
| #! Returns the pending execute transaction summary hash, or `EMPTY_WORD` if none is set. | ||
| #! | ||
| #! Inputs: [] | ||
| #! Outputs: [PENDING_EXECUTE_HASH] | ||
| #! | ||
| #! Where: | ||
| #! - PENDING_EXECUTE_HASH is the pending execute transaction summary hash, or | ||
| #! `EMPTY_WORD` if no execute action is pending. | ||
| #! | ||
| #! Invocation: exec | ||
| proc get_pending_execute |
There was a problem hiding this comment.
Is this procedure description accurate? In set_pending_execute we write PENDING_EXECUTE_FLAG and not a HASH, so there is a mismatch.
| #! - Writes `NEW_TX_HASH` to `PENDING_PROPOSE_SLOT`. | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc cancel_and_propose_new_transaction |
There was a problem hiding this comment.
Is the functionality of this procedure not the same as calling cancel and then propose? Why do we need a single procedure for this?
| exec.delayed_execution::finalize_timelock_proposals | ||
| # => [TX_SUMMARY_COMMITMENT] |
There was a problem hiding this comment.
Given that we clear the pending slots as the last step, the tx summary we computed earlier contains the changes to the pending slots. This works in principle, but feels a bit unclean. Mentioning in case this was not intended.
| #! Panics if: | ||
| #! - block_height_delta is not a valid `u32` (`ERR_TX_INVALID_EXPIRATION_DELTA`). | ||
| #! - block_height_delta is zero (`ERR_TX_INVALID_EXPIRATION_DELTA`). | ||
| #! - block_height_delta is greater than 0xFFFF (`ERR_TX_INVALID_EXPIRATION_DELTA`). |
There was a problem hiding this comment.
Nit: We usually don't mention the exact error variants.
| pub proc cancel_transaction_proposal | ||
| dupw | ||
| # => [TX_SUMMARY_COMMITMENT, TX_SUMMARY_COMMITMENT] | ||
|
|
||
| exec.is_tx_proposed | ||
| # => [is_proposed, TX_SUMMARY_COMMITMENT] | ||
|
|
||
| assert.err=ERR_TX_NOT_PROPOSED | ||
| # => [TX_SUMMARY_COMMITMENT] | ||
|
|
||
| exec.set_pending_cancel | ||
| # => [] | ||
| end |
There was a problem hiding this comment.
Afaict, propose_transaction and cancel_transaction_proposal are both essentially setters for their provided arguments; writing into a temporary storage slot. The arguments come from a tx script, which is constructed by a user. The main work is done in the auth procedure, which reads the slots, enforces delay constraints and updates the maps.
If we do not need these procedures to be called from a note, which I assume is not the case, we could reduce complexity by a lot by removing these procedures and setting AUTH_ARGS to the hash of the following data:
[delay_action, CANCEL_TX_SUMMARY_COMMITIMENT, PROPOSE_TX_SUMMARY_COMMITIMENT, SALT]
where delay_action is one of {execute, propose, cancel}. We then dispatch based on delay_action.
That way, we could handle all logic in one flow within the auth procedure rather than splitting it across a pre-auth and a post-auth phase, and avoid having three temporary storage slots.
Whether the user of the multisig constructs a tx script and calls these APIs or whether they construct AUTH_ARGS in the above way should be essentially equivalent.
Any thoughts? Would this work?
There was a problem hiding this comment.
If we do not need these procedures to be called from a note, which I assume is not the case, we could reduce complexity by a lot by removing these procedures and setting AUTH_ARGS to the hash of the following data:
I was thinking that we might somehow extend delayed execution logic into note-based auth, however, as I explore more about this I've seen that it's not possible to use the same logic as this design requires num_signatures, and this is not the case for owner controlled and rbac.
So, although I believe the current design is easier to read, your proposed way of constructing AUTH_ARGS reduces the complexity, and API calls are easier to use. So, it seems it is better to switch to your proposed design.
There was a problem hiding this comment.
Since this is a bigger refactor, I'd wait for @bobbinth or @mmagician's input before executing on this.
There was a problem hiding this comment.
Any updates on the possible refactoring item? @bobbinth @mmagician
There was a problem hiding this comment.
I haven't gotten into the review yet - but @onurinanc - what's the best place to get the overall design approach from? I remember we had a long discussion a while back, but I've forgotten some important aspects since then, and other aspects have probably changed. So, it may be helpful to have a summary of how the overall mechanism is supposed to work.
There was a problem hiding this comment.
Agreed that monitoring is required - but I think that's the case anyways. Even if prohibit re-proposing canceled proposals, the attacker could modify the proposal slightly (e.g., use a different salt, or maybe make some other trivial change to the proposal) and make a new proposal which would have a different commitment but would be semantically the same. So, AFAICT, there isn't really a way to get away from the need to monitor.
There was a problem hiding this comment.
I think proposed design doesn't work since propose_transaction would need to verify the signatures over the proposed transaction's commitment, but signature verification can only run against the current transaction.
When verify_signatures runs, it goes through the AuthRequest event, and the kernel reconstructs the transaction summary from the current transaction (from account delta, input/output notes) and rejects the message if those commitments don't match. So the signed message is always pinned to the current transaction.
This breaks the design in two ways:
-
The proposed transaction's delta (its future storage changes) differs from the proposing transaction's delta (just the proposals-map write), so the kernel's summary check fails, so you can't verify signatures over the proposed transaction from inside the proposing one.
-
The account delta is only final at the end of the transaction, in the auth procedure. Calling
verify_signaturesmid-transaction insidepropose_transactionsees an incomplete delta. Signature verification structurally has to happen in auth, after all state changes.
So having the proposal verify the proposed transaction isn't possible by reusing the current signature verification. Since signature verification has to run in auth (where the delta is final), it ends up signing the current transaction, which is exactly why the "pending & finalizer" and "auth_args" designs sign the proposing transaction.
There was a problem hiding this comment.
I think proposed design doesn't work since
propose_transactionwould need to verify the signatures over the proposed transaction's commitment, but signature verification can only run against the current transaction.
Ah - good point! I think this is an unfortunate limitation of the current approach - but we may be able to modify this relatively easily.
Refreshing my memory a bit, it seems like we require tx summary to be built even if the signature is found in the advice provider. This shouldn't be strictly needed: if the signature is already in the advice provider, AFAICT, there is no real need to build the tx summary.
So, the change may be as simple as modifying the TransactionEvent::AuthRequest variant to look like:
AuthRequest {
pub_key_commitment: PublicKeyCommitment,
tx_summary: Option<TransactionSummary>,
signature: Option<Vec<Felt>>,
}Or maybe something more "strongly typed" as we really can have either tx_summary or signature - but don't really need to have both.
@PhilippGackstatter - what do you think?
There was a problem hiding this comment.
Agreed, that should work.
Or maybe something more "strongly typed" as we really can have either
tx_summaryorsignature- but don't really need to have both.
We can add the either crate so we can write this as:
AuthRequest {
pub_key_commitment: PublicKeyCommitment,
signature_or_summary: Either<Vec<Felt>, TransactionSummary>,
}We'd only call extract_tx_summary if the signature is None.
There was a problem hiding this comment.
I've implemented this in #3157 - though, instead of using Either just created a TxSummaryOrSignature enum.
partylikeits1983
left a comment
There was a problem hiding this comment.
Looks great! Left two comments below
| #! High-impact action. | ||
| #! |
There was a problem hiding this comment.
Nit:
| #! High-impact action. | |
| #! |
| pub proc update_delayed_execution_policy | ||
| u32assert.err=ERR_MIN_DELAY_NOT_U32 | ||
| dup eq.0 assertz.err=ERR_MIN_DELAY_ZERO | ||
| # => [min_delay, propose_expiration_delta] | ||
|
|
||
| swap u32assert.err=ERR_PROPOSE_EXPIRATION_DELTA_NOT_U32 | ||
| dup eq.0 assertz.err=ERR_PROPOSE_EXPIRATION_DELTA_ZERO | ||
| # => [propose_expiration_delta, min_delay] |
There was a problem hiding this comment.
We should add an assertion here that propose_expiration_delta is a valid u16.
This procedure only checks that propose_expiration_delta is a u32 and non-zero, so it accepts any value up to 4294967295.
But the value set here is later passed into the kernel's tx::update_expiration_block_delta, which only accepts deltas in the range 1 to 65535. Every propose_transaction (and cancel_and_propose_new_transaction) calls that kernel proc, so:
- It's possible to set the policy with
propose_expiration_delta >= 65536; this succeeds and writes the value to storage. - From then on, every
propose_transactionpanics in the kernel withERR_TX_INVALID_EXPIRATION_DELTA, because the stored delta is above65535.
The result is that the whole propose flow would be bricked in this case until someone calls update_delayed_execution_policy again with a valid value (this proc never touches the kernel, so it's recoverable, not permanently stuck).
This isn't really an issue because the rust DelayedExecutionPolicy stores the field as a u16 (max 65535), so an out-of-range value can only get in via a direct MASM call.
| add | ||
| # => [unlock_timestamp, proposal_timestamp] | ||
| end |
There was a problem hiding this comment.
We should have a check here which asserts the resulting unlock_timestamp is a u32
…ution' into onur-multisig-smart-delayed-execution
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good! Only got through the main file delayed_execution.masm but not yet the rest. Left a few small comments, but nothing too important. Will review the rest soon.
| proc get_tx_proposal(tx_summary_commitment: word) | ||
| push.TX_PROPOSALS_SLOT[0..2] | ||
| # => [slot_prefix, slot_suffix, TX_SUMMARY_COMMITMENT] | ||
|
|
||
| exec.active_account::get_map_item | ||
| # => [unlock_timestamp, proposal_timestamp, min_cancel_sigs, 0] | ||
|
|
||
| movup.3 drop | ||
| # => [unlock_timestamp, proposal_timestamp, min_cancel_sigs] | ||
| end |
There was a problem hiding this comment.
Would there be a downside if we used get_initial_map_item here? I suppose it's safe to leave it as-is, but might be even safer to rely on the initial state.
| push.EMPTY_WORD | ||
| # => [EMPTY_WORD, TX_SUMMARY_COMMITMENT] | ||
|
|
||
| swapw | ||
| # => [TX_SUMMARY_COMMITMENT, EMPTY_WORD] | ||
|
|
||
| push.TX_PROPOSALS_SLOT[0..2] | ||
| # => [slot_prefix, slot_suffix, TX_SUMMARY_COMMITMENT, EMPTY_WORD] | ||
|
|
||
| exec.native_account::set_map_item | ||
| # => [OLD_PROPOSAL_WORD] | ||
|
|
||
| dropw | ||
| # => [] |
There was a problem hiding this comment.
Nit/Optional: Should we call write_tx_proposal here with push.0.0.0?
| exec.tx::get_block_timestamp | ||
| # => [proposal_timestamp] |
There was a problem hiding this comment.
Just pointing out that we usually use block numbers rather than timestamps. I generally think the latter is easier to work with for users, but we mostly use block numbers now, so may be best for consistency, cc @bobbinth.
| #! - `assert_unlock_reached` fails. | ||
| #! | ||
| #! Invocation: exec | ||
| proc enforce_tx_timelock |
There was a problem hiding this comment.
| proc enforce_tx_timelock | |
| proc assert_tx_proposed_and_unlocked |
nit: use "assert" phrasing
| # => [unlock_timestamp, proposal_timestamp, min_cancel_sigs] | ||
|
|
||
| dup.1 neq.0 assert.err=ERR_TX_NOT_PROPOSED | ||
| # => [unlock_timestamp, proposal_timestamp, min_cancel_sigs] | ||
|
|
||
| swap drop | ||
| # => [unlock_timestamp, min_cancel_sigs] |
There was a problem hiding this comment.
| # => [unlock_timestamp, proposal_timestamp, min_cancel_sigs] | |
| dup.1 neq.0 assert.err=ERR_TX_NOT_PROPOSED | |
| # => [unlock_timestamp, proposal_timestamp, min_cancel_sigs] | |
| swap drop | |
| # => [unlock_timestamp, min_cancel_sigs] | |
| # => [unlock_timestamp, proposal_timestamp, min_cancel_sigs] | |
| swap neq.0 assert.err=ERR_TX_NOT_PROPOSED | |
| # => [unlock_timestamp, min_cancel_sigs] |
| pub proc cancel_transaction_proposal | ||
| # => [TX_SUMMARY_COMMITMENT] |
There was a problem hiding this comment.
| pub proc cancel_transaction_proposal | |
| # => [TX_SUMMARY_COMMITMENT] | |
| pub proc cancel_transaction_proposal |
| dup.1 neq.0 assert.err=ERR_TX_NOT_PROPOSED | ||
| # => [unlock_timestamp, proposal_timestamp, min_cancel_sigs, TX_SUMMARY_COMMITMENT] | ||
|
|
||
| drop drop movdn.4 | ||
| # => [TX_SUMMARY_COMMITMENT, min_cancel_sigs] |
There was a problem hiding this comment.
| dup.1 neq.0 assert.err=ERR_TX_NOT_PROPOSED | |
| # => [unlock_timestamp, proposal_timestamp, min_cancel_sigs, TX_SUMMARY_COMMITMENT] | |
| drop drop movdn.4 | |
| # => [TX_SUMMARY_COMMITMENT, min_cancel_sigs] | |
| drop neq.0 assert.err=ERR_TX_NOT_PROPOSED | |
| # => [min_cancel_sigs, TX_SUMMARY_COMMITMENT] | |
| movdn.4 | |
| # => [TX_SUMMARY_COMMITMENT, min_cancel_sigs] |
| #! Inputs: [TX_SUMMARY_COMMITMENT] | ||
| #! Outputs: [TX_SUMMARY_COMMITMENT] | ||
| #! | ||
| #! Where: | ||
| #! - TX_SUMMARY_COMMITMENT is the current transaction summary commitment. | ||
| #! | ||
| #! Panics if: | ||
| #! - no proposal exists for `TX_SUMMARY_COMMITMENT` (`ERR_TX_NOT_PROPOSED`). | ||
| #! - the proposal unlock timestamp has not been reached (`ERR_TX_STILL_TIMELOCKED`). | ||
| #! | ||
| #! Invocation: exec | ||
| pub proc handle_execute |
There was a problem hiding this comment.
- I'd maybe call this
execute_proposed_tx. - Consider not returning
TX_SUMMARY_COMMITMENTas it is identical to the input and we try to avoid this pattern.
| # => [TX_SUMMARY_COMMITMENT] | ||
| dupw exec.enforce_tx_timelock drop | ||
| # => [TX_SUMMARY_COMMITMENT] |
There was a problem hiding this comment.
This is the only caller of enforce_tx_timelock and it drops its return value, so we can probably remove the return value from the procedure itself.
| push.0 exec.active_account::get_num_procedures | ||
| # => [num_procedures, requires_delay_acc] | ||
|
|
||
| dup neq.0 | ||
| # => [should_continue, num_procedures, requires_delay_acc] |
There was a problem hiding this comment.
I'd use neq.1 as the loop condition so we naturally exclude the auth procedure and don't need to consider the auth procedure in the loop at all.
| dup.1 exec.active_account::get_procedure_root | ||
| # => [PROC_ROOT, is_non_auth, proc_index, requires_delay_acc] | ||
|
|
||
| exec.native_account::was_procedure_called | ||
| # => [was_called, is_non_auth, proc_index, requires_delay_acc] | ||
|
|
||
| and | ||
| # => [should_process, proc_index, requires_delay_acc] | ||
|
|
||
| if.true | ||
| dup exec.active_account::get_procedure_root | ||
| # => [PROC_ROOT, proc_index, requires_delay_acc] |
There was a problem hiding this comment.
Nit: For efficiency, avoid calling get_procedure_root twice and keep the PROC_ROOT on the stack instead.
| #! Inputs: [] | ||
| #! Outputs: [requires_delay] | ||
| #! | ||
| #! The auth procedure (procedure index 0) is excluded, consistent with `compute_called_proc_policy`. | ||
| #! | ||
| #! Invocation: exec |
There was a problem hiding this comment.
Nit: I'd move this to the "prose section" above the Inputs/Outputs.
| exec.enforce_note_restrictions | ||
| # => [policy_threshold, policy_requires_delay] | ||
|
|
||
| swap drop | ||
| # => [policy_threshold] |
There was a problem hiding this comment.
We now drop the policy_requires_delay value computed by compute_proc_policy_contribution, so we should refactor that procedure so it becomes simpler.
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me! Left a few suggestions to make the implementation clearer, but logically I think it is good.
| pub proc propose_transaction | ||
| # => [TX_SUMMARY_COMMITMENT] | ||
| dupw exec.is_tx_proposed assertz.err=ERR_TX_ALREADY_PROPOSED | ||
| # => [TX_SUMMARY_COMMITMENT] |
There was a problem hiding this comment.
Afaiu, it is currently possible to propose a transaction that was already executed, but it would fail during execution due to multisig::assert_new_tx.
Not sure if it's really worth it, but I think I would add a check here that the proposed transaction is not in EXECUTED_TXS_SLOT to prevent this. Or if not, leave a comment here somewhere to mention the above.
I was also thinking that we could technically avoid the need for both EXECUTED_TXS_SLOT and TX_PROPOSALS_SLOT because we could use the latter to store a flag to indicate that a tx was executed. But, it's not clear that this provides meaningful savings (one slot saved) and the current setup is cleaner by keeping proposals separate from executed transactions (which also apply to txs executed immediately). So, basically, I wouldn't change anything about this.
| # PROC_ROOT is still on the stack; reuse it for the policy lookup. | ||
| exec.get_procedure_policy | ||
| # => [immediate, delayed, restr_proc, proc_index, threshold_acc, requires_delay_acc, restrictions_acc] | ||
| # => [immediate, delayed, restr_proc, proc_index, threshold_acc, restrictions_acc] |
There was a problem hiding this comment.
| # => [immediate, delayed, restr_proc, proc_index, threshold_acc, restrictions_acc] | |
| # => [immediate, delayed, note_restrictions, proc_index, threshold_acc, restrictions_acc] |
nit: standard terminology
| exec.delayed_execution::is_delay_action_only | ||
| not | ||
| # => [is_execution, TX_SUMMARY_COMMITMENT] |
There was a problem hiding this comment.
Suggestion: It might be a bit cleaner overall to have:
exec.delayed_execution::was_propose_or_cancel_called
# => [is_propose_or_cancel, TX_SUMMARY_COMMITMENT]
if.true
# assert that no other procedure besides propose or cancel was called
exec.tx_policy::assert_only_one_non_auth_procedure_called
else
# execution path
# ...
end
where was_propose_or_cancel_called is like is_delay_action_only now but without the assert_only_one_non_auth_procedure_calledcall.
This removes the branch from is_delay_action_only and reuses the branch that we need here anyway, effectively removing one branch. It also makes it a bit clearer that we enforce that no other procedure was called, which is currently a bit hidden.
| # `execute_proposed_tx` consumes the commitment; keep a copy so both branches leave | ||
| # [TX_SUMMARY_COMMITMENT] on the stack. |
There was a problem hiding this comment.
| # `execute_proposed_tx` consumes the commitment; keep a copy so both branches leave | |
| # [TX_SUMMARY_COMMITMENT] on the stack. |
nit
| if.true | ||
| # `execute_proposed_tx` consumes the commitment; keep a copy so both branches leave | ||
| # [TX_SUMMARY_COMMITMENT] on the stack. | ||
| dupw exec.delayed_execution::execute_proposed_tx |
There was a problem hiding this comment.
nit: I suggested this name but I still don't really like it as it doesn't quite capture what the procedure does (assert timestamp reached, remove from proposals), but I don't really have a better suggestion either.
| let mut tx_context_init_builder = mock_chain | ||
| .build_tx_context(account_id, &[], &[])? | ||
| .tx_script(tx_script.clone()) | ||
| .auth_args(salt); | ||
|
|
||
| if let Some(tx_script_args) = tx_script_args { | ||
| tx_context_init_builder = tx_context_init_builder.tx_script_args(tx_script_args); | ||
| } | ||
|
|
||
| if let Some(advice_inputs) = advice_inputs.clone() { | ||
| tx_context_init_builder = tx_context_init_builder.extend_advice_inputs(advice_inputs); | ||
| } | ||
|
|
||
| let tx_summary = tx_context_init_builder | ||
| .build()? | ||
| .execute() | ||
| .await | ||
| .unwrap_err() | ||
| .unwrap_unauthorized_err(); | ||
|
|
||
| let msg = tx_summary.as_ref().to_commitment(); | ||
| let tx_summary = SigningInputs::TransactionSummary(tx_summary); | ||
|
|
||
| let mut tx_context_signed_builder = mock_chain | ||
| .build_tx_context(account_id, &[], &[])? | ||
| .tx_script(tx_script) | ||
| .auth_args(salt); |
There was a problem hiding this comment.
Here I would also build the tx just once and clone the builder to avoid the duplication.
| #[rstest] | ||
| #[case::ecdsa(AuthScheme::EcdsaK256Keccak)] | ||
| #[case::falcon(AuthScheme::Falcon512Poseidon2)] | ||
| #[tokio::test] | ||
| async fn test_multisig_smart_delayed_only_proc_rejects_direct_path_without_proposal( |
There was a problem hiding this comment.
I think the benefit of running these with two signature schemes is fairly little but it costs quite a bit in test execution time, especially the falcon version, so I'd just default to the ecdsa variant. I think the two schemes are sufficiently tested in hybrid_multisig, so I'd remove this from all tests here.
| /// An unauthorized dry-run of a procedure must still yield its `TX_SUMMARY_COMMITMENT` (so a caller | ||
| /// can obtain the commitment to propose). With no proposal present the execution mode is immediate; | ||
| /// because the procedure's policy also permits the immediate path, execution reaches the | ||
| /// signature/threshold check and aborts with `Unauthorized` (carrying the summary) rather than | ||
| /// panicking at the policy layer. | ||
| #[rstest] | ||
| #[case::ecdsa(AuthScheme::EcdsaK256Keccak)] | ||
| #[case::falcon(AuthScheme::Falcon512Poseidon2)] | ||
| #[tokio::test] | ||
| async fn test_multisig_smart_unauthorized_dry_run_returns_tx_summary( |
There was a problem hiding this comment.
This is implicitly tested by all tests that extract a tx summary, so imo we can remove this test.
| match result { | ||
| Err(TransactionExecutorError::TransactionProgramExecutionFailed(_)) => {}, | ||
| Err(err) => panic!("expected transaction program failure, got: {err}"), | ||
| Ok(_) => panic!("bundling a delay action with another procedure must fail"), | ||
| } |
There was a problem hiding this comment.
I think we should be more precise with the assertion using assert_transaction_executor_error!.
| let mut mock_chain = | ||
| MockChainBuilder::with_accounts([multisig_account.clone()]).unwrap().build()?; | ||
|
|
||
| // The execute transaction the proposal is for: in option 4 it just calls the real procedure; |
There was a problem hiding this comment.
nit: another reference to "option 4"
Closes: #3043.
Previously opened a draft PR implementing all features regarding Smart Multisig here: #2973. That PR still relatively big, and we separate the delayed execution logic into this PR.