feat(standards): add AuthMultisigSmart #2973
Conversation
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Thank you for the PR @onurinanc.
The PR is very large which makes it quite difficult to wrap my head around. Roughly 3000 lines is tests, but could we still split up the feature logic into multiple PRs? For example, it seems the timelock controller and spending limits do not depend on each other, so could these be moved to a separate PR? I'm open to other ideas of course, but somehow reducing the line count would be awesome, as I think this PR as-is will be tricky to iterate on.
| /// This bundles the tracked spending window, the amount breakpoints used to derive a spending | ||
| /// tier, and the approval threshold required for each tier. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | ||
| pub struct SpendingPolicyConfig { |
There was a problem hiding this comment.
I think SpendingPolicyConfig isn't clearer than just SpendingPolicy, so I'd suggest renaming it to the latter.
| /// `propose_expiration_delta` controls the transaction expiration delta applied to proposal | ||
| /// transactions. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | ||
| pub struct TimelockControllerConfig { |
There was a problem hiding this comment.
To align with SpendingPolicy, could this be TimelockPolicy? I'm not sure I understand which part is a "controller".
| /// Identifies the oracle instance used by smart multisig price lookups. | ||
| /// | ||
| /// This value is stored as a `(prefix, suffix)` pair so the configured foreign price-reader | ||
| /// procedure can distinguish which oracle feed should be queried during spending-policy | ||
| /// evaluation. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct OracleId { | ||
| prefix: Felt, | ||
| suffix: Felt, | ||
| } |
There was a problem hiding this comment.
Judging from the usage, this is the ID of an account, right? If so, I think we should definitely wrap AccountId here, otherwise any two felts are considered valid at the Rust level, but will fail at the contract level.
| struct TestOracleFixture { | ||
| account: Account, | ||
| oracle_id: [Felt; 2], | ||
| get_price_proc_root: Word, | ||
| } |
There was a problem hiding this comment.
If oracle_id is always set to account.id(), I think we don't need to include it here, right? Or if we do, I'd include it as OracleId for type safety.
| create_multisig_smart_account_with_assets_and_oracle( | ||
| threshold, | ||
| public_keys, | ||
| auth_scheme, | ||
| multisig_starting_assets | ||
| .into_iter() | ||
| .map(|(account_id, amount)| FungibleAsset::new(account_id, amount).unwrap()) | ||
| .collect(), | ||
| [500, 1000, 2000, 1500], | ||
| [1, 2, 3, 4], | ||
| proc_policy_map, |
There was a problem hiding this comment.
Nit: I think this would be easier to read if it was AmountLimits::new(500, 1000, 2000, 1500), same for TierThresholds.
| loc_load.3 u32max | ||
| # => [policy_or_spending_threshold, num_verified_signatures, TX_SUMMARY_COMMITMENT] | ||
|
|
||
| # compute_tx_threshold expects default_threshold on top, policy_threshold underneath. | ||
| loc_load.1 | ||
| # => [default_threshold, policy_threshold, num_verified_signatures, TX_SUMMARY_COMMITMENT] | ||
| # => [default_threshold, policy_or_spending_threshold, num_verified_signatures, TX_SUMMARY_COMMITMENT] | ||
|
|
||
| exec.compute_tx_threshold |
There was a problem hiding this comment.
Nit: We're computing part of the threshold inline here and part of it in compute_tx_threshold. To make this easier to follow, I'd also move the u32max logic into compute_tx_threshold.
| pub use timelock_controller::update_timelock_controller | ||
| pub use timelock_controller::propose_transaction | ||
| pub use timelock_controller::cancel_transaction_proposal | ||
| pub use timelock_controller::cancel_and_propose_new_transaction | ||
| pub use timelock_controller::execute_proposed_transaction |
There was a problem hiding this comment.
Afaict, transaction proposal and execution lives in timelock_controller because this relates to the idea of "delayed" execution, right? This connection doesn't click for me that much, so I wonder if we should rename this module. Maybe miden::standards::auth::multisig_smart::delayed_execution?
2 participants
This is a draft version of
AuthMultisigSmartwhich is not ready to merge yet. The aim of this draft is to get some insights for the PR before make it ready to merge.This is a big PR and still lacks of some improvements such as:
That's being said, it's a draft PR.
Here are some the design discussion that shaped
AuthMultisigSmartthat needs be investigated before diving into it.