From 4aa29a41cf731b8181f03168240e8dedb2adfa7a Mon Sep 17 00:00:00 2001
From: dharjeezy <dharjeezy@gmail.com>
Date: Fri, 12 Jul 2024 12:51:33 +0100
Subject: [PATCH] Try State Hook for Bounties (#4563)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Part of: https://github.com/paritytech/polkadot-sdk/issues/239
Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW
---------
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
---
prdoc/pr_4563.prdoc | 12 +++
substrate/frame/bounties/src/benchmarking.rs | 2 +-
substrate/frame/bounties/src/lib.rs | 48 ++++++++++
substrate/frame/bounties/src/tests.rs | 92 ++++++++++++--------
4 files changed, 116 insertions(+), 38 deletions(-)
create mode 100644 prdoc/pr_4563.prdoc
diff --git a/prdoc/pr_4563.prdoc b/prdoc/pr_4563.prdoc
new file mode 100644
index 00000000000..3780eee5898
--- /dev/null
+++ b/prdoc/pr_4563.prdoc
@@ -0,0 +1,12 @@
+title: Try State Hook for Bounties.
+
+doc:
+ - audience: Runtime User
+ description: |
+ Invariants for storage items in the bounties pallet. Enforces the following Invariants:
+ 1.`BountyCount` should be greater or equals to the length of the number of items in `Bounties`.
+ 2.`BountyCount` should be greater or equals to the length of the number of items in `BountyDescriptions`.
+ 3. Number of items in `Bounties` should be the same as `BountyDescriptions` length.
+crates:
+- name: pallet-bounties
+ bump: minor
diff --git a/substrate/frame/bounties/src/benchmarking.rs b/substrate/frame/bounties/src/benchmarking.rs
index a009b132f05..f53d95d919d 100644
--- a/substrate/frame/bounties/src/benchmarking.rs
+++ b/substrate/frame/bounties/src/benchmarking.rs
@@ -231,5 +231,5 @@ benchmarks_instance_pallet! {
}
}
- impl_benchmark_test_suite!(Bounties, crate::tests::new_test_ext(), crate::tests::Test)
+ impl_benchmark_test_suite!(Bounties, crate::tests::ExtBuilder::default().build(), crate::tests::Test)
}
diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs
index e27ecbda3a2..c04bec2d12a 100644
--- a/substrate/frame/bounties/src/lib.rs
+++ b/substrate/frame/bounties/src/lib.rs
@@ -807,6 +807,54 @@ pub mod pallet {
Ok(())
}
}
+
+ #[pallet::hooks]
+ impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
+ #[cfg(feature = "try-runtime")]
+ fn try_state(_n: BlockNumberFor<T>) -> Result<(), sp_runtime::TryRuntimeError> {
+ Self::do_try_state()
+ }
+ }
+}
+
+#[cfg(any(feature = "try-runtime", test))]
+impl<T: Config<I>, I: 'static> Pallet<T, I> {
+ /// Ensure the correctness of the state of this pallet.
+ ///
+ /// This should be valid before or after each state transition of this pallet.
+ pub fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> {
+ Self::try_state_bounties_count()?;
+
+ Ok(())
+ }
+
+ /// # Invariants
+ ///
+ /// * `BountyCount` should be greater or equals to the length of the number of items in
+ /// `Bounties`.
+ /// * `BountyCount` should be greater or equals to the length of the number of items in
+ /// `BountyDescriptions`.
+ /// * Number of items in `Bounties` should be the same as `BountyDescriptions` length.
+ fn try_state_bounties_count() -> Result<(), sp_runtime::TryRuntimeError> {
+ let bounties_length = Bounties::<T, I>::iter().count() as u32;
+
+ ensure!(
+ <BountyCount<T, I>>::get() >= bounties_length,
+ "`BountyCount` must be grater or equals the number of `Bounties` in storage"
+ );
+
+ let bounties_description_length = BountyDescriptions::<T, I>::iter().count() as u32;
+ ensure!(
+ <BountyCount<T, I>>::get() >= bounties_description_length,
+ "`BountyCount` must be grater or equals the number of `BountiesDescriptions` in storage."
+ );
+
+ ensure!(
+ bounties_length == bounties_description_length,
+ "Number of `Bounties` in storage must be the same as the Number of `BountiesDescription` in storage."
+ );
+ Ok(())
+ }
}
impl<T: Config<I>, I: 'static> Pallet<T, I> {
diff --git a/substrate/frame/bounties/src/tests.rs b/substrate/frame/bounties/src/tests.rs
index 9f3a52263ec..7cd47982674 100644
--- a/substrate/frame/bounties/src/tests.rs
+++ b/substrate/frame/bounties/src/tests.rs
@@ -167,18 +167,36 @@ impl Config<Instance1> for Test {
type TreasuryError = pallet_treasury::Error<Test>;
type TreasuryError1 = pallet_treasury::Error<Test, Instance1>;
-pub fn new_test_ext() -> sp_io::TestExternalities {
- let mut ext: sp_io::TestExternalities = RuntimeGenesisConfig {
- system: frame_system::GenesisConfig::default(),
- balances: pallet_balances::GenesisConfig { balances: vec![(0, 100), (1, 98), (2, 1)] },
- treasury: Default::default(),
- treasury_1: Default::default(),
+pub struct ExtBuilder {}
+
+impl Default for ExtBuilder {
+ fn default() -> Self {
+ Self {}
+ }
+}
+
+impl ExtBuilder {
+ pub fn build(self) -> sp_io::TestExternalities {
+ let mut ext: sp_io::TestExternalities = RuntimeGenesisConfig {
+ system: frame_system::GenesisConfig::default(),
+ balances: pallet_balances::GenesisConfig { balances: vec![(0, 100), (1, 98), (2, 1)] },
+ treasury: Default::default(),
+ treasury_1: Default::default(),
+ }
+ .build_storage()
+ .unwrap()
+ .into();
+ ext.execute_with(|| System::set_block_number(1));
+ ext
+ }
+
+ pub fn build_and_execute(self, test: impl FnOnce() -> ()) {
+ self.build().execute_with(|| {
+ test();
+ Bounties::do_try_state().expect("All invariants must hold after a test");
+ Bounties1::do_try_state().expect("All invariants must hold after a test");
+ })
}
- .build_storage()
- .unwrap()
- .into();
- ext.execute_with(|| System::set_block_number(1));
- ext
}
fn last_event() -> BountiesEvent<Test> {
@@ -192,7 +210,7 @@ fn last_event() -> BountiesEvent<Test> {
#[test]
fn genesis_config_works() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
assert_eq!(Treasury::pot(), 0);
assert_eq!(Treasury::proposal_count(), 0);
});
@@ -200,7 +218,7 @@ fn genesis_config_works() {
#[test]
fn minting_works() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
// Check that accumulate works when we have Some value in Dummy already.
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);
@@ -209,7 +227,7 @@ fn minting_works() {
#[test]
fn accepted_spend_proposal_ignored_outside_spend_period() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 100, 3) });
@@ -222,7 +240,7 @@ fn accepted_spend_proposal_ignored_outside_spend_period() {
#[test]
fn unused_pot_should_diminish() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
let init_total_issuance = Balances::total_issuance();
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Balances::total_issuance(), init_total_issuance + 100);
@@ -235,7 +253,7 @@ fn unused_pot_should_diminish() {
#[test]
fn accepted_spend_proposal_enacted_on_spend_period() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);
@@ -249,7 +267,7 @@ fn accepted_spend_proposal_enacted_on_spend_period() {
#[test]
fn pot_underflow_should_not_diminish() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);
@@ -269,7 +287,7 @@ fn pot_underflow_should_not_diminish() {
// i.e. pot should not include existential deposit needed for account survival.
#[test]
fn treasury_account_doesnt_get_deleted() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);
let treasury_balance = Balances::free_balance(&Treasury::account_id());
@@ -321,7 +339,7 @@ fn inexistent_account_works() {
#[test]
fn propose_bounty_works() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
@@ -358,7 +376,7 @@ fn propose_bounty_works() {
#[test]
fn propose_bounty_validation_works() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
@@ -387,7 +405,7 @@ fn propose_bounty_validation_works() {
#[test]
fn close_bounty_works() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_noop!(Bounties::close_bounty(RuntimeOrigin::root(), 0), Error::<Test>::InvalidIndex);
@@ -412,7 +430,7 @@ fn close_bounty_works() {
#[test]
fn approve_bounty_works() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_noop!(
@@ -473,7 +491,7 @@ fn approve_bounty_works() {
#[test]
fn assign_curator_works() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
@@ -543,7 +561,7 @@ fn assign_curator_works() {
#[test]
fn unassign_curator_works() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
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()));
@@ -596,7 +614,7 @@ fn unassign_curator_works() {
#[test]
fn award_and_claim_bounty_works() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
Balances::make_free_balance_be(&4, 10);
@@ -663,7 +681,7 @@ fn award_and_claim_bounty_works() {
#[test]
fn claim_handles_high_fee() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
Balances::make_free_balance_be(&4, 30);
@@ -704,7 +722,7 @@ fn claim_handles_high_fee() {
#[test]
fn cancel_and_refund() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
@@ -747,7 +765,7 @@ fn cancel_and_refund() {
#[test]
fn award_and_cancel() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
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()));
@@ -790,7 +808,7 @@ fn award_and_cancel() {
#[test]
fn expire_and_unassign() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
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()));
@@ -838,7 +856,7 @@ fn expire_and_unassign() {
#[test]
fn extend_expiry() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
Balances::make_free_balance_be(&4, 10);
@@ -974,7 +992,7 @@ fn genesis_funding_works() {
#[test]
fn unassign_curator_self() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
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()));
@@ -1015,7 +1033,7 @@ fn unassign_curator_self() {
fn accept_curator_handles_different_deposit_calculations() {
// This test will verify that a bounty with and without a fee results
// in a different curator deposit: one using the value, and one using the fee.
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
// Case 1: With a fee
let user = 1;
let bounty_index = 0;
@@ -1092,7 +1110,7 @@ fn accept_curator_handles_different_deposit_calculations() {
#[test]
fn approve_bounty_works_second_instance() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
// Set burn to 0 to make tracking funds easier.
Burn::set(Permill::from_percent(0));
@@ -1118,7 +1136,7 @@ fn approve_bounty_works_second_instance() {
#[test]
fn approve_bounty_insufficient_spend_limit_errors() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
@@ -1136,7 +1154,7 @@ fn approve_bounty_insufficient_spend_limit_errors() {
#[test]
fn approve_bounty_instance1_insufficient_spend_limit_errors() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury1::account_id(), 101);
@@ -1154,7 +1172,7 @@ fn approve_bounty_instance1_insufficient_spend_limit_errors() {
#[test]
fn propose_curator_insufficient_spend_limit_errors() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
@@ -1177,7 +1195,7 @@ fn propose_curator_insufficient_spend_limit_errors() {
#[test]
fn propose_curator_instance1_insufficient_spend_limit_errors() {
- new_test_ext().execute_with(|| {
+ ExtBuilder::default().build_and_execute(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
--
GitLab