From e2f59dadccd4534c2a6ff3240d784f450c7a1257 Mon Sep 17 00:00:00 2001 From: Chigozie Joshua <36326251+guzzit@users.noreply.github.com> Date: Wed, 27 Apr 2022 10:23:46 +0100 Subject: [PATCH] Adds remove_approval feature to treasury pallet (#11243) * added remove_approval feature to treasury pallet * Update frame/treasury/src/lib.rs Co-authored-by: Xiliang Chen * treasury pallet refactoring: removed proposalId check in remove_approval call and trailing new line in tests file * fmt * cargo run --quiet --profile=production --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark pallet --chain=dev --steps=50 --repeat=20 --pallet=pallet_treasury --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/treasury/src/weights.rs --template=./.maintain/frame-weight-template.hbs * Update frame/treasury/src/lib.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * remove_approval extrinsic in treasury pallet:added extra checks in test and extra documentation * refactored error logging on remove_approval extrinsic in treasury pallet Co-authored-by: Xiliang Chen Co-authored-by: Shawn Tabrizi Co-authored-by: Parity Bot Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> --- frame/treasury/src/benchmarking.rs | 11 ++++++ frame/treasury/src/lib.rs | 37 ++++++++++++++++++++ frame/treasury/src/tests.rs | 18 ++++++++++ frame/treasury/src/weights.rs | 54 ++++++++++++++++++------------ 4 files changed, 99 insertions(+), 21 deletions(-) diff --git a/frame/treasury/src/benchmarking.rs b/frame/treasury/src/benchmarking.rs index a0dd58ee3d42a..47bc1b9ea99de 100644 --- a/frame/treasury/src/benchmarking.rs +++ b/frame/treasury/src/benchmarking.rs @@ -87,6 +87,17 @@ benchmarks_instance_pallet! { let proposal_id = Treasury::::proposal_count() - 1; }: _(RawOrigin::Root, proposal_id) + remove_approval { + let (caller, value, beneficiary_lookup) = setup_proposal::(SEED); + Treasury::::propose_spend( + RawOrigin::Signed(caller).into(), + value, + beneficiary_lookup + )?; + let proposal_id = Treasury::::proposal_count() - 1; + Treasury::::approve_proposal(RawOrigin::Root.into(), proposal_id)?; + }: _(RawOrigin::Root, proposal_id) + on_initialize_proposals { let p in 0 .. T::MaxApprovals::get(); setup_pot_account::(); diff --git a/frame/treasury/src/lib.rs b/frame/treasury/src/lib.rs index 81fca5243afa3..3f76ddb639c5b 100644 --- a/frame/treasury/src/lib.rs +++ b/frame/treasury/src/lib.rs @@ -50,6 +50,7 @@ //! - `propose_spend` - Make a spending proposal and stake the required deposit. //! - `reject_proposal` - Reject a proposal, slashing the deposit. //! - `approve_proposal` - Accept the proposal, returning the deposit. +//! - `remove_approval` - Remove an approval, the deposit will no longer be returned. //! //! ## GenesisConfig //! @@ -289,6 +290,8 @@ pub mod pallet { InvalidIndex, /// Too many approvals in the queue. TooManyApprovals, + /// Proposal has not been approved. + ProposalNotApproved, } #[pallet::hooks] @@ -393,6 +396,40 @@ pub mod pallet { .map_err(|_| Error::::TooManyApprovals)?; Ok(()) } + + /// Force a previously approved proposal to be removed from the approval queue. + /// The original deposit will no longer be returned. + /// + /// May only be called from `T::RejectOrigin`. + /// - `proposal_id`: The index of a proposal + /// + /// # + /// - Complexity: O(A) where `A` is the number of approvals + /// - Db reads and writes: `Approvals` + /// # + /// + /// Errors: + /// - `ProposalNotApproved`: The `proposal_id` supplied was not found in the approval queue, + /// i.e., the proposal has not been approved. This could also mean the proposal does not + /// exist altogether, thus there is no way it would have been approved in the first place. + #[pallet::weight((T::WeightInfo::remove_approval(), DispatchClass::Operational))] + pub fn remove_approval( + origin: OriginFor, + #[pallet::compact] proposal_id: ProposalIndex, + ) -> DispatchResult { + T::RejectOrigin::ensure_origin(origin)?; + + Approvals::::try_mutate(|v| -> DispatchResult { + if let Some(index) = v.iter().position(|x| x == &proposal_id) { + v.remove(index); + Ok(()) + } else { + Err(Error::::ProposalNotApproved.into()) + } + })?; + + Ok(()) + } } } diff --git a/frame/treasury/src/tests.rs b/frame/treasury/src/tests.rs index 26189f5201498..a67bbe9b1d1e9 100644 --- a/frame/treasury/src/tests.rs +++ b/frame/treasury/src/tests.rs @@ -388,3 +388,21 @@ fn max_approvals_limited() { ); }); } + +#[test] +fn remove_already_removed_approval_fails() { + new_test_ext().execute_with(|| { + Balances::make_free_balance_be(&Treasury::account_id(), 101); + + assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3)); + assert_ok!(Treasury::approve_proposal(Origin::root(), 0)); + assert_eq!(Treasury::approvals(), vec![0]); + assert_ok!(Treasury::remove_approval(Origin::root(), 0)); + assert_eq!(Treasury::approvals(), vec![]); + + assert_noop!( + Treasury::remove_approval(Origin::root(), 0), + Error::::ProposalNotApproved + ); + }); +} diff --git a/frame/treasury/src/weights.rs b/frame/treasury/src/weights.rs index dcbf5983fa65c..c846a7f6885e9 100644 --- a/frame/treasury/src/weights.rs +++ b/frame/treasury/src/weights.rs @@ -18,12 +18,13 @@ //! Autogenerated weights for pallet_treasury //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev -//! DATE: 2022-01-31, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2022-04-23, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 1024 // Executed Command: -// ./target/production/substrate +// target/production/substrate // benchmark +// pallet // --chain=dev // --steps=50 // --repeat=20 @@ -33,9 +34,7 @@ // --wasm-execution=compiled // --heap-pages=4096 // --output=./frame/treasury/src/weights.rs -// --template=.maintain/frame-weight-template.hbs -// --header=HEADER-APACHE2 -// --raw +// --template=./.maintain/frame-weight-template.hbs #![cfg_attr(rustfmt, rustfmt_skip)] #![allow(unused_parens)] @@ -49,6 +48,7 @@ pub trait WeightInfo { fn propose_spend() -> Weight; fn reject_proposal() -> Weight; fn approve_proposal(p: u32, ) -> Weight; + fn remove_approval() -> Weight; fn on_initialize_proposals(p: u32, ) -> Weight; } @@ -58,34 +58,40 @@ impl WeightInfo for SubstrateWeight { // Storage: Treasury ProposalCount (r:1 w:1) // Storage: Treasury Proposals (r:0 w:1) fn propose_spend() -> Weight { - (21_673_000 as Weight) + (22_063_000 as Weight) .saturating_add(T::DbWeight::get().reads(1 as Weight)) .saturating_add(T::DbWeight::get().writes(2 as Weight)) } // Storage: Treasury Proposals (r:1 w:1) // Storage: System Account (r:1 w:1) fn reject_proposal() -> Weight { - (25_353_000 as Weight) + (25_965_000 as Weight) .saturating_add(T::DbWeight::get().reads(2 as Weight)) .saturating_add(T::DbWeight::get().writes(2 as Weight)) } // Storage: Treasury Proposals (r:1 w:0) // Storage: Treasury Approvals (r:1 w:1) fn approve_proposal(p: u32, ) -> Weight { - (8_164_000 as Weight) - // Standard Error: 1_000 - .saturating_add((57_000 as Weight).saturating_mul(p as Weight)) + (7_299_000 as Weight) + // Standard Error: 0 + .saturating_add((103_000 as Weight).saturating_mul(p as Weight)) .saturating_add(T::DbWeight::get().reads(2 as Weight)) .saturating_add(T::DbWeight::get().writes(1 as Weight)) } // Storage: Treasury Approvals (r:1 w:1) + fn remove_approval() -> Weight { + (3_867_000 as Weight) + .saturating_add(T::DbWeight::get().reads(1 as Weight)) + .saturating_add(T::DbWeight::get().writes(1 as Weight)) + } + // Storage: Treasury Approvals (r:1 w:1) // Storage: Bounties BountyApprovals (r:1 w:1) // Storage: Treasury Proposals (r:2 w:2) // Storage: System Account (r:4 w:4) fn on_initialize_proposals(p: u32, ) -> Weight { - (20_762_000 as Weight) - // Standard Error: 21_000 - .saturating_add((26_835_000 as Weight).saturating_mul(p as Weight)) + (24_020_000 as Weight) + // Standard Error: 22_000 + .saturating_add((28_198_000 as Weight).saturating_mul(p as Weight)) .saturating_add(T::DbWeight::get().reads(2 as Weight)) .saturating_add(T::DbWeight::get().reads((3 as Weight).saturating_mul(p as Weight))) .saturating_add(T::DbWeight::get().writes(2 as Weight)) @@ -98,34 +104,40 @@ impl WeightInfo for () { // Storage: Treasury ProposalCount (r:1 w:1) // Storage: Treasury Proposals (r:0 w:1) fn propose_spend() -> Weight { - (21_673_000 as Weight) + (22_063_000 as Weight) .saturating_add(RocksDbWeight::get().reads(1 as Weight)) .saturating_add(RocksDbWeight::get().writes(2 as Weight)) } // Storage: Treasury Proposals (r:1 w:1) // Storage: System Account (r:1 w:1) fn reject_proposal() -> Weight { - (25_353_000 as Weight) + (25_965_000 as Weight) .saturating_add(RocksDbWeight::get().reads(2 as Weight)) .saturating_add(RocksDbWeight::get().writes(2 as Weight)) } // Storage: Treasury Proposals (r:1 w:0) // Storage: Treasury Approvals (r:1 w:1) fn approve_proposal(p: u32, ) -> Weight { - (8_164_000 as Weight) - // Standard Error: 1_000 - .saturating_add((57_000 as Weight).saturating_mul(p as Weight)) + (7_299_000 as Weight) + // Standard Error: 0 + .saturating_add((103_000 as Weight).saturating_mul(p as Weight)) .saturating_add(RocksDbWeight::get().reads(2 as Weight)) .saturating_add(RocksDbWeight::get().writes(1 as Weight)) } // Storage: Treasury Approvals (r:1 w:1) + fn remove_approval() -> Weight { + (3_867_000 as Weight) + .saturating_add(RocksDbWeight::get().reads(1 as Weight)) + .saturating_add(RocksDbWeight::get().writes(1 as Weight)) + } + // Storage: Treasury Approvals (r:1 w:1) // Storage: Bounties BountyApprovals (r:1 w:1) // Storage: Treasury Proposals (r:2 w:2) // Storage: System Account (r:4 w:4) fn on_initialize_proposals(p: u32, ) -> Weight { - (20_762_000 as Weight) - // Standard Error: 21_000 - .saturating_add((26_835_000 as Weight).saturating_mul(p as Weight)) + (24_020_000 as Weight) + // Standard Error: 22_000 + .saturating_add((28_198_000 as Weight).saturating_mul(p as Weight)) .saturating_add(RocksDbWeight::get().reads(2 as Weight)) .saturating_add(RocksDbWeight::get().reads((3 as Weight).saturating_mul(p as Weight))) .saturating_add(RocksDbWeight::get().writes(2 as Weight))