[FRAME] Make core-fellowship ans salary work for swapped members (#3156)

Fixup for https://github.com/paritytech/polkadot-sdk/pull/2587 to make
the `core-fellowship` crate work with swapped members.

Adds a `MemberSwappedHandler` to the `ranked-collective` pallet that are
implemented by `core-fellowship+salary`.
There is are exhaustive tests
[here](https://github.com/paritytech/polkadot-sdk/blob/72aa7ac17a0e5b16faab5d2992aa2db2e01b05d0/substrate/frame/core-fellowship/src/tests/integration.rs#L338)
and
[here](https://github.com/paritytech/polkadot-sdk/blob/ab3cdb05a5ebc1ff841f8dda67edef0ea40bbba5/substrate/frame/salary/src/tests/integration.rs#L224)
to check that adding member `1` is equivalent to adding member `0` and
then swapping.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

substrate/frame/core-fellowship/src/benchmarking.rs

@@ -227,7 +227,7 @@ mod benchmarks {
 
 	impl_benchmark_test_suite! {
 		CoreFellowship,
-		crate::tests::new_test_ext(),
-		crate::tests::Test,
+		crate::tests::unit::new_test_ext(),
+		crate::tests::unit::Test,
 	}
 }

substrate/frame/core-fellowship/src/lib.rs

@@ -65,10 +65,12 @@ use sp_runtime::RuntimeDebug;
 use sp_std::{marker::PhantomData, prelude::*};
 
 use frame_support::{
+	defensive,
 	dispatch::DispatchResultWithPostInfo,
 	ensure, impl_ensure_origin_with_arg_ignoring_arg,
 	traits::{
 		tokens::Balance as BalanceTrait, EnsureOrigin, EnsureOriginWithArg, Get, RankedMembers,
+		RankedMembersSwapHandler,
 	},
 	BoundedVec,
 };
@@ -249,6 +251,8 @@ pub mod pallet {
 		},
 		/// Pre-ranked account has been inducted at their current rank.
 		Imported { who: T::AccountId, rank: RankOf<T, I> },
+		/// A member had its AccountId swapped.
+		Swapped { who: T::AccountId, new_who: T::AccountId },
 	}
 
 	#[pallet::error]
@@ -603,3 +607,38 @@ impl_ensure_origin_with_arg_ignoring_arg! {
 		EnsureOriginWithArg<T::RuntimeOrigin, A> for EnsureInducted<T, I, MIN_RANK>
 	{}
 }
+
+impl<T: Config<I>, I: 'static> RankedMembersSwapHandler<T::AccountId, u16> for Pallet<T, I> {
+	fn swapped(old: &T::AccountId, new: &T::AccountId, _rank: u16) {
+		if old == new {
+			defensive!("Should not try to swap with self");
+			return
+		}
+		if !Member::<T, I>::contains_key(old) {
+			defensive!("Should not try to swap non-member");
+			return
+		}
+		if Member::<T, I>::contains_key(new) {
+			defensive!("Should not try to overwrite existing member");
+			return
+		}
+
+		if let Some(member) = Member::<T, I>::take(old) {
+			Member::<T, I>::insert(new, member);
+		}
+		if let Some(we) = MemberEvidence::<T, I>::take(old) {
+			MemberEvidence::<T, I>::insert(new, we);
+		}
+
+		Self::deposit_event(Event::<T, I>::Swapped { who: old.clone(), new_who: new.clone() });
+	}
+}
+
+#[cfg(feature = "runtime-benchmarks")]
+impl<T: Config<I>, I: 'static>
+	pallet_ranked_collective::BenchmarkSetup<<T as frame_system::Config>::AccountId> for Pallet<T, I>
+{
+	fn ensure_member(who: &<T as frame_system::Config>::AccountId) {
+		Self::import(frame_system::RawOrigin::Signed(who.clone()).into()).unwrap();
+	}
+}

substrate/frame/core-fellowship/src/tests/integration.rs

@@ -0,0 +1,278 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Integration test together with the ranked-collective pallet.
+
+use frame_support::{
+	assert_noop, assert_ok, derive_impl, hypothetically, ord_parameter_types,
+	pallet_prelude::Weight,
+	parameter_types,
+	traits::{ConstU16, EitherOf, IsInVec, MapSuccess, PollStatus, Polling, TryMapSuccess},
+};
+use frame_system::EnsureSignedBy;
+use pallet_ranked_collective::{EnsureRanked, Geometric, Rank, TallyOf, Votes};
+use sp_core::Get;
+use sp_runtime::{
+	traits::{Convert, ReduceBy, TryMorphInto},
+	BuildStorage, DispatchError,
+};
+type Class = Rank;
+
+use crate as pallet_core_fellowship;
+use crate::*;
+
+type Block = frame_system::mocking::MockBlock<Test>;
+
+frame_support::construct_runtime!(
+	pub enum Test
+	{
+		System: frame_system,
+		CoreFellowship: pallet_core_fellowship,
+		Club: pallet_ranked_collective,
+	}
+);
+
+parameter_types! {
+	pub BlockWeights: frame_system::limits::BlockWeights =
+		frame_system::limits::BlockWeights::simple_max(Weight::from_parts(1_000_000, u64::max_value()));
+}
+
+#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
+impl frame_system::Config for Test {
+	type Block = Block;
+}
+
+parameter_types! {
+	pub static MinRankOfClassDelta: Rank = 0;
+}
+
+parameter_types! {
+	pub ZeroToNine: Vec<u64> = vec![0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
+	pub EvidenceSize: u32 = 1024;
+}
+ord_parameter_types! {
+	pub const One: u64 = 1;
+}
+
+impl Config for Test {
+	type WeightInfo = ();
+	type RuntimeEvent = RuntimeEvent;
+	type Members = Club;
+	type Balance = u64;
+	type ParamsOrigin = EnsureSignedBy<One, u64>;
+	type InductOrigin = EnsureInducted<Test, (), 1>;
+	type ApproveOrigin = TryMapSuccess<EnsureSignedBy<IsInVec<ZeroToNine>, u64>, TryMorphInto<u16>>;
+	type PromoteOrigin = TryMapSuccess<EnsureSignedBy<IsInVec<ZeroToNine>, u64>, TryMorphInto<u16>>;
+	type EvidenceSize = EvidenceSize;
+}
+
+pub struct TestPolls;
+impl Polling<TallyOf<Test>> for TestPolls {
+	type Index = u8;
+	type Votes = Votes;
+	type Moment = u64;
+	type Class = Class;
+
+	fn classes() -> Vec<Self::Class> {
+		unimplemented!()
+	}
+	fn as_ongoing(_: u8) -> Option<(TallyOf<Test>, Self::Class)> {
+		unimplemented!()
+	}
+	fn access_poll<R>(
+		_: Self::Index,
+		_: impl FnOnce(PollStatus<&mut TallyOf<Test>, Self::Moment, Self::Class>) -> R,
+	) -> R {
+		unimplemented!()
+	}
+	fn try_access_poll<R>(
+		_: Self::Index,
+		_: impl FnOnce(
+			PollStatus<&mut TallyOf<Test>, Self::Moment, Self::Class>,
+		) -> Result<R, DispatchError>,
+	) -> Result<R, DispatchError> {
+		unimplemented!()
+	}
+
+	#[cfg(feature = "runtime-benchmarks")]
+	fn create_ongoing(_: Self::Class) -> Result<Self::Index, ()> {
+		unimplemented!()
+	}
+
+	#[cfg(feature = "runtime-benchmarks")]
+	fn end_ongoing(_: Self::Index, _: bool) -> Result<(), ()> {
+		unimplemented!()
+	}
+}
+
+/// Convert the tally class into the minimum rank required to vote on the poll.
+/// MinRank(Class) = Class - Delta
+pub struct MinRankOfClass<Delta>(PhantomData<Delta>);
+impl<Delta: Get<Rank>> Convert<Class, Rank> for MinRankOfClass<Delta> {
+	fn convert(a: Class) -> Rank {
+		a.saturating_sub(Delta::get())
+	}
+}
+
+impl pallet_ranked_collective::Config for Test {
+	type WeightInfo = ();
+	type RuntimeEvent = RuntimeEvent;
+	type PromoteOrigin = EitherOf<
+		// Root can promote arbitrarily.
+		frame_system::EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>,
+		// Members can promote up to the rank of 2 below them.
+		MapSuccess<EnsureRanked<Test, (), 2>, ReduceBy<ConstU16<2>>>,
+	>;
+	type DemoteOrigin = EitherOf<
+		// Root can demote arbitrarily.
+		frame_system::EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>,
+		// Members can demote up to the rank of 3 below them.
+		MapSuccess<EnsureRanked<Test, (), 3>, ReduceBy<ConstU16<3>>>,
+	>;
+	type ExchangeOrigin = EitherOf<
+		// Root can exchange arbitrarily.
+		frame_system::EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>,
+		// Members can exchange up to the rank of 2 below them.
+		MapSuccess<EnsureRanked<Test, (), 2>, ReduceBy<ConstU16<2>>>,
+	>;
+	type Polls = TestPolls;
+	type MinRankOfClass = MinRankOfClass<MinRankOfClassDelta>;
+	type MemberSwappedHandler = CoreFellowship;
+	type VoteWeight = Geometric;
+	#[cfg(feature = "runtime-benchmarks")]
+	type BenchmarkSetup = CoreFellowship;
+}
+
+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(|| {
+		let params = ParamsType {
+			active_salary: [10, 20, 30, 40, 50, 60, 70, 80, 90],
+			passive_salary: [1, 2, 3, 4, 5, 6, 7, 8, 9],
+			demotion_period: [2, 4, 6, 8, 10, 12, 14, 16, 18],
+			min_promotion_period: [3, 6, 9, 12, 15, 18, 21, 24, 27],
+			offboard_timeout: 1,
+		};
+		assert_ok!(CoreFellowship::set_params(signed(1), Box::new(params)));
+		System::set_block_number(1);
+	});
+	ext
+}
+
+fn promote_n_times(acc: u64, r: u16) {
+	for _ in 0..r {
+		assert_ok!(Club::promote_member(RuntimeOrigin::root(), acc));
+	}
+}
+
+fn signed(who: u64) -> RuntimeOrigin {
+	RuntimeOrigin::signed(who)
+}
+
+fn assert_last_event(generic_event: <Test as Config>::RuntimeEvent) {
+	let events = frame_system::Pallet::<Test>::events();
+	let system_event: <Test as frame_system::Config>::RuntimeEvent = generic_event.into();
+	let frame_system::EventRecord { event, .. } = events.last().expect("Event expected");
+	assert_eq!(event, &system_event.into());
+}
+
+fn evidence(e: u32) -> Evidence<Test, ()> {
+	e.encode()
+		.into_iter()
+		.cycle()
+		.take(1024)
+		.collect::<Vec<_>>()
+		.try_into()
+		.expect("Static length matches")
+}
+
+#[test]
+fn swap_simple_works() {
+	new_test_ext().execute_with(|| {
+		for i in 0u16..9 {
+			let acc = i as u64;
+
+			assert_ok!(Club::add_member(RuntimeOrigin::root(), acc));
+			promote_n_times(acc, i);
+			assert_ok!(CoreFellowship::import(signed(acc)));
+
+			// Swapping normally works:
+			assert_ok!(Club::exchange_member(RuntimeOrigin::root(), acc, acc + 10));
+			assert_last_event(Event::Swapped { who: acc, new_who: acc + 10 }.into());
+		}
+	});
+}
+
+/// Exhaustively test that adding member `1` is equivalent to adding member `0` and then swapping.
+///
+/// The member also submits evidence before the swap.
+#[test]
+fn swap_exhaustive_works() {
+	new_test_ext().execute_with(|| {
+		let root_add = hypothetically!({
+			assert_ok!(Club::add_member(RuntimeOrigin::root(), 1));
+			promote_n_times(1, 4);
+			assert_ok!(CoreFellowship::import(signed(1)));
+			assert_ok!(CoreFellowship::submit_evidence(signed(1), Wish::Retention, evidence(1)));
+
+			// The events mess up the storage root:
+			System::reset_events();
+			sp_io::storage::root(sp_runtime::StateVersion::V1)
+		});
+
+		let root_swap = hypothetically!({
+			assert_ok!(Club::add_member(RuntimeOrigin::root(), 0));
+			promote_n_times(0, 4);
+			assert_ok!(CoreFellowship::import(signed(0)));
+			assert_ok!(CoreFellowship::submit_evidence(signed(0), Wish::Retention, evidence(1)));
+
+			// Now we swap:
+			assert_ok!(Club::exchange_member(RuntimeOrigin::root(), 0, 1));
+
+			System::reset_events();
+			sp_io::storage::root(sp_runtime::StateVersion::V1)
+		});
+
+		assert_eq!(root_add, root_swap);
+		// Ensure that we dont compare trivial stuff like `()` from a type error above.
+		assert_eq!(root_add.len(), 32);
+	});
+}
+
+#[test]
+fn swap_bad_noops() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(Club::add_member(RuntimeOrigin::root(), 0));
+		promote_n_times(0, 0);
+		assert_ok!(CoreFellowship::import(signed(0)));
+		assert_ok!(Club::add_member(RuntimeOrigin::root(), 1));
+		promote_n_times(1, 1);
+		assert_ok!(CoreFellowship::import(signed(1)));
+
+		// Swapping for another member is a noop:
+		assert_noop!(
+			Club::exchange_member(RuntimeOrigin::root(), 0, 1),
+			pallet_ranked_collective::Error::<Test>::AlreadyMember
+		);
+		// Swapping for the same member is a noop:
+		assert_noop!(
+			Club::exchange_member(RuntimeOrigin::root(), 0, 0),
+			pallet_ranked_collective::Error::<Test>::SameMember
+		);
+	});
+}

