exchange member with a new account and same rank in the ranked collec… (#2587)

closes https://github.com/polkadot-fellows/help-center/issues/1

---------

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

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

@@ -173,5 +173,20 @@ benchmarks_instance_pallet! {
 		assert_eq!(Voting::<T, I>::iter().count(), 0);
 	}
 
+	exchange_member {
+		let who = make_member::<T, I>(1);
+		let who_lookup = T::Lookup::unlookup(who.clone());
+		let new_who = account::<T::AccountId>("new-member", 0, SEED);
+		let new_who_lookup = T::Lookup::unlookup(new_who.clone());
+		let origin =
+			T::ExchangeOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
+		let call = Call::<T, I>::exchange_member { who: who_lookup, new_who:new_who_lookup};
+	}: { call.dispatch_bypass_filter(origin)? }
+	verify {
+		assert_eq!(Members::<T, I>::get(&new_who).unwrap().rank, 1);
+		assert_eq!(Members::<T, I>::get(&who), None);
+		assert_last_event::<T, I>(Event::MemberExchanged { who, new_who }.into());
+	}
+
 	impl_benchmark_test_suite!(RankedCollective, crate::tests::new_test_ext(), crate::tests::Test);
 }

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

@@ -161,6 +161,7 @@ impl<T: Config<I>, I: 'static, M: GetMaxVoters<Class = ClassOf<T, I>>>
 				crate::Pallet::<T, I>::do_add_member_to_rank(
 					who,
 					T::MinRankOfClass::convert(class.clone()),
+					true,
 				)
 				.expect("could not add members for benchmarks");
 			}
@@ -299,7 +300,7 @@ impl<T: Config<I>, I: 'static> EnsureOriginWithArg<T::RuntimeOrigin, Rank> for E
 	#[cfg(feature = "runtime-benchmarks")]
 	fn try_successful_origin(min_rank: &Rank) -> Result<T::RuntimeOrigin, ()> {
 		let who = frame_benchmarking::account::<T::AccountId>("successful_origin", 0, 0);
-		crate::Pallet::<T, I>::do_add_member_to_rank(who.clone(), *min_rank)
+		crate::Pallet::<T, I>::do_add_member_to_rank(who.clone(), *min_rank, true)
 			.expect("Could not add members for benchmarks");
 		Ok(frame_system::RawOrigin::Signed(who).into())
 	}
@@ -352,7 +353,7 @@ impl<T: Config<I>, I: 'static, const MIN_RANK: u16> EnsureOrigin<T::RuntimeOrigi
 	#[cfg(feature = "runtime-benchmarks")]
 	fn try_successful_origin() -> Result<T::RuntimeOrigin, ()> {
 		let who = frame_benchmarking::account::<T::AccountId>("successful_origin", 0, 0);
-		crate::Pallet::<T, I>::do_add_member_to_rank(who.clone(), MIN_RANK)
+		crate::Pallet::<T, I>::do_add_member_to_rank(who.clone(), MIN_RANK, true)
 			.expect("Could not add members for benchmarks");
 		Ok(frame_system::RawOrigin::Signed(who).into())
 	}
