From 9cd02a07c9a3f5f5de56611c3852f0eeefae9013 Mon Sep 17 00:00:00 2001 From: dharjeezy Date: Thu, 8 Feb 2024 15:03:30 +0100 Subject: [PATCH] Try State Hook for Ranked Collective (#3007) Part of: paritytech/polkadot-sdk#239 Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW --------- Co-authored-by: Liam Aharon --- prdoc/pr_3007.prdoc | 16 +++ .../ranked-collective/src/benchmarking.rs | 2 +- substrate/frame/ranked-collective/src/lib.rs | 134 ++++++++++++++++++ .../frame/ranked-collective/src/tests.rs | 53 ++++--- 4 files changed, 186 insertions(+), 19 deletions(-) create mode 100644 prdoc/pr_3007.prdoc diff --git a/prdoc/pr_3007.prdoc b/prdoc/pr_3007.prdoc new file mode 100644 index 000000000000..17870469a219 --- /dev/null +++ b/prdoc/pr_3007.prdoc @@ -0,0 +1,16 @@ +title: Try State Hook for Ranked Collective. + +doc: + - audience: Runtime User + description: | + Invariants for storage items in the ranked collective pallet. Enforces the following Invariants: + 1. Total number of `Members` in storage should be >= [`MemberIndex`] of a [`Rank`] in `MemberCount`. + 2. `Rank` in Members should be in `MemberCount`. + 3.`Sum` of `MemberCount` index should be the same as the sum of all the index attained for + rank possessed by `Members` + 4. `Member` in storage of `IdToIndex` should be the same as `Member` in `IndexToId`. + 5. `Rank` in `IdToIndex` should be the same as the the `Rank` in `IndexToId`. + 6. `Rank` of the member `who` in `IdToIndex` should be the same as the `Rank` of + the member `who` in `Members` +crates: +- name: pallet-ranked-collective diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index f093fd93e6e2..462f55a238d2 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -181,5 +181,5 @@ benchmarks_instance_pallet! { assert_has_event::(Event::MemberExchanged { who, new_who }.into()); } - impl_benchmark_test_suite!(RankedCollective, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(RankedCollective, crate::tests::ExtBuilder::default().build(), crate::tests::Test); } diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index 65ea886acc4f..2b4fb2dbff47 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -716,6 +716,14 @@ pub mod pallet { } } + #[pallet::hooks] + impl, I: 'static> Hooks> for Pallet { + #[cfg(feature = "try-runtime")] + fn try_state(_n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { + Self::do_try_state() + } + } + impl, I: 'static> Pallet { fn ensure_member(who: &T::AccountId) -> Result { Members::::get(who).ok_or(Error::::NotMember.into()) @@ -847,6 +855,132 @@ pub mod pallet { } } + #[cfg(any(feature = "try-runtime", test))] + impl, I: 'static> Pallet { + /// Ensure the correctness of the state of this pallet. + pub fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> { + Self::try_state_members()?; + Self::try_state_index()?; + + Ok(()) + } + + /// ### Invariants of Member storage items + /// + /// Total number of [`Members`] in storage should be >= [`MemberIndex`] of a [`Rank`] in + /// [`MemberCount`]. + /// [`Rank`] in Members should be in [`MemberCount`] + /// [`Sum`] of [`MemberCount`] index should be the same as the sum of all the index attained + /// for rank possessed by [`Members`] + fn try_state_members() -> Result<(), sp_runtime::TryRuntimeError> { + MemberCount::::iter().try_for_each(|(_, member_index)| -> DispatchResult { + let total_members = Members::::iter().count(); + ensure!( + total_members as u32 >= member_index, + "Total count of `Members` should be greater than or equal to the number of `MemberIndex` of a particular `Rank` in `MemberCount`." + ); + + Ok(()) + })?; + + let mut sum_of_member_rank_indexes = 0; + Members::::iter().try_for_each(|(_, member_record)| -> DispatchResult { + ensure!( + Self::is_rank_in_member_count(member_record.rank.into()), + "`Rank` in Members should be in `MemberCount`" + ); + + sum_of_member_rank_indexes += Self::determine_index_of_a_rank(member_record.rank); + + Ok(()) + })?; + + let sum_of_all_member_count_indexes = + MemberCount::::iter_values().fold(0, |sum, index| sum + index); + ensure!( + sum_of_all_member_count_indexes == sum_of_member_rank_indexes as u32, + "Sum of `MemberCount` index should be the same as the sum of all the index attained for rank possessed by `Members`" + ); + Ok(()) + } + + /// ### Invariants of Index storage items + /// [`Member`] in storage of [`IdToIndex`] should be the same as [`Member`] in [`IndexToId`] + /// [`Rank`] in [`IdToIndex`] should be the same as the the [`Rank`] in [`IndexToId`] + /// [`Rank`] of the member [`who`] in [`IdToIndex`] should be the same as the [`Rank`] of + /// the member [`who`] in [`Members`] + fn try_state_index() -> Result<(), sp_runtime::TryRuntimeError> { + IdToIndex::::iter().try_for_each( + |(rank, who, member_index)| -> DispatchResult { + let who_from_index = IndexToId::::get(rank, member_index).unwrap(); + ensure!( + who == who_from_index, + "`Member` in storage of `IdToIndex` should be the same as `Member` in `IndexToId`." + ); + + ensure!( + Self::is_rank_in_index_to_id_storage(rank.into()), + "`Rank` in `IdToIndex` should be the same as the `Rank` in `IndexToId`" + ); + Ok(()) + }, + )?; + + Members::::iter().try_for_each(|(who, member_record)| -> DispatchResult { + ensure!( + Self::is_who_rank_in_id_to_index_storage(who, member_record.rank), + "`Rank` of the member `who` in `IdToIndex` should be the same as the `Rank` of the member `who` in `Members`" + ); + + Ok(()) + })?; + + Ok(()) + } + + /// Checks if a rank is part of the `MemberCount` + fn is_rank_in_member_count(rank: u32) -> bool { + for (r, _) in MemberCount::::iter() { + if r as u32 == rank { + return true; + } + } + + return false; + } + + /// Checks if a rank is the same as the rank `IndexToId` + fn is_rank_in_index_to_id_storage(rank: u32) -> bool { + for (r, _, _) in IndexToId::::iter() { + if r as u32 == rank { + return true; + } + } + + return false; + } + + /// Checks if a member(who) rank is the same as the rank of a member(who) in `IdToIndex` + fn is_who_rank_in_id_to_index_storage(who: T::AccountId, rank: u16) -> bool { + for (rank_, who_, _) in IdToIndex::::iter() { + if who == who_ && rank == rank_ { + return true; + } + } + + return false; + } + + /// Determines the total index for a rank + fn determine_index_of_a_rank(rank: u16) -> u16 { + let mut sum = 0; + for _ in 0..rank + 1 { + sum += 1; + } + sum + } + } + impl, I: 'static> RankedMembers for Pallet { type AccountId = T::AccountId; type Rank = Rank; diff --git a/substrate/frame/ranked-collective/src/tests.rs b/substrate/frame/ranked-collective/src/tests.rs index c5fccd3f7724..31add52d90af 100644 --- a/substrate/frame/ranked-collective/src/tests.rs +++ b/substrate/frame/ranked-collective/src/tests.rs @@ -183,11 +183,28 @@ impl Config for Test { type BenchmarkSetup = (); } -pub fn new_test_ext() -> sp_io::TestExternalities { - let t = frame_system::GenesisConfig::::default().build_storage().unwrap(); - let mut ext = sp_io::TestExternalities::new(t); - ext.execute_with(|| System::set_block_number(1)); - ext +pub struct ExtBuilder {} + +impl Default for ExtBuilder { + fn default() -> Self { + Self {} + } +} + +impl ExtBuilder { + pub fn build(self) -> sp_io::TestExternalities { + let t = frame_system::GenesisConfig::::default().build_storage().unwrap(); + let mut ext = sp_io::TestExternalities::new(t); + ext.execute_with(|| System::set_block_number(1)); + ext + } + + pub fn build_and_execute(self, test: impl FnOnce() -> ()) { + self.build().execute_with(|| { + test(); + Club::do_try_state().expect("All invariants must hold after a test"); + }) + } } fn next_block() { @@ -225,14 +242,14 @@ fn completed_poll_should_panic() { #[test] fn basic_stuff() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_eq!(tally(3), Tally::from_parts(0, 0, 0)); }); } #[test] fn member_lifecycle_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::demote_member(RuntimeOrigin::root(), 1)); @@ -244,7 +261,7 @@ fn member_lifecycle_works() { #[test] fn add_remove_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_noop!(Club::add_member(RuntimeOrigin::signed(1), 1), DispatchError::BadOrigin); assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); assert_eq!(member_count(0), 1); @@ -274,7 +291,7 @@ fn add_remove_works() { #[test] fn promote_demote_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_noop!(Club::add_member(RuntimeOrigin::signed(1), 1), DispatchError::BadOrigin); assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); assert_eq!(member_count(0), 1); @@ -305,7 +322,7 @@ fn promote_demote_works() { #[test] fn promote_demote_by_rank_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); for _ in 0..7 { assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1)); @@ -372,7 +389,7 @@ fn promote_demote_by_rank_works() { #[test] fn voting_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_ok!(Club::add_member(RuntimeOrigin::root(), 0)); assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1)); @@ -406,7 +423,7 @@ fn voting_works() { #[test] fn cleanup_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::add_member(RuntimeOrigin::root(), 2)); @@ -433,7 +450,7 @@ fn cleanup_works() { #[test] fn remove_member_cleanup_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::add_member(RuntimeOrigin::root(), 2)); @@ -459,7 +476,7 @@ fn remove_member_cleanup_works() { #[test] fn ensure_ranked_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::add_member(RuntimeOrigin::root(), 2)); @@ -528,7 +545,7 @@ fn ensure_ranked_works() { #[test] fn do_add_member_to_rank_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let max_rank = 9u16; assert_ok!(Club::do_add_member_to_rank(69, max_rank / 2, true)); assert_ok!(Club::do_add_member_to_rank(1337, max_rank, true)); @@ -545,7 +562,7 @@ fn do_add_member_to_rank_works() { #[test] fn tally_support_correct() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { // add members, // rank 1: accounts 1, 2, 3 // rank 2: accounts 2, 3 @@ -585,7 +602,7 @@ fn tally_support_correct() { #[test] fn exchange_member_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); assert_eq!(member_count(0), 1); @@ -613,7 +630,7 @@ fn exchange_member_works() { #[test] fn exchange_member_same_noops() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1)); assert_ok!(Club::add_member(RuntimeOrigin::root(), 2));