Try State Hook for Ranked Collective (#3007)

Part of: #239 Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW --------- Co-authored-by: Liam Aharon <[email protected]>
paritytech · Feb 8, 2024 · 9cd02a0 · 9cd02a0
1 parent 07f8592
commit 9cd02a0
Show file tree

Hide file tree

Showing 4 changed files with 186 additions and 19 deletions.
diff --git a/prdoc/pr_3007.prdoc 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
@@ -181,5 +181,5 @@ benchmarks_instance_pallet! {
 		assert_has_event::<T, I>(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
@@ -716,6 +716,14 @@ pub mod pallet {
 		}
 	}
 
+	#[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()
+		}
+	}
+
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		fn ensure_member(who: &T::AccountId) -> Result<MemberRecord, DispatchError> {
 			Members::<T, I>::get(who).ok_or(Error::<T, I>::NotMember.into())
@@ -847,6 +855,132 @@ pub mod pallet {
 		}
 	}
 
+	#[cfg(any(feature = "try-runtime", test))]
+	impl<T: Config<I>, I: 'static> Pallet<T, I> {
+		/// 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::<T, I>::iter().try_for_each(|(_, member_index)| -> DispatchResult {
+				let total_members = Members::<T, I>::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::<T, I>::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::<T, I>::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::<T, I>::iter().try_for_each(
+				|(rank, who, member_index)| -> DispatchResult {
+					let who_from_index = IndexToId::<T, I>::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::<T, I>::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::<T, I>::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::<T, I>::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::<T, I>::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<T: Config<I>, I: 'static> RankedMembers for Pallet<T, I> {
 		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
@@ -183,11 +183,28 @@ impl Config for Test {
 	type BenchmarkSetup = ();
 }
 
-pub fn new_test_ext() -> sp_io::TestExternalities {
-	let t = frame_system::GenesisConfig::<Test>::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::<Test>::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));