diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs index 5e956bcdfee5..ea9c1b64e9c3 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs @@ -332,4 +332,5 @@ impl pallet_treasury::Config for Runtime { sp_core::ConstU8<1>, ConstU32<1000>, >; + type BlockNumberProvider = crate::System; } diff --git a/polkadot/runtime/common/src/impls.rs b/polkadot/runtime/common/src/impls.rs index 709cc69cdbea..e00a4661e97f 100644 --- a/polkadot/runtime/common/src/impls.rs +++ b/polkadot/runtime/common/src/impls.rs @@ -344,6 +344,7 @@ mod tests { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<0>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index cb40a43a9974..8d206c349884 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -530,6 +530,7 @@ impl pallet_treasury::Config for Runtime { AssetRate, >; type PayoutPeriod = PayoutSpendPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = polkadot_runtime_common::impls::benchmarks::TreasuryArguments; } diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index aefc4e2db375..a7119ee83c37 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -715,6 +715,7 @@ impl pallet_treasury::Config for Runtime { AssetRate, >; type PayoutPeriod = PayoutSpendPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = polkadot_runtime_common::impls::benchmarks::TreasuryArguments; } diff --git a/prdoc/pr_3970.prdoc b/prdoc/pr_3970.prdoc new file mode 100644 index 000000000000..5c20e7444782 --- /dev/null +++ b/prdoc/pr_3970.prdoc @@ -0,0 +1,17 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Update Treasury to Support Block Number Provider + +doc: + - audience: Runtime Dev + description: | + The goal of this PR is to have the treasury pallet work on a parachain which does not produce blocks on a regular schedule, thus can use the relay chain as a block provider. Because blocks are not produced regularly, we cannot make the assumption that block number increases monotonically, and thus have new logic to handle multiple spend periods passing between blocks. To migrate existing treasury implementations, simply add `type BlockNumberProvider = System` to have the same behavior as before. + +crates: +- name: pallet-treasury + bump: major +- name: pallet-bounties + bump: minor +- name: pallet-child-bounties + bump: minor diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 9bde1bc5c858..84bf1f46d918 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -1247,6 +1247,7 @@ impl pallet_treasury::Config for Runtime { type Paymaster = PayAssetFromAccount; type BalanceConverter = AssetRate; type PayoutPeriod = SpendPayoutPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } diff --git a/substrate/frame/bounties/src/benchmarking.rs b/substrate/frame/bounties/src/benchmarking.rs index a009b132f059..7d12fce66df7 100644 --- a/substrate/frame/bounties/src/benchmarking.rs +++ b/substrate/frame/bounties/src/benchmarking.rs @@ -25,13 +25,17 @@ use frame_benchmarking::v1::{ account, benchmarks_instance_pallet, whitelisted_caller, BenchmarkError, }; use frame_system::{pallet_prelude::BlockNumberFor, RawOrigin}; -use sp_runtime::traits::Bounded; +use sp_runtime::traits::{BlockNumberProvider, Bounded}; use crate::Pallet as Bounties; use pallet_treasury::Pallet as Treasury; const SEED: u32 = 0; +fn set_block_number, I: 'static>(n: BlockNumberFor) { + >::BlockNumberProvider::set_block_number(n); +} + // Create bounties that are approved for use in `on_initialize`. fn create_approved_bounties, I: 'static>(n: u32) -> Result<(), BenchmarkError> { for i in 0..n { @@ -77,7 +81,8 @@ fn create_bounty, I: 'static>( let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin.clone(), bounty_id)?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + set_block_number::(T::SpendPeriod::get()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); Bounties::::propose_curator(approve_origin, bounty_id, curator_lookup.clone(), fee)?; Bounties::::accept_curator(RawOrigin::Signed(curator).into(), bounty_id)?; Ok((curator_lookup, bounty_id)) @@ -115,16 +120,17 @@ benchmarks_instance_pallet! { let bounty_id = BountyCount::::get() - 1; let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin.clone(), bounty_id)?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + set_block_number::(T::SpendPeriod::get()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); }: _(approve_origin, bounty_id, curator_lookup, fee) // Worst case when curator is inactive and any sender unassigns the curator. unassign_curator { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); let bounty_id = BountyCount::::get() - 1; - frame_system::Pallet::::set_block_number(T::BountyUpdatePeriod::get() + 2u32.into()); + set_block_number::(T::SpendPeriod::get() + T::BountyUpdatePeriod::get() + 2u32.into()); let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), bounty_id) @@ -136,14 +142,15 @@ benchmarks_instance_pallet! { let bounty_id = BountyCount::::get() - 1; let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin.clone(), bounty_id)?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + set_block_number::(T::SpendPeriod::get()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); Bounties::::propose_curator(approve_origin, bounty_id, curator_lookup, fee)?; }: _(RawOrigin::Signed(curator), bounty_id) award_bounty { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); let bounty_id = BountyCount::::get() - 1; let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?; @@ -154,7 +161,7 @@ benchmarks_instance_pallet! { claim_bounty { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); let bounty_id = BountyCount::::get() - 1; let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?; @@ -163,7 +170,7 @@ benchmarks_instance_pallet! { let beneficiary = T::Lookup::unlookup(beneficiary_account.clone()); Bounties::::award_bounty(RawOrigin::Signed(curator.clone()).into(), bounty_id, beneficiary)?; - frame_system::Pallet::::set_block_number(T::BountyDepositPayoutDelay::get() + 1u32.into()); + set_block_number::(T::SpendPeriod::get() + T::BountyDepositPayoutDelay::get() + 1u32.into()); ensure!(T::Currency::free_balance(&beneficiary_account).is_zero(), "Beneficiary already has balance"); }: _(RawOrigin::Signed(curator), bounty_id) @@ -183,7 +190,7 @@ benchmarks_instance_pallet! { close_bounty_active { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); let bounty_id = BountyCount::::get() - 1; let approve_origin = T::RejectOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; @@ -195,7 +202,7 @@ benchmarks_instance_pallet! { extend_bounty_expiry { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); let bounty_id = BountyCount::::get() - 1; let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?; diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs index e27ecbda3a2b..79d0399a221d 100644 --- a/substrate/frame/bounties/src/lib.rs +++ b/substrate/frame/bounties/src/lib.rs @@ -94,7 +94,7 @@ use frame_support::traits::{ }; use sp_runtime::{ - traits::{AccountIdConversion, BadOrigin, Saturating, StaticLookup, Zero}, + traits::{AccountIdConversion, BadOrigin, BlockNumberProvider, Saturating, StaticLookup, Zero}, DispatchResult, Permill, RuntimeDebug, }; @@ -486,7 +486,7 @@ pub mod pallet { // If the sender is not the curator, and the curator is inactive, // slash the curator. if sender != *curator { - let block_number = frame_system::Pallet::::block_number(); + let block_number = Self::treasury_block_number(); if *update_due < block_number { slash_curator(curator, &mut bounty.curator_deposit); // Continue to change bounty status below... @@ -550,8 +550,8 @@ pub mod pallet { T::Currency::reserve(curator, deposit)?; bounty.curator_deposit = deposit; - let update_due = frame_system::Pallet::::block_number() + - T::BountyUpdatePeriod::get(); + let update_due = + Self::treasury_block_number() + T::BountyUpdatePeriod::get(); bounty.status = BountyStatus::Active { curator: curator.clone(), update_due }; @@ -605,8 +605,7 @@ pub mod pallet { bounty.status = BountyStatus::PendingPayout { curator: signer, beneficiary: beneficiary.clone(), - unlock_at: frame_system::Pallet::::block_number() + - T::BountyDepositPayoutDelay::get(), + unlock_at: Self::treasury_block_number() + T::BountyDepositPayoutDelay::get(), }; Ok(()) @@ -637,10 +636,7 @@ pub mod pallet { if let BountyStatus::PendingPayout { curator, beneficiary, unlock_at } = bounty.status { - ensure!( - frame_system::Pallet::::block_number() >= unlock_at, - Error::::Premature - ); + ensure!(Self::treasury_block_number() >= unlock_at, Error::::Premature); let bounty_account = Self::bounty_account_id(bounty_id); let balance = T::Currency::free_balance(&bounty_account); let fee = bounty.fee.min(balance); // just to be safe @@ -793,7 +789,7 @@ pub mod pallet { match bounty.status { BountyStatus::Active { ref curator, ref mut update_due } => { ensure!(*curator == signer, Error::::RequireCurator); - *update_due = (frame_system::Pallet::::block_number() + + *update_due = (Self::treasury_block_number() + T::BountyUpdatePeriod::get()) .max(*update_due); }, @@ -810,6 +806,14 @@ pub mod pallet { } impl, I: 'static> Pallet { + /// Get the block number used in the treasury pallet. + /// + /// It may be configured to use the relay chain block number on a parachain. + pub fn treasury_block_number() -> BlockNumberFor { + >::BlockNumberProvider::current_block_number() + } + + /// Calculate the deposit required for a curator. pub fn calculate_curator_deposit(fee: &BalanceOf) -> BalanceOf { let mut deposit = T::CuratorDepositMultiplier::get() * *fee; diff --git a/substrate/frame/bounties/src/tests.rs b/substrate/frame/bounties/src/tests.rs index 9f3a52263ec8..3f95cc0b408c 100644 --- a/substrate/frame/bounties/src/tests.rs +++ b/substrate/frame/bounties/src/tests.rs @@ -40,6 +40,12 @@ use super::Event as BountiesEvent; type Block = frame_system::mocking::MockBlock; +// This function directly jumps to a block number, and calls `on_initialize`. +fn go_to_block(n: u64) { + ::BlockNumberProvider::set_block_number(n); + >::on_initialize(n); +} + frame_support::construct_runtime!( pub enum Test { @@ -98,6 +104,7 @@ impl pallet_treasury::Config for Test { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<10>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } @@ -120,6 +127,7 @@ impl pallet_treasury::Config for Test { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<10>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } @@ -214,7 +222,7 @@ fn accepted_spend_proposal_ignored_outside_spend_period() { assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 100, 3) }); - >::on_initialize(1); + go_to_block(1); assert_eq!(Balances::free_balance(3), 0); assert_eq!(Treasury::pot(), 100); }); @@ -227,7 +235,7 @@ fn unused_pot_should_diminish() { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::total_issuance(), init_total_issuance + 100); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 50); assert_eq!(Balances::total_issuance(), init_total_issuance + 50); }); @@ -241,7 +249,7 @@ fn accepted_spend_proposal_enacted_on_spend_period() { assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 100, 3) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Balances::free_balance(3), 100); assert_eq!(Treasury::pot(), 0); }); @@ -255,11 +263,11 @@ fn pot_underflow_should_not_diminish() { assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 150, 3) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed assert_ok!(Balances::deposit_into_existing(&Treasury::account_id(), 100)); - >::on_initialize(4); + go_to_block(4); assert_eq!(Balances::free_balance(3), 150); // Fund has been spent assert_eq!(Treasury::pot(), 25); // Pot has finally changed }); @@ -276,12 +284,12 @@ fn treasury_account_doesnt_get_deleted() { assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), treasury_balance, 3) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), Treasury::pot(), 3) }); - >::on_initialize(4); + go_to_block(4); assert_eq!(Treasury::pot(), 0); // Pot is emptied assert_eq!(Balances::free_balance(Treasury::account_id()), 1); // but the account is still there }); @@ -304,7 +312,8 @@ fn inexistent_account_works() { assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 99, 3) }); assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 1, 3) }); - >::on_initialize(2); + go_to_block(2); + assert_eq!(Treasury::pot(), 0); // Pot hasn't changed assert_eq!(Balances::free_balance(3), 0); // Balance of `3` hasn't changed @@ -312,7 +321,7 @@ fn inexistent_account_works() { assert_eq!(Treasury::pot(), 99); // Pot now contains funds assert_eq!(Balances::free_balance(Treasury::account_id()), 100); // Account does exist - >::on_initialize(4); + go_to_block(4); assert_eq!(Treasury::pot(), 0); // Pot has changed assert_eq!(Balances::free_balance(3), 99); // Balance of `3` has changed @@ -322,8 +331,6 @@ fn inexistent_account_works() { #[test] fn propose_bounty_works() { new_test_ext().execute_with(|| { - System::set_block_number(1); - Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); @@ -359,8 +366,6 @@ fn propose_bounty_works() { #[test] fn propose_bounty_validation_works() { new_test_ext().execute_with(|| { - System::set_block_number(1); - Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); @@ -388,7 +393,6 @@ fn propose_bounty_validation_works() { #[test] fn close_bounty_works() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_noop!(Bounties::close_bounty(RuntimeOrigin::root(), 0), Error::::InvalidIndex); @@ -413,7 +417,6 @@ fn close_bounty_works() { #[test] fn approve_bounty_works() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_noop!( Bounties::approve_bounty(RuntimeOrigin::root(), 0), @@ -448,7 +451,7 @@ fn approve_bounty_works() { assert_eq!(Balances::reserved_balance(0), deposit); assert_eq!(Balances::free_balance(0), 100 - deposit); - >::on_initialize(2); + go_to_block(2); // return deposit assert_eq!(Balances::reserved_balance(0), 0); @@ -474,7 +477,6 @@ fn approve_bounty_works() { #[test] fn assign_curator_works() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_noop!( @@ -486,8 +488,7 @@ fn assign_curator_works() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_noop!( Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 50), @@ -544,14 +545,12 @@ fn assign_curator_works() { #[test] fn unassign_curator_works() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let fee = 4; @@ -597,15 +596,13 @@ fn unassign_curator_works() { #[test] fn award_and_claim_bounty_works() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&4, 10); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let fee = 4; assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, fee)); @@ -635,8 +632,7 @@ fn award_and_claim_bounty_works() { assert_noop!(Bounties::claim_bounty(RuntimeOrigin::signed(1), 0), Error::::Premature); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); assert_ok!(Balances::transfer_allow_death( RuntimeOrigin::signed(0), @@ -664,23 +660,20 @@ fn award_and_claim_bounty_works() { #[test] fn claim_handles_high_fee() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&4, 30); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 49)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); assert_ok!(Bounties::award_bounty(RuntimeOrigin::signed(4), 0, 3)); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); // make fee > balance let res = Balances::slash(&Bounties::bounty_account_id(0), 10); @@ -705,16 +698,13 @@ fn claim_handles_high_fee() { #[test] fn cancel_and_refund() { new_test_ext().execute_with(|| { - System::set_block_number(1); - Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Balances::transfer_allow_death( RuntimeOrigin::signed(0), @@ -748,14 +738,12 @@ fn cancel_and_refund() { #[test] fn award_and_cancel() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 0, 10)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(0), 0)); @@ -791,14 +779,12 @@ fn award_and_cancel() { #[test] fn expire_and_unassign() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 1, 10)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(1), 0)); @@ -806,16 +792,14 @@ fn expire_and_unassign() { assert_eq!(Balances::free_balance(1), 93); assert_eq!(Balances::reserved_balance(1), 5); - System::set_block_number(22); - >::on_initialize(22); + go_to_block(22); assert_noop!( Bounties::unassign_curator(RuntimeOrigin::signed(0), 0), Error::::Premature ); - System::set_block_number(23); - >::on_initialize(23); + go_to_block(23); assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(0), 0)); @@ -839,7 +823,6 @@ fn expire_and_unassign() { #[test] fn extend_expiry() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&4, 10); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); @@ -851,8 +834,7 @@ fn extend_expiry() { Error::::UnexpectedStatus ); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 10)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -860,8 +842,7 @@ fn extend_expiry() { assert_eq!(Balances::free_balance(4), 5); assert_eq!(Balances::reserved_balance(4), 5); - System::set_block_number(10); - >::on_initialize(10); + go_to_block(10); assert_noop!( Bounties::extend_bounty_expiry(RuntimeOrigin::signed(0), 0, Vec::new()), @@ -895,8 +876,7 @@ fn extend_expiry() { } ); - System::set_block_number(25); - >::on_initialize(25); + go_to_block(25); assert_noop!( Bounties::unassign_curator(RuntimeOrigin::signed(0), 0), @@ -975,13 +955,11 @@ fn genesis_funding_works() { #[test] fn unassign_curator_self() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 1, 10)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(1), 0)); @@ -989,8 +967,7 @@ fn unassign_curator_self() { assert_eq!(Balances::free_balance(1), 93); assert_eq!(Balances::reserved_balance(1), 5); - System::set_block_number(8); - >::on_initialize(8); + go_to_block(8); assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(1), 0)); @@ -1022,7 +999,6 @@ fn accept_curator_handles_different_deposit_calculations() { let value = 88; let fee = 42; - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&user, 100); // Allow for a larger spend limit: @@ -1030,8 +1006,7 @@ fn accept_curator_handles_different_deposit_calculations() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), value, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), bounty_index)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), bounty_index, user, fee)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(user), bounty_index)); @@ -1052,8 +1027,7 @@ fn accept_curator_handles_different_deposit_calculations() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), value, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), bounty_index)); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), bounty_index, user, fee)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(user), bounty_index)); @@ -1078,8 +1052,7 @@ fn accept_curator_handles_different_deposit_calculations() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), value, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), bounty_index)); - System::set_block_number(6); - >::on_initialize(6); + go_to_block(6); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), bounty_index, user, fee)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(user), bounty_index)); @@ -1096,7 +1069,6 @@ fn approve_bounty_works_second_instance() { // Set burn to 0 to make tracking funds easier. Burn::set(Permill::from_percent(0)); - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&Treasury1::account_id(), 201); assert_eq!(Balances::free_balance(&Treasury::account_id()), 101); @@ -1104,7 +1076,7 @@ fn approve_bounty_works_second_instance() { assert_ok!(Bounties1::propose_bounty(RuntimeOrigin::signed(0), 10, b"12345".to_vec())); assert_ok!(Bounties1::approve_bounty(RuntimeOrigin::root(), 0)); - >::on_initialize(2); + go_to_block(2); >::on_initialize(2); // Bounties 1 is funded... but from where? @@ -1119,8 +1091,6 @@ fn approve_bounty_works_second_instance() { #[test] fn approve_bounty_insufficient_spend_limit_errors() { new_test_ext().execute_with(|| { - System::set_block_number(1); - Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); @@ -1137,8 +1107,6 @@ fn approve_bounty_insufficient_spend_limit_errors() { #[test] fn approve_bounty_instance1_insufficient_spend_limit_errors() { new_test_ext().execute_with(|| { - System::set_block_number(1); - Balances::make_free_balance_be(&Treasury1::account_id(), 101); assert_eq!(Treasury1::pot(), 100); @@ -1155,7 +1123,6 @@ fn approve_bounty_instance1_insufficient_spend_limit_errors() { #[test] fn propose_curator_insufficient_spend_limit_errors() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); // Temporarily set a larger spend limit; @@ -1163,8 +1130,7 @@ fn propose_curator_insufficient_spend_limit_errors() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 51, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); SpendLimit::set(50); // 51 will not work since the limit is 50. @@ -1178,7 +1144,6 @@ fn propose_curator_insufficient_spend_limit_errors() { #[test] fn propose_curator_instance1_insufficient_spend_limit_errors() { new_test_ext().execute_with(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); // Temporarily set a larger spend limit; @@ -1186,7 +1151,6 @@ fn propose_curator_instance1_insufficient_spend_limit_errors() { assert_ok!(Bounties1::propose_bounty(RuntimeOrigin::signed(0), 11, b"12345".to_vec())); assert_ok!(Bounties1::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); >::on_initialize(2); SpendLimit1::set(10); diff --git a/substrate/frame/child-bounties/src/benchmarking.rs b/substrate/frame/child-bounties/src/benchmarking.rs index 947cfcfaa96a..2ebe64d95953 100644 --- a/substrate/frame/child-bounties/src/benchmarking.rs +++ b/substrate/frame/child-bounties/src/benchmarking.rs @@ -23,6 +23,7 @@ use super::*; use frame_benchmarking::v1::{account, benchmarks, whitelisted_caller, BenchmarkError}; use frame_system::{pallet_prelude::BlockNumberFor, RawOrigin}; +use sp_runtime::traits::BlockNumberProvider; use crate::Pallet as ChildBounties; use pallet_bounties::Pallet as Bounties; @@ -54,6 +55,10 @@ struct BenchmarkChildBounty { reason: Vec, } +fn set_block_number(n: BlockNumberFor) { + ::BlockNumberProvider::set_block_number(n); +} + fn setup_bounty( user: u32, description: u32, @@ -114,7 +119,8 @@ fn activate_bounty( let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin, child_bounty_setup.bounty_id)?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + set_block_number::(T::SpendPeriod::get()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); Bounties::::propose_curator( RawOrigin::Root.into(), child_bounty_setup.bounty_id, @@ -229,8 +235,8 @@ benchmarks! { unassign_curator { setup_pot_account::(); let bounty_setup = activate_child_bounty::(0, T::MaximumReasonLength::get())?; - Treasury::::on_initialize(BlockNumberFor::::zero()); - frame_system::Pallet::::set_block_number(T::BountyUpdatePeriod::get() + 1u32.into()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); + set_block_number::(T::SpendPeriod::get() + T::BountyUpdatePeriod::get() + 1u32.into()); let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), bounty_setup.bounty_id, bounty_setup.child_bounty_id) @@ -266,7 +272,7 @@ benchmarks! { let beneficiary_account: T::AccountId = account("beneficiary", 0, SEED); let beneficiary = T::Lookup::unlookup(beneficiary_account.clone()); - frame_system::Pallet::::set_block_number(T::BountyDepositPayoutDelay::get()); + set_block_number::(T::SpendPeriod::get() + T::BountyDepositPayoutDelay::get()); ensure!(T::Currency::free_balance(&beneficiary_account).is_zero(), "Beneficiary already has balance."); @@ -303,7 +309,7 @@ benchmarks! { close_child_bounty_active { setup_pot_account::(); let bounty_setup = activate_child_bounty::(0, T::MaximumReasonLength::get())?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); }: close_child_bounty(RawOrigin::Root, bounty_setup.bounty_id, bounty_setup.child_bounty_id) verify { assert_last_event::(Event::Canceled { diff --git a/substrate/frame/child-bounties/src/lib.rs b/substrate/frame/child-bounties/src/lib.rs index 04a1f9799cb8..e36e15b36458 100644 --- a/substrate/frame/child-bounties/src/lib.rs +++ b/substrate/frame/child-bounties/src/lib.rs @@ -65,7 +65,10 @@ use frame_support::traits::{ }; use sp_runtime::{ - traits::{AccountIdConversion, BadOrigin, CheckedSub, Saturating, StaticLookup, Zero}, + traits::{ + AccountIdConversion, BadOrigin, BlockNumberProvider, CheckedSub, Saturating, StaticLookup, + Zero, + }, DispatchResult, RuntimeDebug, }; @@ -520,7 +523,7 @@ pub mod pallet { let (parent_curator, update_due) = Self::ensure_bounty_active(parent_bounty_id)?; if sender == parent_curator || - update_due < frame_system::Pallet::::block_number() + update_due < Self::treasury_block_number() { // Slash the child-bounty curator if // + the call is made by the parent bounty curator. @@ -599,7 +602,7 @@ pub mod pallet { child_bounty.status = ChildBountyStatus::PendingPayout { curator: signer, beneficiary: beneficiary.clone(), - unlock_at: frame_system::Pallet::::block_number() + + unlock_at: Self::treasury_block_number() + T::BountyDepositPayoutDelay::get(), }; Ok(()) @@ -661,7 +664,7 @@ pub mod pallet { // Ensure block number is elapsed for processing the // claim. ensure!( - frame_system::Pallet::::block_number() >= *unlock_at, + Self::treasury_block_number() >= *unlock_at, BountiesError::::Premature, ); @@ -769,6 +772,13 @@ pub mod pallet { } impl Pallet { + /// Get the block number used in the treasury pallet. + /// + /// It may be configured to use the relay chain block number on a parachain. + pub fn treasury_block_number() -> BlockNumberFor { + ::BlockNumberProvider::current_block_number() + } + // This function will calculate the deposit of a curator. fn calculate_curator_deposit( parent_curator: &T::AccountId, diff --git a/substrate/frame/child-bounties/src/tests.rs b/substrate/frame/child-bounties/src/tests.rs index 125844fa70e2..96d01b03560d 100644 --- a/substrate/frame/child-bounties/src/tests.rs +++ b/substrate/frame/child-bounties/src/tests.rs @@ -42,6 +42,12 @@ use super::Event as ChildBountiesEvent; type Block = frame_system::mocking::MockBlock; type BountiesError = pallet_bounties::Error; +// This function directly jumps to a block number, and calls `on_initialize`. +fn go_to_block(n: u64) { + ::BlockNumberProvider::set_block_number(n); + >::on_initialize(n); +} + frame_support::construct_runtime!( pub enum Test { @@ -98,6 +104,7 @@ impl pallet_treasury::Config for Test { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<10>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } @@ -184,15 +191,14 @@ fn add_child_bounty() { // Curator, child-bounty curator & beneficiary. // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let fee = 8; assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, fee)); @@ -278,7 +284,7 @@ fn child_bounty_assign_curator() { // 3, Test for DB state of `ChildBounties`. // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&4, 101); Balances::make_free_balance_be(&8, 101); @@ -287,8 +293,7 @@ fn child_bounty_assign_curator() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let fee = 4; assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, fee)); @@ -383,7 +388,7 @@ fn child_bounty_assign_curator() { fn award_claim_child_bounty() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -396,8 +401,7 @@ fn award_claim_child_bounty() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -449,7 +453,7 @@ fn award_claim_child_bounty() { BountiesError::Premature ); - System::set_block_number(9); + go_to_block(9); assert_ok!(ChildBounties::claim_child_bounty(RuntimeOrigin::signed(7), 0, 0)); @@ -474,7 +478,7 @@ fn award_claim_child_bounty() { fn close_child_bounty_added() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -487,8 +491,7 @@ fn close_child_bounty_added() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); @@ -504,7 +507,7 @@ fn close_child_bounty_added() { assert_eq!(last_event(), ChildBountiesEvent::Added { index: 0, child_index: 0 }); - System::set_block_number(4); + go_to_block(4); // Close child-bounty. // Wrong origin. @@ -531,7 +534,7 @@ fn close_child_bounty_added() { fn close_child_bounty_active() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -544,8 +547,7 @@ fn close_child_bounty_active() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); @@ -589,7 +591,7 @@ fn close_child_bounty_active() { fn close_child_bounty_pending() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -602,8 +604,7 @@ fn close_child_bounty_pending() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let parent_fee = 6; assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, parent_fee)); @@ -650,7 +651,7 @@ fn close_child_bounty_pending() { fn child_bounty_added_unassign_curator() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -663,8 +664,7 @@ fn child_bounty_added_unassign_curator() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); @@ -692,7 +692,7 @@ fn child_bounty_added_unassign_curator() { fn child_bounty_curator_proposed_unassign_curator() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -705,8 +705,7 @@ fn child_bounty_curator_proposed_unassign_curator() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); @@ -767,7 +766,7 @@ fn child_bounty_active_unassign_curator() { // bounty. Unassign from random account. Should slash. new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -782,8 +781,7 @@ fn child_bounty_active_unassign_curator() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -797,8 +795,7 @@ fn child_bounty_active_unassign_curator() { )); assert_eq!(last_event(), ChildBountiesEvent::Added { index: 0, child_index: 0 }); - System::set_block_number(3); - >::on_initialize(3); + go_to_block(3); // Propose and accept curator for child-bounty. let fee = 6; @@ -817,8 +814,7 @@ fn child_bounty_active_unassign_curator() { } ); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); // Unassign curator - from reject origin. assert_ok!(ChildBounties::unassign_curator(RuntimeOrigin::root(), 0, 0)); @@ -856,8 +852,7 @@ fn child_bounty_active_unassign_curator() { } ); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); // Unassign curator again - from parent curator. assert_ok!(ChildBounties::unassign_curator(RuntimeOrigin::signed(4), 0, 0)); @@ -893,8 +888,7 @@ fn child_bounty_active_unassign_curator() { } ); - System::set_block_number(6); - >::on_initialize(6); + go_to_block(6); // Unassign curator again - from child-bounty curator. assert_ok!(ChildBounties::unassign_curator(RuntimeOrigin::signed(6), 0, 0)); @@ -932,8 +926,7 @@ fn child_bounty_active_unassign_curator() { } ); - System::set_block_number(7); - >::on_initialize(7); + go_to_block(7); // Unassign curator again - from non curator; non reject origin; some random guy. // Bounty update period is not yet complete. @@ -942,8 +935,7 @@ fn child_bounty_active_unassign_curator() { BountiesError::Premature ); - System::set_block_number(20); - >::on_initialize(20); + go_to_block(20); // Unassign child curator from random account after inactivity. assert_ok!(ChildBounties::unassign_curator(RuntimeOrigin::signed(3), 0, 0)); @@ -972,7 +964,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { // This can happen when the curator of parent bounty has been unassigned. new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -987,8 +979,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -1002,8 +993,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { )); assert_eq!(last_event(), ChildBountiesEvent::Added { index: 0, child_index: 0 }); - System::set_block_number(3); - >::on_initialize(3); + go_to_block(3); // Propose and accept curator for child-bounty. let fee = 8; @@ -1022,14 +1012,12 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { } ); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); // Unassign parent bounty curator. assert_ok!(Bounties::unassign_curator(RuntimeOrigin::root(), 0)); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); // Try unassign child-bounty curator - from non curator; non reject // origin; some random guy. Bounty update period is not yet complete. @@ -1057,15 +1045,13 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { assert_eq!(Balances::free_balance(8), 101 - expected_child_deposit); assert_eq!(Balances::reserved_balance(8), 0); // slashed - System::set_block_number(6); - >::on_initialize(6); + go_to_block(6); // Propose and accept curator for parent-bounty again. assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 5, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(5), 0)); - System::set_block_number(7); - >::on_initialize(7); + go_to_block(7); // Propose and accept curator for child-bounty again. let fee = 2; @@ -1084,8 +1070,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { } ); - System::set_block_number(8); - >::on_initialize(8); + go_to_block(8); assert_noop!( ChildBounties::unassign_curator(RuntimeOrigin::signed(3), 0, 0), @@ -1095,8 +1080,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { // Unassign parent bounty curator again. assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(5), 0)); - System::set_block_number(9); - >::on_initialize(9); + go_to_block(9); // Unassign curator again - from parent curator. assert_ok!(ChildBounties::unassign_curator(RuntimeOrigin::signed(7), 0, 0)); @@ -1123,7 +1107,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { fn close_parent_with_child_bounty() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -1142,8 +1126,7 @@ fn close_parent_with_child_bounty() { Error::::ParentBountyNotActive ); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -1157,8 +1140,7 @@ fn close_parent_with_child_bounty() { )); assert_eq!(last_event(), ChildBountiesEvent::Added { index: 0, child_index: 0 }); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); // Try close parent-bounty. // Child bounty active, can't close parent. @@ -1167,8 +1149,6 @@ fn close_parent_with_child_bounty() { BountiesError::HasActiveChildBounty ); - System::set_block_number(2); - // Close child-bounty. assert_ok!(ChildBounties::close_child_bounty(RuntimeOrigin::root(), 0, 0)); @@ -1187,7 +1167,7 @@ fn children_curator_fee_calculation_test() { // from parent bounty fee when claiming bounties. new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -1199,8 +1179,7 @@ fn children_curator_fee_calculation_test() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -1214,8 +1193,7 @@ fn children_curator_fee_calculation_test() { )); assert_eq!(last_event(), ChildBountiesEvent::Added { index: 0, child_index: 0 }); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); let fee = 6; @@ -1245,7 +1223,7 @@ fn children_curator_fee_calculation_test() { } ); - System::set_block_number(9); + go_to_block(9); // Claim child-bounty. assert_ok!(ChildBounties::claim_child_bounty(RuntimeOrigin::signed(7), 0, 0)); @@ -1256,7 +1234,7 @@ fn children_curator_fee_calculation_test() { // Award the parent bounty. assert_ok!(Bounties::award_bounty(RuntimeOrigin::signed(4), 0, 9)); - System::set_block_number(15); + go_to_block(15); // Claim the parent bounty. assert_ok!(Bounties::claim_bounty(RuntimeOrigin::signed(9), 0)); @@ -1282,7 +1260,7 @@ fn accept_curator_handles_different_deposit_calculations() { let parent_value = 1_000_000; let parent_fee = 10_000; - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), parent_value * 3); Balances::make_free_balance_be(&parent_curator, parent_fee * 100); assert_ok!(Bounties::propose_bounty( @@ -1292,8 +1270,7 @@ fn accept_curator_handles_different_deposit_calculations() { )); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), parent_index)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator( RuntimeOrigin::root(), @@ -1319,8 +1296,7 @@ fn accept_curator_handles_different_deposit_calculations() { child_value, b"12345-p1".to_vec() )); - System::set_block_number(3); - >::on_initialize(3); + go_to_block(3); assert_ok!(ChildBounties::propose_curator( RuntimeOrigin::signed(parent_curator), parent_index, @@ -1354,8 +1330,7 @@ fn accept_curator_handles_different_deposit_calculations() { child_value, b"12345-p1".to_vec() )); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); assert_ok!(ChildBounties::propose_curator( RuntimeOrigin::signed(parent_curator), parent_index, @@ -1387,8 +1362,7 @@ fn accept_curator_handles_different_deposit_calculations() { child_value, b"12345-p1".to_vec() )); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); assert_ok!(ChildBounties::propose_curator( RuntimeOrigin::signed(parent_curator), parent_index, @@ -1423,8 +1397,7 @@ fn accept_curator_handles_different_deposit_calculations() { child_value, b"12345-p1".to_vec() )); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); assert_ok!(ChildBounties::propose_curator( RuntimeOrigin::signed(parent_curator), parent_index, diff --git a/substrate/frame/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs index 7e4a9368ad0c..f6f130b7e261 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -119,6 +119,7 @@ impl pallet_treasury::Config for Test { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<10>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } @@ -141,6 +142,7 @@ impl pallet_treasury::Config for Test { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<10>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index b437e5ef137e..c6bdc95980ca 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -85,8 +85,11 @@ use codec::{Decode, Encode, MaxEncodedLen}; use scale_info::TypeInfo; use sp_runtime::{ - traits::{AccountIdConversion, CheckedAdd, Saturating, StaticLookup, Zero}, - Permill, RuntimeDebug, + traits::{ + AccountIdConversion, BlockNumberProvider, CheckedAdd, One, Saturating, StaticLookup, + UniqueSaturatedInto, Zero, + }, + PerThing, Permill, RuntimeDebug, }; use sp_std::{collections::btree_map::BTreeMap, prelude::*}; @@ -100,6 +103,7 @@ use frame_support::{ weights::Weight, PalletId, }; +use frame_system::pallet_prelude::BlockNumberFor; pub use pallet::*; pub use weights::WeightInfo; @@ -272,6 +276,9 @@ pub mod pallet { /// Helper type for benchmarks. #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper: ArgumentsFactory; + + /// Provider for the block number. Normally this is the `frame_system` pallet. + type BlockNumberProvider: BlockNumberProvider>; } /// Number of proposals that have been made. @@ -322,6 +329,10 @@ pub mod pallet { OptionQuery, >; + /// The blocknumber for the last triggered spend period. + #[pallet::storage] + pub(crate) type LastSpendPeriod = StorageValue<_, BlockNumberFor, OptionQuery>; + #[pallet::genesis_config] #[derive(frame_support::DefaultNoBound)] pub struct GenesisConfig, I: 'static = ()> { @@ -414,7 +425,8 @@ pub mod pallet { impl, I: 'static> Hooks> for Pallet { /// ## Complexity /// - `O(A)` where `A` is the number of approvals - fn on_initialize(n: frame_system::pallet_prelude::BlockNumberFor) -> Weight { + fn on_initialize(_do_not_use_local_block_number: BlockNumberFor) -> Weight { + let block_number = T::BlockNumberProvider::current_block_number(); let pot = Self::pot(); let deactivated = Deactivated::::get(); if pot != deactivated { @@ -428,17 +440,29 @@ pub mod pallet { } // Check to see if we should spend some funds! - if (n % T::SpendPeriod::get()).is_zero() { - Self::spend_funds() + let last_spend_period = LastSpendPeriod::::get() + // This unwrap should only occur one time on any blockchain. + // `update_last_spend_period` will populate the `LastSpendPeriod` storage if it is + // empty. + .unwrap_or_else(|| Self::update_last_spend_period()); + let blocks_since_last_spend_period = block_number.saturating_sub(last_spend_period); + let safe_spend_period = T::SpendPeriod::get().max(BlockNumberFor::::one()); + + // Safe because of `max(1)` above. + let (spend_periods_passed, extra_blocks) = ( + blocks_since_last_spend_period / safe_spend_period, + blocks_since_last_spend_period % safe_spend_period, + ); + let new_last_spend_period = block_number.saturating_sub(extra_blocks); + if spend_periods_passed > BlockNumberFor::::zero() { + Self::spend_funds(spend_periods_passed, new_last_spend_period) } else { Weight::zero() } } #[cfg(feature = "try-runtime")] - fn try_state( - _: frame_system::pallet_prelude::BlockNumberFor, - ) -> Result<(), sp_runtime::TryRuntimeError> { + fn try_state(_: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { Self::do_try_state()?; Ok(()) } @@ -594,7 +618,7 @@ pub mod pallet { let max_amount = T::SpendOrigin::ensure_origin(origin)?; let beneficiary = T::BeneficiaryLookup::lookup(*beneficiary)?; - let now = frame_system::Pallet::::block_number(); + let now = T::BlockNumberProvider::current_block_number(); let valid_from = valid_from.unwrap_or(now); let expire_at = valid_from.saturating_add(T::PayoutPeriod::get()); ensure!(expire_at > now, Error::::SpendExpired); @@ -672,7 +696,7 @@ pub mod pallet { pub fn payout(origin: OriginFor, index: SpendIndex) -> DispatchResult { ensure_signed(origin)?; let mut spend = Spends::::get(index).ok_or(Error::::InvalidIndex)?; - let now = frame_system::Pallet::::block_number(); + let now = T::BlockNumberProvider::current_block_number(); ensure!(now >= spend.valid_from, Error::::EarlyPayout); ensure!(spend.expire_at > now, Error::::SpendExpired); ensure!( @@ -718,7 +742,7 @@ pub mod pallet { ensure_signed(origin)?; let mut spend = Spends::::get(index).ok_or(Error::::InvalidIndex)?; - let now = frame_system::Pallet::::block_number(); + let now = T::BlockNumberProvider::current_block_number(); if now > spend.expire_at && !matches!(spend.status, State::Attempted { .. }) { // spend has expired and no further status update is expected. @@ -792,8 +816,31 @@ impl, I: 'static> Pallet { T::PalletId::get().into_account_truncating() } + // Backfill the `LastSpendPeriod` storage, assuming that no configuration has changed + // since introducing this code. Used specifically for a migration-less switch to populate + // `LastSpendPeriod`. + fn update_last_spend_period() -> BlockNumberFor { + let block_number = T::BlockNumberProvider::current_block_number(); + let spend_period = T::SpendPeriod::get().max(BlockNumberFor::::one()); + let time_since_last_spend = block_number % spend_period; + // If it happens that this logic runs directly on a spend period block, we need to backdate + // to the last spend period so a spend still occurs this block. + let last_spend_period = if time_since_last_spend.is_zero() { + block_number.saturating_sub(spend_period) + } else { + // Otherwise, this is the last time we had a spend period. + block_number.saturating_sub(time_since_last_spend) + }; + LastSpendPeriod::::put(last_spend_period); + last_spend_period + } + /// Spend some money! returns number of approvals before spend. - pub fn spend_funds() -> Weight { + pub fn spend_funds( + spend_periods_passed: BlockNumberFor, + new_last_spend_period: BlockNumberFor, + ) -> Weight { + LastSpendPeriod::::put(new_last_spend_period); let mut total_weight = Weight::zero(); let mut budget_remaining = Self::pot(); @@ -845,10 +892,15 @@ impl, I: 'static> Pallet { &mut missed_any, ); - if !missed_any { - // burn some proportion of the remaining budget if we run a surplus. - let burn = (T::Burn::get() * budget_remaining).min(budget_remaining); - budget_remaining -= burn; + if !missed_any && !T::Burn::get().is_zero() { + // Get the amount of treasury that should be left after potentially multiple spend + // periods have passed. + let one_minus_burn = T::Burn::get().left_from_one(); + let percent_left = + one_minus_burn.saturating_pow(spend_periods_passed.unique_saturated_into()); + let new_budget_remaining = percent_left * budget_remaining; + let burn = budget_remaining.saturating_sub(new_budget_remaining); + budget_remaining = new_budget_remaining; let (debit, credit) = T::Currency::pair(burn); imbalance.subsume(debit); diff --git a/substrate/frame/treasury/src/tests.rs b/substrate/frame/treasury/src/tests.rs index 97b735928192..100fccef09cb 100644 --- a/substrate/frame/treasury/src/tests.rs +++ b/substrate/frame/treasury/src/tests.rs @@ -94,6 +94,12 @@ fn set_status(id: u64, s: PaymentStatus) { STATUS.with(|m| m.borrow_mut().insert(id, s)); } +// This function directly jumps to a block number, and calls `on_initialize`. +fn go_to_block(n: u64) { + ::BlockNumberProvider::set_block_number(n); + >::on_initialize(n); +} + pub struct TestPay; impl Pay for TestPay { type Beneficiary = u128; @@ -179,6 +185,7 @@ impl Config for Test { type Paymaster = TestPay; type BalanceConverter = MulBy>; type PayoutPeriod = SpendPayoutPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } @@ -251,7 +258,7 @@ fn spend_local_origin_permissioning_works() { fn spend_local_origin_works() { ExtBuilder::default().build().execute_with(|| { // Check that accumulate works when we have Some value in Dummy already. - Balances::make_free_balance_be(&Treasury::account_id(), 101); + Balances::make_free_balance_be(&Treasury::account_id(), 102); // approve spend of some amount to beneficiary `6`. assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); @@ -261,12 +268,12 @@ fn spend_local_origin_works() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(12), 20, 6)); assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(13), 50, 6)); // free balance of `6` is zero, spend period has not passed. - >::on_initialize(1); + go_to_block(1); assert_eq!(Balances::free_balance(6), 0); // free balance of `6` is `100`, spend period has passed. - >::on_initialize(2); + go_to_block(2); assert_eq!(Balances::free_balance(6), 100); - // `100` spent, `1` burned. + // `100` spent, `1` burned, `1` in ED. assert_eq!(Treasury::pot(), 0); }); } @@ -287,7 +294,7 @@ fn accepted_spend_proposal_ignored_outside_spend_period() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 100, 3)); - >::on_initialize(1); + go_to_block(1); assert_eq!(Balances::free_balance(3), 0); assert_eq!(Treasury::pot(), 100); }); @@ -300,7 +307,7 @@ fn unused_pot_should_diminish() { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::total_issuance(), init_total_issuance + 100); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 50); assert_eq!(Balances::total_issuance(), init_total_issuance + 50); }); @@ -314,7 +321,7 @@ fn accepted_spend_proposal_enacted_on_spend_period() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 100, 3)); - >::on_initialize(2); + go_to_block(2); assert_eq!(Balances::free_balance(3), 100); assert_eq!(Treasury::pot(), 0); }); @@ -328,11 +335,11 @@ fn pot_underflow_should_not_diminish() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 150, 3)); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed let _ = Balances::deposit_into_existing(&Treasury::account_id(), 100).unwrap(); - >::on_initialize(4); + go_to_block(4); assert_eq!(Balances::free_balance(3), 150); // Fund has been spent assert_eq!(Treasury::pot(), 25); // Pot has finally changed }); @@ -349,12 +356,12 @@ fn treasury_account_doesnt_get_deleted() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), treasury_balance, 3)); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), Treasury::pot(), 3)); - >::on_initialize(4); + go_to_block(4); assert_eq!(Treasury::pot(), 0); // Pot is emptied assert_eq!(Balances::free_balance(Treasury::account_id()), 1); // but the account is still there }); @@ -378,7 +385,8 @@ fn inexistent_account_works() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 99, 3)); assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 1, 3)); - >::on_initialize(2); + go_to_block(2); + assert_eq!(Treasury::pot(), 0); // Pot hasn't changed assert_eq!(Balances::free_balance(3), 0); // Balance of `3` hasn't changed @@ -386,7 +394,7 @@ fn inexistent_account_works() { assert_eq!(Treasury::pot(), 99); // Pot now contains funds assert_eq!(Balances::free_balance(Treasury::account_id()), 100); // Account does exist - >::on_initialize(4); + go_to_block(4); assert_eq!(Treasury::pot(), 0); // Pot has changed assert_eq!(Balances::free_balance(3), 99); // Balance of `3` has changed @@ -919,3 +927,35 @@ fn try_state_spends_invariant_3_works() { ); }); } + +#[test] +fn multiple_spend_periods_work() { + ExtBuilder::default().build().execute_with(|| { + // Check that accumulate works when we have Some value in Dummy already. + // 100 will be spent, 1024 will be the burn amount, 1 for ED + Balances::make_free_balance_be(&Treasury::account_id(), 100 + 1024 + 1); + // approve spend of total amount 100 to beneficiary `6`. + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(11), 10, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(12), 20, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(13), 50, 6)); + // free balance of `6` is zero, spend period has not passed. + go_to_block(1); + assert_eq!(Balances::free_balance(6), 0); + // free balance of `6` is `100`, spend period has passed. + go_to_block(2); + assert_eq!(Balances::free_balance(6), 100); + // `100` spent, 50% burned + assert_eq!(Treasury::pot(), 512); + + // 3 more spends periods pass at once, and an extra block. + go_to_block(2 + (3 * 2) + 1); + // Pot should be reduced by 50% 3 times, so 1/8th the amount. + assert_eq!(Treasury::pot(), 64); + // Even though we are on block 9, the last spend period was block 8. + assert_eq!(LastSpendPeriod::::get(), Some(8)); + }); +}