From a1b2ecb902d6555880f1bd76027d3a5c03f38c26 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <git@kchr.de>
Date: Mon, 4 Dec 2023 19:43:27 +0100
Subject: [PATCH] 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 | 9 +++
substrate/frame/ranked-collective/src/lib.rs | 25 ++++----
.../frame/ranked-collective/src/tests.rs | 57 ++++++++++---------
3 files changed, 53 insertions(+), 38 deletions(-)
create mode 100644 prdoc/pr_2591.prdoc
diff --git a/prdoc/pr_2591.prdoc b/prdoc/pr_2591.prdoc
new file mode 100644
index 00000000000..fe967cb6785
--- /dev/null
+++ b/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
diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs
index deb1ccf2357..51ee7d7144b 100644
--- a/substrate/frame/ranked-collective/src/lib.rs
+++ b/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.
diff --git a/substrate/frame/ranked-collective/src/tests.rs b/substrate/frame/ranked-collective/src/tests.rs
index 3a557065160..60c0da3d7ac 100644
--- a/substrate/frame/ranked-collective/src/tests.rs
+++ b/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(|| {
--
GitLab