Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
Adds remove_approval feature to treasury pallet (#11243)
Browse files Browse the repository at this point in the history 
* added remove_approval feature to treasury pallet

* Update frame/treasury/src/lib.rs

Co-authored-by: Xiliang Chen <xlchen1291@gmail.com>

* 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 <xlchen1291@gmail.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Parity Bot <admin@parity.io>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
  • Loading branch information
@guzzit @xlc
@shawntabrizi @joepetrowski
5 people authored Apr 27, 2022
1 parent 6e740bc commit 3a02a69
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 21 deletions.
11 changes: 11 additions & 0 deletions frame/treasury/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,17 @@ benchmarks_instance_pallet! {
let proposal_id = Treasury::<T, _>::proposal_count() - 1;
}: _(RawOrigin::Root, proposal_id)

remove_approval {
let (caller, value, beneficiary_lookup) = setup_proposal::<T, _>(SEED);
Treasury::<T, _>::propose_spend(
RawOrigin::Signed(caller).into(),
value,
beneficiary_lookup
)?;
let proposal_id = Treasury::<T, _>::proposal_count() - 1;
Treasury::<T, I>::approve_proposal(RawOrigin::Root.into(), proposal_id)?;
}: _(RawOrigin::Root, proposal_id)

on_initialize_proposals {
let p in 0 .. T::MaxApprovals::get();
setup_pot_account::<T, _>();
Expand Down
37 changes: 37 additions & 0 deletions frame/treasury/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
//!
Expand Down Expand Up @@ -289,6 +290,8 @@ pub mod pallet {
InvalidIndex,
/// Too many approvals in the queue.
TooManyApprovals,
/// Proposal has not been approved.
ProposalNotApproved,
}

#[pallet::hooks]
Expand Down Expand Up @@ -393,6 +396,40 @@ pub mod pallet {
.map_err(|_| Error::<T, I>::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
///
/// # <weight>
/// - Complexity: O(A) where `A` is the number of approvals
/// - Db reads and writes: `Approvals`
/// # </weight>
///
/// 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<T>,
#[pallet::compact] proposal_id: ProposalIndex,
) -> DispatchResult {
T::RejectOrigin::ensure_origin(origin)?;

Approvals::<T, I>::try_mutate(|v| -> DispatchResult {
if let Some(index) = v.iter().position(|x| x == &proposal_id) {
v.remove(index);
Ok(())
} else {
Err(Error::<T, I>::ProposalNotApproved.into())
}
})?;

Ok(())
}
}
}

Expand Down
18 changes: 18 additions & 0 deletions frame/treasury/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Test, _>::ProposalNotApproved
);
});
}
54 changes: 33 additions & 21 deletions frame/treasury/src/weights.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)]
Expand All @@ -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;
}

Expand All @@ -58,34 +58,40 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// 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))
Expand All @@ -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))
Expand Down

0 comments on commit 3a02a69

Please sign in to comment.