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

Add BoundedVec to Treasury Pallet #8665

Merged
5 commits merged into from
Apr 26, 2021
Merged

Add BoundedVec to Treasury Pallet #8665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e5c7bfb
bounded treasury approvals
shawntabrizi Apr 26, 2021
f7992b2
update benchmarks
shawntabrizi Apr 26, 2021
38916ad
update configs
shawntabrizi Apr 26, 2021
5728fa8
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Apr 26, 2021
363516c
fix weight param
shawntabrizi Apr 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,7 @@ parameter_types! {
pub const MaximumReasonLength: u32 = 16384;
pub const BountyCuratorDeposit: Permill = Permill::from_percent(50);
pub const BountyValueMinimum: Balance = 5 * DOLLARS;
pub const MaxApprovals: u32 = 100;
}

impl pallet_treasury::Config for Runtime {
Expand All @@ -742,6 +743,7 @@ impl pallet_treasury::Config for Runtime {
type BurnDestination = ();
type SpendFunds = Bounties;
type WeightInfo = pallet_treasury::weights::SubstrateWeight<Runtime>;
type MaxApprovals = MaxApprovals;
}

impl pallet_bounties::Config for Runtime {
Expand Down
2 changes: 2 additions & 0 deletions frame/bounties/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ parameter_types! {
pub const Burn: Permill = Permill::from_percent(50);
pub const DataDepositPerByte: u64 = 1;
pub const TreasuryPalletId: PalletId = PalletId(*b"py/trsry");
pub const MaxApprovals: u32 = 100;
}
// impl pallet_treasury::Config for Test {
impl pallet_treasury::Config for Test {
Expand All @@ -121,6 +122,7 @@ impl pallet_treasury::Config for Test {
type BurnDestination = (); // Just gets burned.
type WeightInfo = ();
type SpendFunds = Bounties;
type MaxApprovals = MaxApprovals;
}
parameter_types! {
pub const BountyDepositBase: u64 = 80;
Expand Down
2 changes: 2 additions & 0 deletions frame/tips/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ parameter_types! {
pub const DataDepositPerByte: u64 = 1;
pub const TreasuryPalletId: PalletId = PalletId(*b"py/trsry");
pub const MaximumReasonLength: u32 = 16384;
pub const MaxApprovals: u32 = 100;
}
impl pallet_treasury::Config for Test {
type PalletId = TreasuryPalletId;
Expand All @@ -142,6 +143,7 @@ impl pallet_treasury::Config for Test {
type BurnDestination = (); // Just gets burned.
type WeightInfo = ();
type SpendFunds = ();
type MaxApprovals = MaxApprovals;
}
parameter_types! {
pub const TipCountdown: u64 = 1;
Expand Down
6 changes: 4 additions & 2 deletions frame/treasury/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ fn create_approved_proposals<T: Config<I>, I: Instance>(n: u32) -> Result<(), &'
let proposal_id = <ProposalCount<I>>::get() - 1;
Treasury::<T, I>::approve_proposal(RawOrigin::Root.into(), proposal_id)?;
}
ensure!(<Approvals<I>>::get().len() == n as usize, "Not all approved");
ensure!(<Approvals<T, I>>::get().len() == n as usize, "Not all approved");
Ok(())
}

Expand Down Expand Up @@ -85,6 +85,8 @@ benchmarks_instance! {
}: _(RawOrigin::Root, proposal_id)

approve_proposal {
let p in 0 .. T::MaxApprovals::get() - 1;
create_approved_proposals::<T, _>(p)?;
let (caller, value, beneficiary_lookup) = setup_proposal::<T, _>(SEED);
Treasury::<T, _>::propose_spend(
RawOrigin::Signed(caller).into(),
Expand All @@ -95,7 +97,7 @@ benchmarks_instance! {
}: _(RawOrigin::Root, proposal_id)

on_initialize_proposals {
let p in 0 .. 100;
let p in 0 .. T::MaxApprovals::get();
setup_pot_account::<T, _>();
create_approved_proposals::<T, _>(p)?;
}: {
Expand Down
20 changes: 14 additions & 6 deletions frame/treasury/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,13 @@ mod benchmarking;
pub mod weights;

use sp_std::prelude::*;
use frame_support::{decl_module, decl_storage, decl_event, ensure, print, decl_error, PalletId};
use frame_support::{
decl_module, decl_storage, decl_event, ensure, print, decl_error,
PalletId, BoundedVec, bounded_vec::TryAppendValue,
Copy link
Contributor

@kianenigma kianenigma Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PalletId, BoundedVec, bounded_vec::TryAppendValue,
PalletId, BoundedVec,

Hmm are you sure we need this? We implemented it directly for storage::types so we won't need to import again.

Copy link
Member Author

@shawntabrizi shawntabrizi Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did get a compiler issue. let me see

Copy link
Member Author

@shawntabrizi shawntabrizi Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

error[E0599]: no function or associated item named `try_append` found for struct `Approvals<T, I>` in the current scope
   --> frame/treasury/src/lib.rs:329:23
    |
178 | decl_storage! {
    | ------------- function or associated item `try_append` not found for this
...
329 |             Approvals::<T, I>::try_append(proposal_id).map_err(|_| Error::<T, I>::TooManyApprovals)?;
    |                                ^^^^^^^^^^ function or associated item not found in `Approvals<T, I>`
    |
    = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
    |
66  | use crate::sp_api_hidden_includes_decl_storage::hidden_include::bounded_vec::TryAppendValue;

If you know a better fix, happy to accept a follow up pr

};
use frame_support::traits::{
Currency, Get, Imbalance, OnUnbalanced, ExistenceRequirement::KeepAlive,
ReservableCurrency, WithdrawReasons
ReservableCurrency, WithdrawReasons,
};
use sp_runtime::{
Permill, RuntimeDebug,
Expand Down Expand Up @@ -128,6 +131,9 @@ pub trait Config<I=DefaultInstance>: frame_system::Config {

/// Runtime hooks to external pallet using treasury to compute spend funds.
type SpendFunds: SpendFunds<Self, I>;

/// The maximum number of approvals that can wait in the spending queue.
type MaxApprovals: Get<u32>;
}

/// A trait to allow the Treasury Pallet to spend it's funds for other purposes.
Expand Down Expand Up @@ -180,7 +186,7 @@ decl_storage! {
=> Option<Proposal<T::AccountId, BalanceOf<T, I>>>;

/// Proposal indices that have been approved but not yet awarded.
pub Approvals get(fn approvals): Vec<ProposalIndex>;
pub Approvals get(fn approvals): BoundedVec<ProposalIndex, T::MaxApprovals>;
}
add_extra_genesis {
build(|_config| {
Expand Down Expand Up @@ -225,6 +231,8 @@ decl_error! {
InsufficientProposersBalance,
/// No proposal or bounty at that index.
InvalidIndex,
/// Too many approvals in the queue.
TooManyApprovals,
}
}

Expand Down Expand Up @@ -313,12 +321,12 @@ decl_module! {
/// - DbReads: `Proposals`, `Approvals`
/// - DbWrite: `Approvals`
/// # </weight>
#[weight = (T::WeightInfo::approve_proposal(), DispatchClass::Operational)]
#[weight = (T::WeightInfo::approve_proposal(T::MaxApprovals::get()), DispatchClass::Operational)]
pub fn approve_proposal(origin, #[compact] proposal_id: ProposalIndex) {
T::ApproveOrigin::ensure_origin(origin)?;

ensure!(<Proposals<T, I>>::contains_key(proposal_id), Error::<T, I>::InvalidIndex);
Approvals::<I>::append(proposal_id);
Approvals::<T, I>::try_append(proposal_id).map_err(|_| Error::<T, I>::TooManyApprovals)?;
}

/// # <weight>
Expand Down Expand Up @@ -365,7 +373,7 @@ impl<T: Config<I>, I: Instance> Module<T, I> {

let mut missed_any = false;
let mut imbalance = <PositiveImbalanceOf<T, I>>::zero();
let proposals_len = Approvals::<I>::mutate(|v| {
let proposals_len = Approvals::<T, I>::mutate(|v| {
let proposals_approvals_len = v.len() as u32;
v.retain(|&index| {
// Should always be true, but shouldn't panic if false or we're screwed.
Expand Down
19 changes: 19 additions & 0 deletions frame/treasury/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ parameter_types! {
pub const BountyUpdatePeriod: u32 = 20;
pub const BountyCuratorDeposit: Permill = Permill::from_percent(50);
pub const BountyValueMinimum: u64 = 1;
pub const MaxApprovals: u32 = 100;
}
impl Config for Test {
type PalletId = TreasuryPalletId;
Expand All @@ -117,6 +118,7 @@ impl Config for Test {
type BurnDestination = (); // Just gets burned.
type WeightInfo = ();
type SpendFunds = ();
type MaxApprovals = MaxApprovals;
}

pub fn new_test_ext() -> sp_io::TestExternalities {
Expand Down Expand Up @@ -359,3 +361,20 @@ fn genesis_funding_works() {
assert_eq!(Treasury::pot(), initial_funding - Balances::minimum_balance());
});
}

#[test]
fn max_approvals_limited() {
new_test_ext().execute_with(|| {
Balances::make_free_balance_be(&Treasury::account_id(), u64::max_value());
Balances::make_free_balance_be(&0, u64::max_value());

for _ in 0 .. MaxApprovals::get() {
assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3));
assert_ok!(Treasury::approve_proposal(Origin::root(), 0));
}

// One too many will fail
assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3));
assert_noop!(Treasury::approve_proposal(Origin::root(), 0), Error::<Test, _>::TooManyApprovals);
});
}
42 changes: 23 additions & 19 deletions frame/treasury/src/weights.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// This file is part of Substrate.

// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd.
// Copyright (C) 2021 Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -17,12 +17,12 @@

//! Autogenerated weights for pallet_treasury
//!
//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0
//! DATE: 2020-12-16, STEPS: `[50, ]`, REPEAT: 20, LOW RANGE: [], HIGH RANGE: []
//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 3.0.0
//! DATE: 2021-04-26, STEPS: `[50, ]`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
//! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128

// Executed Command:
// ./target/release/substrate
// target/release/substrate
// benchmark
// --chain=dev
// --steps=50
Expand All @@ -46,32 +46,34 @@ use sp_std::marker::PhantomData;
pub trait WeightInfo {
fn propose_spend() -> Weight;
fn reject_proposal() -> Weight;
fn approve_proposal() -> Weight;
fn approve_proposal(p: u32, ) -> Weight;
fn on_initialize_proposals(p: u32, ) -> Weight;
}

/// Weights for pallet_treasury using the Substrate node and recommended hardware.
pub struct SubstrateWeight<T>(PhantomData<T>);
impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
fn propose_spend() -> Weight {
(59_986_000 as Weight)
(45_393_000 as Weight)
.saturating_add(T::DbWeight::get().reads(1 as Weight))
.saturating_add(T::DbWeight::get().writes(2 as Weight))
}
fn reject_proposal() -> Weight {
(48_300_000 as Weight)
(42_796_000 as Weight)
.saturating_add(T::DbWeight::get().reads(2 as Weight))
.saturating_add(T::DbWeight::get().writes(2 as Weight))
}
fn approve_proposal() -> Weight {
(14_054_000 as Weight)
fn approve_proposal(p: u32, ) -> Weight {
(14_153_000 as Weight)
// Standard Error: 1_000
.saturating_add((94_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))
}
fn on_initialize_proposals(p: u32, ) -> Weight {
(86_038_000 as Weight)
// Standard Error: 18_000
.saturating_add((78_781_000 as Weight).saturating_mul(p as Weight))
(51_633_000 as Weight)
// Standard Error: 42_000
.saturating_add((65_705_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 @@ -82,24 +84,26 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// For backwards compatibility and tests
impl WeightInfo for () {
fn propose_spend() -> Weight {
(59_986_000 as Weight)
(45_393_000 as Weight)
.saturating_add(RocksDbWeight::get().reads(1 as Weight))
.saturating_add(RocksDbWeight::get().writes(2 as Weight))
}
fn reject_proposal() -> Weight {
(48_300_000 as Weight)
(42_796_000 as Weight)
.saturating_add(RocksDbWeight::get().reads(2 as Weight))
.saturating_add(RocksDbWeight::get().writes(2 as Weight))
}
fn approve_proposal() -> Weight {
(14_054_000 as Weight)
fn approve_proposal(p: u32, ) -> Weight {
(14_153_000 as Weight)
// Standard Error: 1_000
.saturating_add((94_000 as Weight).saturating_mul(p as Weight))
.saturating_add(RocksDbWeight::get().reads(2 as Weight))
.saturating_add(RocksDbWeight::get().writes(1 as Weight))
}
fn on_initialize_proposals(p: u32, ) -> Weight {
(86_038_000 as Weight)
// Standard Error: 18_000
.saturating_add((78_781_000 as Weight).saturating_mul(p as Weight))
(51_633_000 as Weight)
// Standard Error: 42_000
.saturating_add((65_705_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