Make max_weight optional in multisig.approve. (#1716)

Co-authored-by: Adam Dossa <[email protected]>
PolymeshAssociation · Sep 17, 2024 · 6a479a8 · 6a479a8
1 parent db18fa5
commit 6a479a8
Show file tree

Hide file tree

Showing 5 changed files with 26 additions and 21 deletions.
diff --git 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
@@ -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>) -> Weight {
+        max_weight.unwrap_or_else(|| {
+            // TODO: Use a better default weight.
+            Self::create_proposal()
+        })
+    }
+
+    fn approve_and_execute(max_weight: &Option<Weight>) -> 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
@@ -238,7 +238,7 @@ benchmarks! {
 
     approve {
         let (alice, multisig, signers, users, proposal_id, proposal, ephemeral_multisig) = generate_multisig_and_create_proposal::<T>(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
@@ -264,17 +264,14 @@ pub mod pallet {
         ///
         /// If quorum is reached, the proposal will be immediately executed.
         #[pallet::call_index(2)]
-        #[pallet::weight({
-          <T as Config>::WeightInfo::approve()
-            .saturating_add(<T as Config>::WeightInfo::execute_proposal())
-            .saturating_add(*max_weight)
-        })]
+        #[pallet::weight(<T as Config>::WeightInfo::approve_and_execute(max_weight))]
         pub fn approve(
             origin: OriginFor<T>,
             multisig: T::AccountId,
             proposal_id: u64,
-            max_weight: Weight,
+            max_weight: Option<Weight>,
         ) -> DispatchResultWithPostInfo {
+            let max_weight = <T as Config>::WeightInfo::default_max_weight(&max_weight);
             let signer = ensure_signed(origin)?;
             with_base_weight(<T as Config>::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
@@ -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::<TestStorage>::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::<TestStorage>::get(&ms_address, proposal_id).unwrap();