Skip to content

feat(standards): Add DelayedExecution (Timelocked Accounts) to AuthMultisigSmart#3044

Open
onurinanc wants to merge 14 commits into
nextfrom
onur-multisig-smart-delayed-execution
Open

feat(standards): Add DelayedExecution (Timelocked Accounts) to AuthMultisigSmart#3044
onurinanc wants to merge 14 commits into
nextfrom
onur-multisig-smart-delayed-execution

Conversation

@onurinanc

Copy link
Copy Markdown
Collaborator

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.

@PhilippGackstatter PhilippGackstatter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_propose functionality when we already have the individual cancel and propose.

Comment on lines +12 to +13
# Map entries: [TX_HASH] -> [unlock_timestamp, proposal_timestamp, min_cancel_sigs, 1]
const TX_PROPOSALS_SLOT = word("miden::standards::auth::multisig_smart::tx_proposals")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd call this TX_SUMMARY_COMMITMENT instead of TX_HASH for accuracy (everywhere it is used in this PR).

Comment on lines +48 to +54
#! Inputs: [min_delay, propose_expiration_delta]
#! Outputs: []
#!
#! Side effects:
#! - Writes DELAYED_EXECUTION_SLOT with word
#! `[min_delay, propose_expiration_delta, 0, 0]` (see `get_delayed_execution`).
#!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think we should rather add a Where: clause explaining the inputs rather than the side effect which can be checked by looking at the code.

Comment on lines +88 to +91
#! Loads the delayed execution policy configuration from `DELAYED_EXECUTION_SLOT`.
#!
#! Inputs: []
#! Outputs: [min_delay, propose_expiration_delta, 0, 0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Returning the zeros is unnecessary.

Comment on lines +155 to +157
proc get_propose_expiration_delta
exec.get_delayed_execution
# => [min_delay, propose_expiration_delta, 0, 0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Comment on lines +178 to +193
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +892 to +895
#! Finalizes a pending cancel action for the current transaction.
#!
#! Inputs: [num_verified_signatures, TX_HASH]
#! Outputs: [num_verified_signatures, TX_HASH]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +810 to +822
#! 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +477 to +487
#! 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the functionality of this procedure not the same as calling cancel and then propose? Why do we need a single procedure for this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented here now: #3043 (comment)

Comment on lines +851 to +852
exec.delayed_execution::finalize_timelock_proposals
# => [TX_SUMMARY_COMMITMENT]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mmagician mmagician requested a review from Fumuran June 8, 2026 16:18
Comment on lines +110 to +113
#! 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`).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We usually don't mention the exact error variants.

Comment on lines +599 to +611
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

@PhilippGackstatter PhilippGackstatter Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a bigger refactor, I'd wait for @bobbinth or @mmagician's input before executing on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add DelayedExecution (Timelocked Accounts) to AuthMultisigSmart

2 participants