diff --git a/prdoc/pr_2591.prdoc b/prdoc/pr_2591.prdoc new file mode 100644 index 0000000000000000000000000000000000000000..fe967cb678592edf337ad537a2d549e4afa78b14 --- /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 deb1ccf23578b797ad0ce950b3b721609010c2ff..51ee7d7144b14d40e0bb97c1374dcdc8b39c8467 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 3a557065160904ebec1c256ba559cbab6400443b..60c0da3d7ac29d08c258a73e280c7cee8a469f03 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(|| {