From 6a479a835447fb83e147299a868c02790a45873d Mon Sep 17 00:00:00 2001 From: Robert Gabriel Jakabosky Date: Tue, 17 Sep 2024 13:53:17 -0700 Subject: [PATCH] Make max_weight optional in multisig.approve. (#1716) Co-authored-by: Adam Dossa --- integration/tests/multisig_permissions.rs | 2 +- pallets/common/src/traits/multisig.rs | 13 +++++++++++++ pallets/multisig/src/benchmarking.rs | 2 +- pallets/multisig/src/lib.rs | 9 +++------ pallets/runtime/tests/src/multisig.rs | 21 ++++++++------------- 5 files changed, 26 insertions(+), 21 deletions(-) diff --git a/integration/tests/multisig_permissions.rs b/integration/tests/multisig_permissions.rs index 3045812099..732883962b 100644 --- a/integration/tests/multisig_permissions.rs +++ b/integration/tests/multisig_permissions.rs @@ -262,7 +262,7 @@ impl MuliSigState { self.api .call() .multi_sig() - .approve(self.account.clone(), id, weight)?; + .approve(self.account.clone(), id, Some(weight))?; let mut results = Vec::new(); for signer in &mut self.signers[1..self.sigs_required as usize] { let res = approve_call.submit_and_watch(signer).await?; diff --git a/pallets/common/src/traits/multisig.rs b/pallets/common/src/traits/multisig.rs index 9bf58e9fab..69bc4de65d 100644 --- a/pallets/common/src/traits/multisig.rs +++ b/pallets/common/src/traits/multisig.rs @@ -40,6 +40,19 @@ pub trait WeightInfo { fn create_join_identity() -> Weight; fn approve_join_identity() -> Weight; fn join_identity() -> Weight; + + fn default_max_weight(max_weight: &Option) -> Weight { + max_weight.unwrap_or_else(|| { + // TODO: Use a better default weight. + Self::create_proposal() + }) + } + + fn approve_and_execute(max_weight: &Option) -> Weight { + Self::approve() + .saturating_add(Self::execute_proposal()) + .saturating_add(Self::default_max_weight(max_weight)) + } } /// This trait is used to add a signer to a multisig and enable unlinking multisig from an identity diff --git a/pallets/multisig/src/benchmarking.rs b/pallets/multisig/src/benchmarking.rs index 01f906a705..f03e94d77c 100644 --- a/pallets/multisig/src/benchmarking.rs +++ b/pallets/multisig/src/benchmarking.rs @@ -238,7 +238,7 @@ benchmarks! { approve { let (alice, multisig, signers, users, proposal_id, proposal, ephemeral_multisig) = generate_multisig_and_create_proposal::(3, 3).unwrap(); - }: _(users[2].origin(), ephemeral_multisig, proposal_id, Weight::MAX) + }: _(users[2].origin(), ephemeral_multisig, proposal_id, None) verify { assert_vote_cast!(proposal_id, multisig, signers.last().unwrap()); } diff --git a/pallets/multisig/src/lib.rs b/pallets/multisig/src/lib.rs index f93e7432ca..4d3941992c 100644 --- a/pallets/multisig/src/lib.rs +++ b/pallets/multisig/src/lib.rs @@ -264,17 +264,14 @@ pub mod pallet { /// /// If quorum is reached, the proposal will be immediately executed. #[pallet::call_index(2)] - #[pallet::weight({ - ::WeightInfo::approve() - .saturating_add(::WeightInfo::execute_proposal()) - .saturating_add(*max_weight) - })] + #[pallet::weight(::WeightInfo::approve_and_execute(max_weight))] pub fn approve( origin: OriginFor, multisig: T::AccountId, proposal_id: u64, - max_weight: Weight, + max_weight: Option, ) -> DispatchResultWithPostInfo { + let max_weight = ::WeightInfo::default_max_weight(&max_weight); let signer = ensure_signed(origin)?; with_base_weight(::WeightInfo::approve(), || { Self::base_approve(&multisig, signer, proposal_id, max_weight) diff --git a/pallets/runtime/tests/src/multisig.rs b/pallets/runtime/tests/src/multisig.rs index 61e196c143..e1afaf26e8 100644 --- a/pallets/runtime/tests/src/multisig.rs +++ b/pallets/runtime/tests/src/multisig.rs @@ -1,6 +1,6 @@ use frame_support::{ assert_err, assert_err_ignore_postinfo, assert_noop, assert_ok, assert_storage_noop, - dispatch::DispatchResult, dispatch::Weight, BoundedVec, + dispatch::DispatchResult, BoundedVec, }; use pallet_multisig::{self as multisig, AdminDid, ProposalStates, ProposalVoteCounts, Votes}; @@ -246,7 +246,7 @@ fn change_multisig_sigs_required() { charlie.clone(), ms_address.clone(), 0, - Weight::MAX + None )); next_block(); assert_eq!(MultiSig::ms_signs_required(ms_address), 1); @@ -768,7 +768,7 @@ fn check_for_approval_closure() { let multi_purpose_nonce = Identity::multi_purpose_nonce(); assert_storage_noop!(assert_err_ignore_postinfo!( - MultiSig::approve(dave.clone(), ms_address.clone(), proposal_id, Weight::MAX), + MultiSig::approve(dave.clone(), ms_address.clone(), proposal_id, None), Error::ProposalAlreadyExecuted )); @@ -854,7 +854,7 @@ fn reject_proposals() { proposal_id1 )); assert_storage_noop!(assert_err_ignore_postinfo!( - MultiSig::approve(dave.clone(), ms_address.clone(), proposal_id1, Weight::MAX), + MultiSig::approve(dave.clone(), ms_address.clone(), proposal_id1, None), Error::ProposalAlreadyRejected )); let vote_count1 = @@ -882,7 +882,7 @@ fn reject_proposals() { proposal_id2 )); assert_storage_noop!(assert_err_ignore_postinfo!( - MultiSig::approve(dave.clone(), ms_address.clone(), proposal_id2, Weight::MAX), + MultiSig::approve(dave.clone(), ms_address.clone(), proposal_id2, None), Error::ProposalAlreadyRejected )); @@ -1299,7 +1299,7 @@ fn expired_proposals() { bob.clone(), ms_address.clone(), proposal_id, - Weight::MAX + None )); vote_count = ProposalVoteCounts::::get(&ms_address, proposal_id).unwrap(); @@ -1315,12 +1315,7 @@ fn expired_proposals() { // Approval fails when proposal has expired set_timestamp(expires_at); assert_noop!( - MultiSig::approve( - charlie.clone(), - ms_address.clone(), - proposal_id, - Weight::MAX - ), + MultiSig::approve(charlie.clone(), ms_address.clone(), proposal_id, None), Error::ProposalExpired ); @@ -1340,7 +1335,7 @@ fn expired_proposals() { charlie, ms_address.clone(), proposal_id, - Weight::MAX + None )); vote_count = ProposalVoteCounts::::get(&ms_address, proposal_id).unwrap();