From d7678db9ff42363e63f2a39f429ece20d5c4024e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 3 Apr 2024 11:16:34 -0400 Subject: [PATCH 01/20] add block number provider --- substrate/frame/treasury/src/lib.rs | 13 +++++++++---- substrate/frame/treasury/src/tests.rs | 1 + 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index d569ae406ea3..820f5391e8e8 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -83,7 +83,9 @@ use codec::{Decode, Encode, MaxEncodedLen}; use scale_info::TypeInfo; use sp_runtime::{ - traits::{AccountIdConversion, CheckedAdd, Saturating, StaticLookup, Zero}, + traits::{ + AccountIdConversion, BlockNumberProvider, CheckedAdd, Saturating, StaticLookup, Zero, + }, Permill, RuntimeDebug, }; use sp_std::{collections::btree_map::BTreeMap, prelude::*}; @@ -289,6 +291,9 @@ pub mod pallet { /// Helper type for benchmarks. #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper: ArgumentsFactory; + + /// Provider for the block number. + type BlockNumberProvider: BlockNumberProvider>; } /// Number of proposals that have been made. @@ -734,7 +739,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); @@ -812,7 +817,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!( @@ -858,7 +863,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. diff --git a/substrate/frame/treasury/src/tests.rs b/substrate/frame/treasury/src/tests.rs index 67d81cb5c302..24412de1fed5 100644 --- a/substrate/frame/treasury/src/tests.rs +++ b/substrate/frame/treasury/src/tests.rs @@ -194,6 +194,7 @@ impl Config for Test { type Paymaster = TestPay; type BalanceConverter = MulBy>; type PayoutPeriod = SpendPayoutPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } From a8b649d413ec9ac7ec9bb97f034ca326db82f054 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 3 Apr 2024 12:03:00 -0400 Subject: [PATCH 02/20] logic for handling multiple spend periods passed --- substrate/frame/treasury/src/lib.rs | 51 +++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index 820f5391e8e8..ed7be9e6b6de 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -84,9 +84,10 @@ use scale_info::TypeInfo; use sp_runtime::{ traits::{ - AccountIdConversion, BlockNumberProvider, CheckedAdd, Saturating, StaticLookup, Zero, + AccountIdConversion, BlockNumberProvider, CheckedAdd, One, Saturating, StaticLookup, + UniqueSaturatedInto, Zero, }, - Permill, RuntimeDebug, + PerThing, Permill, RuntimeDebug, }; use sp_std::{collections::btree_map::BTreeMap, prelude::*}; @@ -100,6 +101,7 @@ use frame_support::{ weights::Weight, PalletId, }; +use frame_system::pallet_prelude::BlockNumberFor; pub use pallet::*; pub use weights::WeightInfo; @@ -344,6 +346,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 = ()> { @@ -442,7 +448,7 @@ 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(n: BlockNumberFor) -> Weight { let pot = Self::pot(); let deactivated = Deactivated::::get(); if pot != deactivated { @@ -456,17 +462,27 @@ 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 = + // AUDIT REVIEW TODO! Needs to handle migration story for this better. + LastSpendPeriod::::get().unwrap_or(BlockNumberFor::::zero()); + let blocks_since_last_spend_period = n.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 = n.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(()) } @@ -947,7 +963,11 @@ impl, I: 'static> Pallet { } /// 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(); @@ -999,10 +1019,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); From 757c7f482d16983ed6d02763f62e149fa6f63096 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 3 Apr 2024 12:18:14 -0400 Subject: [PATCH 03/20] update for new associated type --- polkadot/runtime/common/src/impls.rs | 1 + polkadot/runtime/rococo/src/lib.rs | 1 + polkadot/runtime/westend/src/lib.rs | 1 + substrate/bin/node/runtime/src/lib.rs | 1 + substrate/frame/bounties/src/tests.rs | 2 ++ substrate/frame/child-bounties/src/tests.rs | 1 + substrate/frame/tips/src/tests.rs | 2 ++ 7 files changed, 9 insertions(+) diff --git a/polkadot/runtime/common/src/impls.rs b/polkadot/runtime/common/src/impls.rs index acf5a701a62d..1a4339d8c880 100644 --- a/polkadot/runtime/common/src/impls.rs +++ b/polkadot/runtime/common/src/impls.rs @@ -336,6 +336,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 c41ffdbe72db..6135ca61b16b 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -505,6 +505,7 @@ impl pallet_treasury::Config for Runtime { >; type BalanceConverter = AssetRate; type PayoutPeriod = PayoutSpendPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = runtime_common::impls::benchmarks::TreasuryArguments; } diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index e6381513170f..226a1f42bceb 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -763,6 +763,7 @@ impl pallet_treasury::Config for Runtime { >; type BalanceConverter = AssetRate; type PayoutPeriod = PayoutSpendPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = runtime_common::impls::benchmarks::TreasuryArguments; } diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index a9606ac0bb75..b6b16d61c02e 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -1243,6 +1243,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/tests.rs b/substrate/frame/bounties/src/tests.rs index de747db53749..5c9f19843343 100644 --- a/substrate/frame/bounties/src/tests.rs +++ b/substrate/frame/bounties/src/tests.rs @@ -115,6 +115,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 = (); } @@ -142,6 +143,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/child-bounties/src/tests.rs b/substrate/frame/child-bounties/src/tests.rs index 30601f821e43..ca8a87e7a0ed 100644 --- a/substrate/frame/child-bounties/src/tests.rs +++ b/substrate/frame/child-bounties/src/tests.rs @@ -115,6 +115,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/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs index 78df3736815a..57ea12818679 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -136,6 +136,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 = (); } @@ -163,6 +164,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 = (); } From 0f74c0cdb315f0de8e50664821839e14a58fffef Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 3 Apr 2024 12:30:03 -0400 Subject: [PATCH 04/20] add and fix tests --- substrate/frame/treasury/src/tests.rs | 36 +++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/substrate/frame/treasury/src/tests.rs b/substrate/frame/treasury/src/tests.rs index 24412de1fed5..2f068737ed08 100644 --- a/substrate/frame/treasury/src/tests.rs +++ b/substrate/frame/treasury/src/tests.rs @@ -267,7 +267,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)); @@ -282,7 +282,7 @@ fn spend_local_origin_works() { // free balance of `6` is `100`, spend period has passed. >::on_initialize(2); assert_eq!(Balances::free_balance(6), 100); - // `100` spent, `1` burned. + // `100` spent, `1` burned, `1` in ED. assert_eq!(Treasury::pot(), 0); }); } @@ -1142,3 +1142,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. + >::on_initialize(1); + assert_eq!(Balances::free_balance(6), 0); + // free balance of `6` is `100`, spend period has passed. + >::on_initialize(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. + >::on_initialize(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)); + }); +} From b0cc89d5ae12187a0b1e457aee4ba47a4fe27367 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 3 Apr 2024 17:30:16 -0400 Subject: [PATCH 05/20] fix bug! --- substrate/frame/treasury/src/lib.rs | 7 +++-- substrate/frame/treasury/src/tests.rs | 36 +++++++++++++--------- substrate/primitives/runtime/src/traits.rs | 6 ++-- 3 files changed, 28 insertions(+), 21 deletions(-) diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index ed7be9e6b6de..1956fe848d86 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -448,7 +448,8 @@ pub mod pallet { impl, I: 'static> Hooks> for Pallet { /// ## Complexity /// - `O(A)` where `A` is the number of approvals - fn on_initialize(n: 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 { @@ -465,7 +466,7 @@ pub mod pallet { let last_spend_period = // AUDIT REVIEW TODO! Needs to handle migration story for this better. LastSpendPeriod::::get().unwrap_or(BlockNumberFor::::zero()); - let blocks_since_last_spend_period = n.saturating_sub(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. @@ -473,7 +474,7 @@ pub mod pallet { blocks_since_last_spend_period / safe_spend_period, blocks_since_last_spend_period % safe_spend_period, ); - let new_last_spend_period = n.saturating_sub(extra_blocks); + 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 { diff --git a/substrate/frame/treasury/src/tests.rs b/substrate/frame/treasury/src/tests.rs index 2f068737ed08..c67cbb37e4c1 100644 --- a/substrate/frame/treasury/src/tests.rs +++ b/substrate/frame/treasury/src/tests.rs @@ -104,6 +104,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; @@ -277,10 +283,10 @@ 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, `1` in ED. assert_eq!(Treasury::pot(), 0); @@ -347,7 +353,7 @@ fn accepted_spend_proposal_ignored_outside_spend_period() { Treasury::approve_proposal(RuntimeOrigin::root(), 0) }); - >::on_initialize(1); + go_to_block(1); assert_eq!(Balances::free_balance(3), 0); assert_eq!(Treasury::pot(), 100); }); @@ -360,7 +366,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); }); @@ -380,7 +386,7 @@ fn rejected_spend_proposal_ignored_on_spend_period() { Treasury::reject_proposal(RuntimeOrigin::root(), 0) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Balances::free_balance(3), 0); assert_eq!(Treasury::pot(), 50); }); @@ -473,7 +479,7 @@ fn accepted_spend_proposal_enacted_on_spend_period() { Treasury::approve_proposal(RuntimeOrigin::root(), 0) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Balances::free_balance(3), 100); assert_eq!(Treasury::pot(), 0); }); @@ -494,11 +500,11 @@ fn pot_underflow_should_not_diminish() { Treasury::approve_proposal(RuntimeOrigin::root(), 0) }); - >::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 }); @@ -522,7 +528,7 @@ fn treasury_account_doesnt_get_deleted() { Treasury::approve_proposal(RuntimeOrigin::root(), 0) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed assert_ok!({ @@ -534,7 +540,7 @@ fn treasury_account_doesnt_get_deleted() { Treasury::approve_proposal(RuntimeOrigin::root(), 1) }); - >::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 }); @@ -571,7 +577,7 @@ fn inexistent_account_works() { #[allow(deprecated)] Treasury::approve_proposal(RuntimeOrigin::root(), 1) }); - >::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 @@ -579,7 +585,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 @@ -1158,16 +1164,16 @@ fn multiple_spend_periods_work() { 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, 50% burned assert_eq!(Treasury::pot(), 512); // 3 more spends periods pass at once, and an extra block. - >::on_initialize(2 + ( 3 * 2 ) + 1); + 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. diff --git a/substrate/primitives/runtime/src/traits.rs b/substrate/primitives/runtime/src/traits.rs index 5a6c306dd2a1..48fc90877b88 100644 --- a/substrate/primitives/runtime/src/traits.rs +++ b/substrate/primitives/runtime/src/traits.rs @@ -2322,12 +2322,12 @@ pub trait BlockNumberProvider { /// . fn current_block_number() -> Self::BlockNumber; - /// Utility function only to be used in benchmarking scenarios, to be implemented optionally, - /// else a noop. + /// Utility function only to be used in benchmarking scenarios or tests, to be implemented + /// optionally, else a noop. /// /// It allows for setting the block number that will later be fetched /// This is useful in case the block number provider is different than System - #[cfg(feature = "runtime-benchmarks")] + #[cfg(any(feature = "std", feature = "runtime-benchmarks", test))] fn set_block_number(_block: Self::BlockNumber) {} } From f01f0e69e657a59cda42331070e47071800d4a9e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 3 Apr 2024 22:01:55 -0400 Subject: [PATCH 06/20] introduce `update_last_spend_period` --- substrate/frame/treasury/src/lib.rs | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index 1956fe848d86..d76e656c6379 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -463,9 +463,11 @@ pub mod pallet { } // Check to see if we should spend some funds! - let last_spend_period = - // AUDIT REVIEW TODO! Needs to handle migration story for this better. - LastSpendPeriod::::get().unwrap_or(BlockNumberFor::::zero()); + 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(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()); @@ -963,6 +965,25 @@ impl, I: 'static> Pallet { r } + // 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( spend_periods_passed: BlockNumberFor, From 904dd96e4a5d7d52f5430d2893623bad87665bab Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 3 Apr 2024 22:02:57 -0400 Subject: [PATCH 07/20] remove test feature flag --- substrate/primitives/runtime/src/traits.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/primitives/runtime/src/traits.rs b/substrate/primitives/runtime/src/traits.rs index 48fc90877b88..d023aa045dbe 100644 --- a/substrate/primitives/runtime/src/traits.rs +++ b/substrate/primitives/runtime/src/traits.rs @@ -2327,7 +2327,7 @@ pub trait BlockNumberProvider { /// /// It allows for setting the block number that will later be fetched /// This is useful in case the block number provider is different than System - #[cfg(any(feature = "std", feature = "runtime-benchmarks", test))] + #[cfg(any(feature = "std", feature = "runtime-benchmarks"))] fn set_block_number(_block: Self::BlockNumber) {} } From 1add90c1bbe4b3b32397d78f41cff6ee49771439 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 10 Apr 2024 11:21:59 -0400 Subject: [PATCH 08/20] Update substrate/frame/treasury/src/lib.rs --- substrate/frame/treasury/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index d76e656c6379..3fd9c03edf69 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -294,7 +294,7 @@ pub mod pallet { #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper: ArgumentsFactory; - /// Provider for the block number. + /// Provider for the block number. Normally this is the `frame_system` pallet. type BlockNumberProvider: BlockNumberProvider>; } From a45e5065dfd7aebcbc90243da09206eeb77dfb0c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 10 Apr 2024 13:05:30 -0400 Subject: [PATCH 09/20] Update mod.rs --- .../collectives/collectives-westend/src/fellowship/mod.rs | 1 + 1 file changed, 1 insertion(+) 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 3816d2ed848e..492709abf029 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs @@ -352,4 +352,5 @@ impl pallet_treasury::Config for Runtime { sp_core::ConstU8<1>, ConstU32<1000>, >; + type BlockNumberProvider = System; } From 64ceef4c2a43d371707feb9e0e455621146c1c80 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Apr 2024 15:14:49 -0400 Subject: [PATCH 10/20] Create pr_3970.prdoc --- prdoc/pr_3970.prdoc | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 prdoc/pr_3970.prdoc diff --git a/prdoc/pr_3970.prdoc b/prdoc/pr_3970.prdoc new file mode 100644 index 000000000000..c0286b352288 --- /dev/null +++ b/prdoc/pr_3970.prdoc @@ -0,0 +1,11 @@ +# 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. + +crates: [ ] From e3a06a89db8814f4cb930103c9eb4b493ac9aa4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 15 Apr 2024 21:34:48 +0200 Subject: [PATCH 11/20] Update substrate/frame/treasury/src/lib.rs --- substrate/frame/treasury/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index 1c9620a96811..f84f538ddcda 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -469,7 +469,7 @@ pub mod pallet { // 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(Self::update_last_spend_period()); + .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()); From d0027d158b2473dfb5b81f614ec1c717bf25a49e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Apr 2024 15:37:39 -0400 Subject: [PATCH 12/20] Update pr_3970.prdoc --- prdoc/pr_3970.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_3970.prdoc b/prdoc/pr_3970.prdoc index c0286b352288..a84bcffec24a 100644 --- a/prdoc/pr_3970.prdoc +++ b/prdoc/pr_3970.prdoc @@ -6,6 +6,6 @@ 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. + 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: [ ] From 874db2a6d3b12b9807d01a6f57bef3816761e86d Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Apr 2024 21:01:58 -0400 Subject: [PATCH 13/20] Update mod.rs --- .../collectives/collectives-westend/src/fellowship/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 492709abf029..106fa3c8c7f4 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs @@ -352,5 +352,5 @@ impl pallet_treasury::Config for Runtime { sp_core::ConstU8<1>, ConstU32<1000>, >; - type BlockNumberProvider = System; + type BlockNumberProvider = crate::System; } From e155ee61a340a115da49cf7cc30d9816ae4c3d90 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Apr 2024 21:51:24 -0400 Subject: [PATCH 14/20] fix bounties test --- substrate/frame/bounties/src/tests.rs | 70 +++++++++++++++------------ 1 file changed, 38 insertions(+), 32 deletions(-) diff --git a/substrate/frame/bounties/src/tests.rs b/substrate/frame/bounties/src/tests.rs index 5c9f19843343..746e88046551 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 { @@ -280,7 +286,7 @@ fn accepted_spend_proposal_ignored_outside_spend_period() { Treasury::approve_proposal(RuntimeOrigin::root(), 0) }); - >::on_initialize(1); + go_to_block(1); assert_eq!(Balances::free_balance(3), 0); assert_eq!(Treasury::pot(), 100); }); @@ -293,7 +299,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); }); @@ -313,7 +319,7 @@ fn rejected_spend_proposal_ignored_on_spend_period() { Treasury::reject_proposal(RuntimeOrigin::root(), 0) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Balances::free_balance(3), 0); assert_eq!(Treasury::pot(), 50); }); @@ -406,7 +412,7 @@ fn accepted_spend_proposal_enacted_on_spend_period() { Treasury::approve_proposal(RuntimeOrigin::root(), 0) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Balances::free_balance(3), 100); assert_eq!(Treasury::pot(), 0); }); @@ -427,11 +433,11 @@ fn pot_underflow_should_not_diminish() { Treasury::approve_proposal(RuntimeOrigin::root(), 0) }); - >::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 }); @@ -455,7 +461,7 @@ fn treasury_account_doesnt_get_deleted() { Treasury::approve_proposal(RuntimeOrigin::root(), 0) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed assert_ok!({ @@ -467,7 +473,7 @@ fn treasury_account_doesnt_get_deleted() { Treasury::approve_proposal(RuntimeOrigin::root(), 1) }); - >::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 }); @@ -504,7 +510,7 @@ fn inexistent_account_works() { #[allow(deprecated)] Treasury::approve_proposal(RuntimeOrigin::root(), 1) }); - >::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 @@ -512,7 +518,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 @@ -645,7 +651,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); @@ -684,7 +690,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), @@ -748,7 +754,7 @@ fn unassign_curator_works() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let fee = 4; @@ -802,7 +808,7 @@ fn award_and_claim_bounty_works() { 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)); @@ -833,7 +839,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), @@ -869,7 +875,7 @@ fn claim_handles_high_fee() { 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)); @@ -877,7 +883,7 @@ fn claim_handles_high_fee() { 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); @@ -911,7 +917,7 @@ fn cancel_and_refund() { 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), @@ -952,7 +958,7 @@ fn award_and_cancel() { 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)); @@ -995,7 +1001,7 @@ fn expire_and_unassign() { 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)); @@ -1004,7 +1010,7 @@ fn expire_and_unassign() { 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), @@ -1012,7 +1018,7 @@ fn expire_and_unassign() { ); System::set_block_number(23); - >::on_initialize(23); + go_to_block(23); assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(0), 0)); @@ -1049,7 +1055,7 @@ fn extend_expiry() { ); 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)); @@ -1058,7 +1064,7 @@ fn extend_expiry() { 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()), @@ -1093,7 +1099,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), @@ -1178,7 +1184,7 @@ fn unassign_curator_self() { 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)); @@ -1187,7 +1193,7 @@ fn unassign_curator_self() { 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)); @@ -1228,7 +1234,7 @@ fn accept_curator_handles_different_deposit_calculations() { 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)); @@ -1250,7 +1256,7 @@ fn accept_curator_handles_different_deposit_calculations() { 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)); @@ -1276,7 +1282,7 @@ fn accept_curator_handles_different_deposit_calculations() { 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)); @@ -1301,7 +1307,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? @@ -1361,7 +1367,7 @@ fn propose_curator_insufficient_spend_limit_errors() { 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. From 53a1a2e523cf252370caa14e06880330ce5c07f3 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Apr 2024 22:00:19 -0400 Subject: [PATCH 15/20] update child bounties tests for consistency --- substrate/frame/child-bounties/src/tests.rs | 138 ++++++++------------ 1 file changed, 55 insertions(+), 83 deletions(-) diff --git a/substrate/frame/child-bounties/src/tests.rs b/substrate/frame/child-bounties/src/tests.rs index ca8a87e7a0ed..9e1d08783737 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 { @@ -201,15 +207,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)); @@ -292,7 +297,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); @@ -301,8 +306,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)); @@ -397,7 +401,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); @@ -410,8 +414,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)); @@ -463,7 +466,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)); @@ -488,7 +491,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); @@ -501,8 +504,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)); @@ -518,7 +520,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. @@ -545,7 +547,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); @@ -558,8 +560,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)); @@ -603,7 +604,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); @@ -616,8 +617,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)); @@ -664,7 +664,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); @@ -677,8 +677,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)); @@ -706,7 +705,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); @@ -719,8 +718,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)); @@ -781,7 +779,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); @@ -796,8 +794,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)); @@ -811,8 +808,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; @@ -831,8 +827,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)); @@ -870,8 +865,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)); @@ -907,8 +901,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)); @@ -946,8 +939,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. @@ -956,8 +948,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)); @@ -986,7 +977,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); @@ -1001,8 +992,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)); @@ -1016,8 +1006,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; @@ -1036,14 +1025,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. @@ -1071,15 +1058,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; @@ -1098,8 +1083,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), @@ -1109,8 +1093,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)); @@ -1137,7 +1120,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); @@ -1156,8 +1139,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)); @@ -1171,8 +1153,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. @@ -1181,8 +1162,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)); @@ -1201,7 +1180,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); @@ -1213,8 +1192,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)); @@ -1228,8 +1206,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; @@ -1259,7 +1236,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)); @@ -1270,7 +1247,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)); @@ -1296,7 +1273,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( @@ -1306,8 +1283,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(), @@ -1333,8 +1309,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, @@ -1368,8 +1343,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, @@ -1401,8 +1375,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, @@ -1437,8 +1410,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, From e1f25b3aab0a7ea33297ff10c0677420d53e7a9c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 28 Apr 2024 05:26:15 -0400 Subject: [PATCH 16/20] more fixes --- substrate/frame/bounties/src/benchmarking.rs | 10 +++-- substrate/frame/bounties/src/lib.rs | 29 +++++++------ substrate/frame/bounties/src/tests.rs | 45 -------------------- 3 files changed, 23 insertions(+), 61 deletions(-) diff --git a/substrate/frame/bounties/src/benchmarking.rs b/substrate/frame/bounties/src/benchmarking.rs index 3558847c8fed..0ab531b8f18d 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 { @@ -124,7 +128,7 @@ benchmarks_instance_pallet! { let (curator_lookup, bounty_id) = create_bounty::()?; Treasury::::on_initialize(BlockNumberFor::::zero()); let bounty_id = BountyCount::::get() - 1; - frame_system::Pallet::::set_block_number(T::BountyUpdatePeriod::get() + 2u32.into()); + set_block_number::(T::BountyUpdatePeriod::get() + 2u32.into()); let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), bounty_id) @@ -163,7 +167,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::BountyDepositPayoutDelay::get() + 1u32.into()); ensure!(T::Currency::free_balance(&beneficiary_account).is_zero(), "Beneficiary already has balance"); }: _(RawOrigin::Signed(curator), bounty_id) diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs index c099fc48b7a3..d96bbedb1380 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, }; @@ -487,7 +487,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::local_block_number(); if *update_due < block_number { slash_curator(curator, &mut bounty.curator_deposit); // Continue to change bounty status below... @@ -551,8 +551,7 @@ 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::local_block_number() + T::BountyUpdatePeriod::get(); bounty.status = BountyStatus::Active { curator: curator.clone(), update_due }; @@ -606,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::local_block_number() + T::BountyDepositPayoutDelay::get(), }; Ok(()) @@ -638,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::local_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 @@ -794,9 +789,8 @@ 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() + - T::BountyUpdatePeriod::get()) - .max(*update_due); + *update_due = (Self::local_block_number() + T::BountyUpdatePeriod::get()) + .max(*update_due); }, _ => return Err(Error::::UnexpectedStatus.into()), } @@ -811,6 +805,15 @@ pub mod pallet { } impl, I: 'static> Pallet { + /// Get the block number used for this pallet. + /// + /// This comes from the Treasury pallet which may be configured to use the relay chain on a + /// parachain. + pub fn local_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 746e88046551..a0c9f8cf2a44 100644 --- a/substrate/frame/bounties/src/tests.rs +++ b/substrate/frame/bounties/src/tests.rs @@ -528,8 +528,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); @@ -562,8 +560,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); @@ -591,7 +587,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); @@ -616,7 +611,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), @@ -677,7 +671,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!( @@ -689,7 +682,6 @@ fn assign_curator_works() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); go_to_block(2); assert_noop!( @@ -747,13 +739,11 @@ 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); go_to_block(2); let fee = 4; @@ -800,14 +790,12 @@ 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); go_to_block(2); let fee = 4; @@ -838,7 +826,6 @@ fn award_and_claim_bounty_works() { assert_noop!(Bounties::claim_bounty(RuntimeOrigin::signed(1), 0), Error::::Premature); - System::set_block_number(5); go_to_block(5); assert_ok!(Balances::transfer_allow_death( @@ -867,14 +854,12 @@ 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); go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 49)); @@ -882,7 +867,6 @@ fn claim_handles_high_fee() { assert_ok!(Bounties::award_bounty(RuntimeOrigin::signed(4), 0, 3)); - System::set_block_number(5); go_to_block(5); // make fee > balance @@ -908,15 +892,12 @@ 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); go_to_block(2); assert_ok!(Balances::transfer_allow_death( @@ -951,13 +932,11 @@ 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); go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 0, 10)); @@ -994,13 +973,11 @@ 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); go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 1, 10)); @@ -1009,7 +986,6 @@ fn expire_and_unassign() { assert_eq!(Balances::free_balance(1), 93); assert_eq!(Balances::reserved_balance(1), 5); - System::set_block_number(22); go_to_block(22); assert_noop!( @@ -1017,7 +993,6 @@ fn expire_and_unassign() { Error::::Premature ); - System::set_block_number(23); go_to_block(23); assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(0), 0)); @@ -1042,7 +1017,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())); @@ -1054,7 +1028,6 @@ fn extend_expiry() { Error::::UnexpectedStatus ); - System::set_block_number(2); go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 10)); @@ -1063,7 +1036,6 @@ fn extend_expiry() { assert_eq!(Balances::free_balance(4), 5); assert_eq!(Balances::reserved_balance(4), 5); - System::set_block_number(10); go_to_block(10); assert_noop!( @@ -1098,7 +1070,6 @@ fn extend_expiry() { } ); - System::set_block_number(25); go_to_block(25); assert_noop!( @@ -1178,12 +1149,10 @@ 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); go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 1, 10)); @@ -1192,7 +1161,6 @@ fn unassign_curator_self() { assert_eq!(Balances::free_balance(1), 93); assert_eq!(Balances::reserved_balance(1), 5); - System::set_block_number(8); go_to_block(8); assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(1), 0)); @@ -1225,7 +1193,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: @@ -1233,7 +1200,6 @@ 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); go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), bounty_index, user, fee)); @@ -1255,7 +1221,6 @@ 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); go_to_block(4); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), bounty_index, user, fee)); @@ -1281,7 +1246,6 @@ 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); go_to_block(6); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), bounty_index, user, fee)); @@ -1299,7 +1263,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); @@ -1322,8 +1285,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); @@ -1340,8 +1301,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); @@ -1358,7 +1317,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; @@ -1366,7 +1324,6 @@ 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); go_to_block(2); SpendLimit::set(50); @@ -1381,7 +1338,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; @@ -1389,7 +1345,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); From cc805224b2707d28ed02bbcaf8410343e00f4ead Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 28 Apr 2024 05:46:05 -0400 Subject: [PATCH 17/20] fix `on_initialize(0)` which is invalid --- substrate/frame/bounties/src/benchmarking.rs | 23 +++++++++++--------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/substrate/frame/bounties/src/benchmarking.rs b/substrate/frame/bounties/src/benchmarking.rs index 0ab531b8f18d..543c23419830 100644 --- a/substrate/frame/bounties/src/benchmarking.rs +++ b/substrate/frame/bounties/src/benchmarking.rs @@ -81,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)) @@ -119,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; - 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) @@ -140,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)?; @@ -158,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)?; @@ -167,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)?; - 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) @@ -187,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::ApproveOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; @@ -199,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)?; From b38350002345e13ced1c54f64d9b9eb08e16e05d Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 28 Apr 2024 05:53:59 -0400 Subject: [PATCH 18/20] fix up child bounties for block number --- .../frame/child-bounties/src/benchmarking.rs | 16 +++++++++++----- substrate/frame/child-bounties/src/lib.rs | 19 +++++++++++++++---- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/substrate/frame/child-bounties/src/benchmarking.rs b/substrate/frame/child-bounties/src/benchmarking.rs index 1973564d0dc1..7139e0617ed9 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 1eedeaa5a1ae..d03bce52504b 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, }; @@ -525,7 +528,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::local_block_number() { // Slash the child-bounty curator if // + the call is made by the parent bounty curator. @@ -604,7 +607,7 @@ pub mod pallet { child_bounty.status = ChildBountyStatus::PendingPayout { curator: signer, beneficiary: beneficiary.clone(), - unlock_at: frame_system::Pallet::::block_number() + + unlock_at: Self::local_block_number() + T::BountyDepositPayoutDelay::get(), }; Ok(()) @@ -666,7 +669,7 @@ pub mod pallet { // Ensure block number is elapsed for processing the // claim. ensure!( - frame_system::Pallet::::block_number() >= *unlock_at, + Self::local_block_number() >= *unlock_at, BountiesError::::Premature, ); @@ -774,6 +777,14 @@ pub mod pallet { } impl Pallet { + /// Get the block number used for this pallet. + /// + /// This comes from the Treasury pallet which may be configured to use the relay chain on a + /// parachain. + pub fn local_block_number() -> BlockNumberFor { + ::BlockNumberProvider::current_block_number() + } + // This function will calculate the deposit of a curator. fn calculate_curator_deposit( parent_curator: &T::AccountId, From bf6b8d2fe000a0e4217e562a7bdecb6b40896844 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 28 Apr 2024 05:56:06 -0400 Subject: [PATCH 19/20] choose a more specific name --- substrate/frame/bounties/src/lib.rs | 21 +++++++++++---------- substrate/frame/child-bounties/src/lib.rs | 13 ++++++------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs index d96bbedb1380..13de25aed506 100644 --- a/substrate/frame/bounties/src/lib.rs +++ b/substrate/frame/bounties/src/lib.rs @@ -487,7 +487,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 = Self::local_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... @@ -551,7 +551,8 @@ pub mod pallet { T::Currency::reserve(curator, deposit)?; bounty.curator_deposit = deposit; - let update_due = Self::local_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,7 +606,7 @@ pub mod pallet { bounty.status = BountyStatus::PendingPayout { curator: signer, beneficiary: beneficiary.clone(), - unlock_at: Self::local_block_number() + T::BountyDepositPayoutDelay::get(), + unlock_at: Self::treasury_block_number() + T::BountyDepositPayoutDelay::get(), }; Ok(()) @@ -636,7 +637,7 @@ pub mod pallet { if let BountyStatus::PendingPayout { curator, beneficiary, unlock_at } = bounty.status { - ensure!(Self::local_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 @@ -789,8 +790,9 @@ pub mod pallet { match bounty.status { BountyStatus::Active { ref curator, ref mut update_due } => { ensure!(*curator == signer, Error::::RequireCurator); - *update_due = (Self::local_block_number() + T::BountyUpdatePeriod::get()) - .max(*update_due); + *update_due = (Self::treasury_block_number() + + T::BountyUpdatePeriod::get()) + .max(*update_due); }, _ => return Err(Error::::UnexpectedStatus.into()), } @@ -805,11 +807,10 @@ pub mod pallet { } impl, I: 'static> Pallet { - /// Get the block number used for this pallet. + /// Get the block number used in the treasury pallet. /// - /// This comes from the Treasury pallet which may be configured to use the relay chain on a - /// parachain. - pub fn local_block_number() -> BlockNumberFor { + /// It may be configured to use the relay chain block number on a parachain. + pub fn treasury_block_number() -> BlockNumberFor { >::BlockNumberProvider::current_block_number() } diff --git a/substrate/frame/child-bounties/src/lib.rs b/substrate/frame/child-bounties/src/lib.rs index d03bce52504b..2a0b2a94692e 100644 --- a/substrate/frame/child-bounties/src/lib.rs +++ b/substrate/frame/child-bounties/src/lib.rs @@ -528,7 +528,7 @@ pub mod pallet { let (parent_curator, update_due) = Self::ensure_bounty_active(parent_bounty_id)?; if sender == parent_curator || - update_due < Self::local_block_number() + update_due < Self::treasury_block_number() { // Slash the child-bounty curator if // + the call is made by the parent bounty curator. @@ -607,7 +607,7 @@ pub mod pallet { child_bounty.status = ChildBountyStatus::PendingPayout { curator: signer, beneficiary: beneficiary.clone(), - unlock_at: Self::local_block_number() + + unlock_at: Self::treasury_block_number() + T::BountyDepositPayoutDelay::get(), }; Ok(()) @@ -669,7 +669,7 @@ pub mod pallet { // Ensure block number is elapsed for processing the // claim. ensure!( - Self::local_block_number() >= *unlock_at, + Self::treasury_block_number() >= *unlock_at, BountiesError::::Premature, ); @@ -777,11 +777,10 @@ pub mod pallet { } impl Pallet { - /// Get the block number used for this pallet. + /// Get the block number used in the treasury pallet. /// - /// This comes from the Treasury pallet which may be configured to use the relay chain on a - /// parachain. - pub fn local_block_number() -> BlockNumberFor { + /// It may be configured to use the relay chain block number on a parachain. + pub fn treasury_block_number() -> BlockNumberFor { ::BlockNumberProvider::current_block_number() } From 2ef0ce3c330aeef96c60e2332cb4ae2ca723327c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 6 May 2024 20:39:55 -0400 Subject: [PATCH 20/20] Update prdoc/pr_3970.prdoc Co-authored-by: Muharem --- prdoc/pr_3970.prdoc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/prdoc/pr_3970.prdoc b/prdoc/pr_3970.prdoc index a84bcffec24a..5c20e7444782 100644 --- a/prdoc/pr_3970.prdoc +++ b/prdoc/pr_3970.prdoc @@ -8,4 +8,10 @@ doc: 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: [ ] +crates: +- name: pallet-treasury + bump: major +- name: pallet-bounties + bump: minor +- name: pallet-child-bounties + bump: minor