@@ -390,6 +391,9 @@ pub mod pallet {
 		/// maximum rank *from which* the demotion/removal may be.
 		type DemoteOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = Rank>;
 
+		/// The origin that can swap the account of a member.
+		type ExchangeOrigin: EnsureOrigin<Self::RuntimeOrigin>;
+
 		/// The polling system used for our voting.
 		type Polls: Polling<TallyOf<Self, I>, Votes = Votes, Moment = BlockNumberFor<Self>>;
 
@@ -454,6 +458,8 @@ pub mod pallet {
 		/// The member `who` has voted for the `poll` with the given `vote` leading to an updated
 		/// `tally`.
 		Voted { who: T::AccountId, poll: PollIndexOf<T, I>, vote: VoteRecord, tally: TallyOf<T, I> },
+		/// The member `who` had their `AccountId` changed to `new_who`.
+		MemberExchanged { who: T::AccountId, new_who: T::AccountId },
 	}
 
 	#[pallet::error]
@@ -476,6 +482,8 @@ pub mod pallet {
 		InvalidWitness,
 		/// The origin is not sufficiently privileged to do the operation.
 		NoPermission,
+		/// The new member to exchange is the same as the old member
+		SameMember,
 	}
 
 	#[pallet::call]
@@ -492,7 +500,7 @@ pub mod pallet {
 		pub fn add_member(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
 			let _ = T::PromoteOrigin::ensure_origin(origin)?;
 			let who = T::Lookup::lookup(who)?;
-			Self::do_add_member(who)
+			Self::do_add_member(who, true)
 		}
 
 		/// Increment the rank of an existing member by one.
@@ -506,7 +514,7 @@ pub mod pallet {
 		pub fn promote_member(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
 			let max_rank = T::PromoteOrigin::ensure_origin(origin)?;
 			let who = T::Lookup::lookup(who)?;
-			Self::do_promote_member(who, Some(max_rank))
+			Self::do_promote_member(who, Some(max_rank), true)
 		}
 
 		/// Decrement the rank of an existing member by one. If the member is already at rank zero,
@@ -544,10 +552,7 @@ pub mod pallet {
 			ensure!(min_rank >= rank, Error::<T, I>::InvalidWitness);
 			ensure!(max_rank >= rank, Error::<T, I>::NoPermission);
 
-			for r in 0..=rank {
-				Self::remove_from_rank(&who, r)?;
-			}
-			Members::<T, I>::remove(&who);
+			Self::do_remove_member_from_rank(&who, rank)?;
 			Self::deposit_event(Event::MemberRemoved { who, rank });
 			Ok(PostDispatchInfo {
 				actual_weight: Some(T::WeightInfo::remove_member(rank as u32)),
@@ -650,6 +655,33 @@ pub mod pallet {
 				pays_fee: Pays::No,
 			})
 		}
+
+		/// Exchanges a member with a new account and the same existing rank.
+		///
+		/// - `origin`: Must be the `ExchangeOrigin`.
+		/// - `who`: Account of existing member of rank greater than zero to be exchanged.
+		/// - `new_who`: New Account of existing member of rank greater than zero to exchanged to.
+		#[pallet::call_index(6)]
+		#[pallet::weight(T::WeightInfo::exchange_member())]
+		pub fn exchange_member(
+			origin: OriginFor<T>,
+			who: AccountIdLookupOf<T>,
+			new_who: AccountIdLookupOf<T>,
+		) -> DispatchResult {
+			T::ExchangeOrigin::ensure_origin(origin)?;
+			let who = T::Lookup::lookup(who)?;
+			let new_who = T::Lookup::lookup(new_who)?;
+
+			ensure!(who != new_who, Error::<T, I>::SameMember);
+
+			let MemberRecord { rank, .. } = Self::ensure_member(&who)?;
+
+			Self::do_remove_member_from_rank(&who, rank)?;
+			Self::do_add_member_to_rank(new_who.clone(), rank, false)?;
+
+			Self::deposit_event(Event::MemberExchanged { who, new_who });
+			Ok(())
+		}
 	}
 
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
@@ -683,7 +715,7 @@ pub mod pallet {
 		/// Adds a member into the ranked collective at level 0.
 		///
 		/// No origin checks are executed.
-		pub fn do_add_member(who: T::AccountId) -> DispatchResult {
+		pub fn do_add_member(who: T::AccountId, emit_event: bool) -> DispatchResult {
 			ensure!(!Members::<T, I>::contains_key(&who), Error::<T, I>::AlreadyMember);
 			let index = MemberCount::<T, I>::get(0);
 			let count = index.checked_add(1).ok_or(Overflow)?;
@@ -692,7 +724,9 @@ pub mod pallet {
 			IdToIndex::<T, I>::insert(0, &who, index);
 			IndexToId::<T, I>::insert(0, index, &who);
 			MemberCount::<T, I>::insert(0, count);
-			Self::deposit_event(Event::MemberAdded { who });
+			if emit_event {
+				Self::deposit_event(Event::MemberAdded { who });
+			}
 			Ok(())
 		}
 
@@ -703,6 +737,7 @@ pub mod pallet {
 		pub fn do_promote_member(
 			who: T::AccountId,
 			maybe_max_rank: Option<Rank>,
+			emit_event: bool,
 		) -> DispatchResult {
 			let record = Self::ensure_member(&who)?;
 			let rank = record.rank.checked_add(1).ok_or(Overflow)?;
@@ -714,7 +749,9 @@ pub mod pallet {
 			IdToIndex::<T, I>::insert(rank, &who, index);
 			IndexToId::<T, I>::insert(rank, index, &who);
 			Members::<T, I>::insert(&who, MemberRecord { rank });
-			Self::deposit_event(Event::RankChanged { who, rank });
+			if emit_event {
+				Self::deposit_event(Event::RankChanged { who, rank });
+			}
 			Ok(())
 		}
 
@@ -747,10 +784,14 @@ pub mod pallet {
 
 		/// Add a member to the rank collective, and continue to promote them until a certain rank
 		/// is reached.
-		pub fn do_add_member_to_rank(who: T::AccountId, rank: Rank) -> DispatchResult {
-			Self::do_add_member(who.clone())?;
+		pub fn do_add_member_to_rank(
+			who: T::AccountId,
+			rank: Rank,
+			emit_event: bool,
+		) -> DispatchResult {
+			Self::do_add_member(who.clone(), emit_event)?;
 			for _ in 0..rank {
-				Self::do_promote_member(who.clone(), None)?;
+				Self::do_promote_member(who.clone(), None, emit_event)?;
 			}
 			Ok(())
 		}
@@ -763,6 +804,15 @@ pub mod pallet {
 			use frame_support::traits::CallerTrait;
 			o.as_signed().and_then(Self::rank_of)
 		}
+
+		/// Removes a member from the rank collective
+		pub fn do_remove_member_from_rank(who: &T::AccountId, rank: Rank) -> DispatchResult {
+			for r in 0..=rank {
+				Self::remove_from_rank(&who, r)?;
+			}
+			Members::<T, I>::remove(&who);
+			Ok(())
+		}
 	}
 
 	impl<T: Config<I>, I: 'static> RankedMembers for Pallet<T, I> {
@@ -778,11 +828,11 @@ pub mod pallet {
 		}
 
 		fn induct(who: &Self::AccountId) -> DispatchResult {
-			Self::do_add_member(who.clone())
+			Self::do_add_member(who.clone(), true)
 		}
 
 		fn promote(who: &Self::AccountId) -> DispatchResult {
-			Self::do_promote_member(who.clone(), None)
+			Self::do_promote_member(who.clone(), None, true)
 		}
 
 		fn demote(who: &Self::AccountId) -> DispatchResult {

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

@@ -164,6 +164,12 @@ impl Config for Test {
 		// 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 VoteWeight = Geometric;
@@ -516,8 +522,8 @@ fn ensure_ranked_works() {
 fn do_add_member_to_rank_works() {
 	new_test_ext().execute_with(|| {
 		let max_rank = 9u16;
-		assert_ok!(Club::do_add_member_to_rank(69, max_rank / 2));
-		assert_ok!(Club::do_add_member_to_rank(1337, max_rank));
+		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));
 		for i in 0..=max_rank {
 			if i <= max_rank / 2 {
 				assert_eq!(member_count(i), 2);
@@ -568,3 +574,31 @@ fn tally_support_correct() {
 		MinRankOfClassDelta::set(0);
 	});
 }
+
+#[test]
+fn exchange_member_works() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(Club::add_member(RuntimeOrigin::root(), 1));
+		assert_eq!(member_count(0), 1);
+
+		assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1));
+
+		let member_record = MemberRecord { rank: 1 };
+		assert_eq!(Members::<Test>::get(1), Some(member_record.clone()));
+		assert_eq!(Members::<Test>::get(2), None);
+
+		assert_ok!(Club::exchange_member(RuntimeOrigin::root(), 1, 2));
+		assert_eq!(member_count(0), 1);
+
+		assert_eq!(Members::<Test>::get(1), None);
+		assert_eq!(Members::<Test>::get(2), Some(member_record));
+
+		assert_ok!(Club::add_member(RuntimeOrigin::root(), 3));
+		assert_ok!(Club::promote_member(RuntimeOrigin::root(), 3));
+
+		assert_noop!(
+			Club::exchange_member(RuntimeOrigin::signed(3), 2, 1),
+			DispatchError::BadOrigin
+		);
+	});
+}

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

@@ -58,6 +58,7 @@ pub trait WeightInfo {
 	fn demote_member(r: u32, ) -> Weight;
 	fn vote() -> Weight;
 	fn cleanup_poll(n: u32, ) -> Weight;
+	fn exchange_member() -> Weight;
 }
 
 /// Weights for pallet_ranked_collective using the Substrate node and recommended hardware.
@@ -180,6 +181,25 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 			.saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(n.into())))
 			.saturating_add(Weight::from_parts(0, 2540).saturating_mul(n.into()))
 	}
+
+	/// Storage: `RankedCollective::Members` (r:2 w:2)
+	/// Proof: `RankedCollective::Members` (`max_values`: None, `max_size`: Some(42), added: 2517, mode: `MaxEncodedLen`)
+	/// Storage: `RankedCollective::MemberCount` (r:2 w:2)
+	/// Proof: `RankedCollective::MemberCount` (`max_values`: None, `max_size`: Some(14), added: 2489, mode: `MaxEncodedLen`)
+	/// Storage: `RankedCollective::IdToIndex` (r:2 w:2)
+	/// Proof: `RankedCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`)
+	/// Storage: `RankedCollective::IndexToId` (r:0 w:2)
+	/// Proof: `RankedCollective::IndexToId` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`)
+	fn exchange_member() -> Weight {
+		// Proof Size summary in bytes:
+		//  Measured:  `437`
+		//  Estimated: `6048`
+		// Minimum execution time: 138_000_000 picoseconds.
+		Weight::from_parts(141_000_000, 0)
+			.saturating_add(Weight::from_parts(0, 6048))
+			.saturating_add(T::DbWeight::get().reads(6))
+			.saturating_add(T::DbWeight::get().writes(8))
+	}
 }
 
 // For backwards compatibility and tests
@@ -301,4 +321,23 @@ impl WeightInfo for () {
 			.saturating_add(RocksDbWeight::get().writes((1_u64).saturating_mul(n.into())))
 			.saturating_add(Weight::from_parts(0, 2540).saturating_mul(n.into()))
 	}
+
+	/// Storage: `RankedCollective::Members` (r:2 w:2)
+	/// Proof: `RankedCollective::Members` (`max_values`: None, `max_size`: Some(42), added: 2517, mode: `MaxEncodedLen`)
+	/// Storage: `RankedCollective::MemberCount` (r:2 w:2)
+	/// Proof: `RankedCollective::MemberCount` (`max_values`: None, `max_size`: Some(14), added: 2489, mode: `MaxEncodedLen`)
+	/// Storage: `RankedCollective::IdToIndex` (r:2 w:2)
+	/// Proof: `RankedCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`)
+	/// Storage: `RankedCollective::IndexToId` (r:0 w:2)
+	/// Proof: `RankedCollective::IndexToId` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`)
+	fn exchange_member() -> Weight {
+		// Proof Size summary in bytes:
+		//  Measured:  `437`
+		//  Estimated: `6048`
+		// Minimum execution time: 138_000_000 picoseconds.
+		Weight::from_parts(141_000_000, 0)
+			.saturating_add(Weight::from_parts(0, 6048))
+			.saturating_add(RocksDbWeight::get().reads(6))
+			.saturating_add(RocksDbWeight::get().writes(8))
+	}
 }