substrate/frame/core-fellowship/src/tests/mod.rs

@@ -0,0 +1,23 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#![cfg(test)]
+
+//! Tests for this crate.
+
+pub(crate) mod integration;
+pub(crate) mod unit;

substrate/frame/core-fellowship/src/tests/unit.rs

@@ -23,18 +23,14 @@ use frame_support::{
 	assert_noop, assert_ok, derive_impl, ord_parameter_types,
 	pallet_prelude::Weight,
 	parameter_types,
-	traits::{tokens::GetSalary, ConstU32, ConstU64, Everything, IsInVec, TryMapSuccess},
+	traits::{tokens::GetSalary, ConstU32, IsInVec, TryMapSuccess},
 };
 use frame_system::EnsureSignedBy;
-use sp_core::H256;
-use sp_runtime::{
-	traits::{BlakeTwo256, IdentityLookup, TryMorphInto},
-	BuildStorage, DispatchError, DispatchResult,
-};
+use sp_runtime::{traits::TryMorphInto, BuildStorage, DispatchError, DispatchResult};
 use sp_std::cell::RefCell;
 
-use super::*;
 use crate as pallet_core_fellowship;
+use crate::*;
 
 type Block = frame_system::mocking::MockBlock<Test>;
 
@@ -53,29 +49,7 @@ parameter_types! {
 
 #[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>;
 }
 
 thread_local! {