pallet-ranked-collective: Ensure to cleanup state in remove_member (#2591)

This ensures that we cleanup the state of `who` in `remove_member`.

---------

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>

prdoc/pr_2591.prdoc

@@ -0,0 +1,9 @@
+title: Ensure to cleanup state in remove_member
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Cleanes up the state properly if a member of a ranked collective is removed.
+
+crates:
+  - name: pallet-ranked-collective

substrate/frame/ranked-collective/src/lib.rs

@@ -663,16 +663,21 @@ pub mod pallet {
 		}
 
 		fn remove_from_rank(who: &T::AccountId, rank: Rank) -> DispatchResult {
-			let last_index = MemberCount::<T, I>::get(rank).saturating_sub(1);
-			let index = IdToIndex::<T, I>::get(rank, &who).ok_or(Error::<T, I>::Corruption)?;
-			if index != last_index {
-				let last =
-					IndexToId::<T, I>::get(rank, last_index).ok_or(Error::<T, I>::Corruption)?;
-				IdToIndex::<T, I>::insert(rank, &last, index);
-				IndexToId::<T, I>::insert(rank, index, &last);
-			}
-			MemberCount::<T, I>::mutate(rank, |r| r.saturating_dec());
-			Ok(())
+			MemberCount::<T, I>::try_mutate(rank, |last_index| {
+				last_index.saturating_dec();
+				let index = IdToIndex::<T, I>::get(rank, &who).ok_or(Error::<T, I>::Corruption)?;
+				if index != *last_index {
+					let last = IndexToId::<T, I>::get(rank, *last_index)
+						.ok_or(Error::<T, I>::Corruption)?;
+					IdToIndex::<T, I>::insert(rank, &last, index);
+					IndexToId::<T, I>::insert(rank, index, &last);
+				}
+
+				IdToIndex::<T, I>::remove(rank, who);
+				IndexToId::<T, I>::remove(rank, last_index);
+
+				Ok(())
+			})
 		}
 
 		/// Adds a member into the ranked collective at level 0.

substrate/frame/ranked-collective/src/tests.rs

@@ -23,13 +23,10 @@ use frame_support::{
 	assert_noop, assert_ok, derive_impl,
 	error::BadOrigin,
 	parameter_types,
-	traits::{ConstU16, ConstU32, ConstU64, EitherOf, Everything, MapSuccess, Polling},
-};
-use sp_core::{Get, H256};
-use sp_runtime::{
-	traits::{BlakeTwo256, IdentityLookup, ReduceBy},
-	BuildStorage,
+	traits::{ConstU16, EitherOf, MapSuccess, Polling},
 };
+use sp_core::Get;
+use sp_runtime::{traits::ReduceBy, BuildStorage};
 
 use super::*;
 use crate as pallet_ranked_collective;
@@ -47,29 +44,7 @@ frame_support::construct_runtime!(
 
 #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
 impl frame_system::Config for Test {
-	type BaseCallFilter = Everything;
-	type BlockWeights = ();
-	type BlockLength = ();
-	type DbWeight = ();
-	type RuntimeOrigin = RuntimeOrigin;
-	type Nonce = u64;
-	type RuntimeCall = RuntimeCall;
-	type Hash = H256;
-	type Hashing = BlakeTwo256;
-	type AccountId = u64;
-	type Lookup = IdentityLookup<Self::AccountId>;
 	type Block = Block;
-	type RuntimeEvent = RuntimeEvent;
-	type BlockHashCount = ConstU64<250>;
-	type Version = ();
-	type PalletInfo = PalletInfo;
-	type AccountData = ();
-	type OnNewAccount = ();
-	type OnKilledAccount = ();
-	type SystemWeightInfo = ();
-	type SS58Prefix = ();
-	type OnSetCode = ();
-	type MaxConsumers = ConstU32<16>;
 }
 
 #[derive(Clone, PartialEq, Eq, Debug)]
@@ -442,6 +417,32 @@ fn cleanup_works() {
 	});
 }
 
+#[test]
+fn remove_member_cleanup_works() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(Club::add_member(RuntimeOrigin::root(), 1));
+		assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1));
+		assert_ok!(Club::add_member(RuntimeOrigin::root(), 2));
+		assert_ok!(Club::promote_member(RuntimeOrigin::root(), 2));
+		assert_ok!(Club::add_member(RuntimeOrigin::root(), 3));
+		assert_ok!(Club::promote_member(RuntimeOrigin::root(), 3));
+
+		assert_eq!(IdToIndex::<Test>::get(1, 2), Some(1));
+		assert_eq!(IndexToId::<Test>::get(1, 1), Some(2));
+
+		assert_eq!(IdToIndex::<Test>::get(1, 3), Some(2));
+		assert_eq!(IndexToId::<Test>::get(1, 2), Some(3));
+
+		assert_ok!(Club::remove_member(RuntimeOrigin::root(), 2, 1));
+
+		assert_eq!(IdToIndex::<Test>::get(1, 2), None);
+		assert_eq!(IndexToId::<Test>::get(1, 1), Some(3));
+
+		assert_eq!(IdToIndex::<Test>::get(1, 3), Some(1));
+		assert_eq!(IndexToId::<Test>::get(1, 2), None);
+	});
+}
+
 #[test]
 fn ensure_ranked_works() {
 	new_test_ext().execute_with(|| {