[FRAME] Make core-fellowship and salary work for swapped members

[ggwpez]

Fixup for #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 and here to check that adding member 1 is equivalent to adding member 0 and then swapping.

Integration

The only change of config is in the ranked-collective pallet. Simply add all pallets that depend on it as RankedMembers provider:

impl pallet_ranked_collective::Config for Runtime {
+	type MemberSwappedHandler = (CoreFellowship, Salary);
+	#[cfg(feature = "runtime-benchmarks")]
+	type BenchmarkSetup = (CoreFellowship, Salary);

[kianenigma]

I am itching for a try-state check that ensures members that are kept in Config::Members are the same as the set that is kept here. Something like:

diff --git a/substrate/frame/core-fellowship/src/lib.rs b/substrate/frame/core-fellowship/src/lib.rs
index a0a45c7c59..1db600deb0 100644
--- a/substrate/frame/core-fellowship/src/lib.rs
+++ b/substrate/frame/core-fellowship/src/lib.rs
@@ -547,6 +547,17 @@ pub mod pallet {
 				Self::deposit_event(e);
 			}
 		}
+
+		/// Invariants:
+		///
+		/// - All members tracked in this pallet under [`Members`] and [`MemberEvidence`] must have
+		///   be part of [`Config::Members`].
+		#[cfg(feature = "try-runtime")]
+		fn do_try_state() {
+			let external_members = T::Members::all().collect::<Vec<_>>();
+			let internal_members = todo!();
+			// convert to set, then compare.
+		}
 	}
 
 	impl<T: Config<I>, I: 'static> GetSalary<RankOf<T, I>, T::AccountId, T::Balance> for Pallet<T, I> {
diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs
index 88631884c5..92dc44e16f 100644
--- a/substrate/frame/ranked-collective/src/lib.rs
+++ b/substrate/frame/ranked-collective/src/lib.rs
@@ -838,5 +838,10 @@ pub mod pallet {
 		fn demote(who: &Self::AccountId) -> DispatchResult {
 			Self::do_demote_member(who.clone(), None)
 		}
+
+		#[cfg(feature = "try-runtime")]
+		fn all() -> Vec<T::AccountId> {
+			Members::<T, I>::iter().map(|(who, _)| who).collect()
+		}
 	}
 }
diff --git a/substrate/frame/support/src/traits/members.rs b/substrate/frame/support/src/traits/members.rs
index d667eaa7e9..edd627257c 100644
--- a/substrate/frame/support/src/traits/members.rs
+++ b/substrate/frame/support/src/traits/members.rs
@@ -295,6 +295,9 @@ pub trait RankedMembers {
 	/// Demote a member to the next lower rank; demoting beyond the `min_rank` removes the
 	/// member entirely.
 	fn demote(who: &Self::AccountId) -> DispatchResult;
+
+	#[cfg(feature = "try-runtime")]
+	fn all() -> Vec<T::AccountId>;
 }
 
 /// Trait for type that can handle the initialization of account IDs at genesis.

[ggwpez]

Will do as follow up

[georgepisaltu]

nit: I'd rename this to something like register_as_member or similar because you're not really running checks and returning the validity, you're doing setup work.

[ggwpez]

Yea true. But will do it as follow up to not retrigger the CI now to not prevent the merge.

[kianenigma]

I think the salary pallet's modification is yet another reason to consider multi-pallet try-state checks to make sure these pallets with interdependency remain in sync.

Although, try-state would only go as far as catching the issues in production, I am not sure what we can do at the code level.

[ggwpez]

Although, try-state would only go as far as catching the issues in production, I am not sure what we can do at the code level.

Fuzzing maybe? And calling the try-state at the end of the tests.