Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Jan 31, 2024
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,10 @@ impl pallet_ranked_collective::Config<AmbassadorCollectiveInstance> for Runtime
type ExchangeOrigin = ExchangeOrigin;
type Polls = AmbassadorReferenda;
type MinRankOfClass = sp_runtime::traits::Identity;
type MemberSwappedHandler = (crate::AmbassadorCore, crate::AmbassadorSalary);
type VoteWeight = pallet_ranked_collective::Linear;
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkSetup = (crate::AmbassadorCore, crate::AmbassadorSalary);
}

parameter_types! {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,10 @@ impl pallet_ranked_collective::Config<FellowshipCollectiveInstance> for Runtime
EitherOf<EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>, Fellows>;
type Polls = FellowshipReferenda;
type MinRankOfClass = tracks::MinRankOfClass;
type MemberSwappedHandler = (crate::FellowshipCore, crate::FellowshipSalary);
type VoteWeight = pallet_ranked_collective::Geometric;
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkSetup = (crate::FellowshipCore, crate::FellowshipSalary);
}

pub type FellowshipCoreInstance = pallet_core_fellowship::Instance1;
Expand Down
3 changes: 3 additions & 0 deletions polkadot/runtime/rococo/src/governance/fellowship.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,5 +344,8 @@ impl pallet_ranked_collective::Config<FellowshipCollectiveInstance> for Runtime
EitherOf<EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>, Fellows>;
type Polls = FellowshipReferenda;
type MinRankOfClass = sp_runtime::traits::Identity;
type MemberSwappedHandler = ();
type VoteWeight = pallet_ranked_collective::Geometric;
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkSetup = ();
}
13 changes: 13 additions & 0 deletions prdoc/pr_3156.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
title: "Adapt core-fellowship and salary pallets for swapped members"

doc:
- audience: Runtime Dev
description: |
The ranked-collective pallet got the ability to swap members but the core-fellowship and
salary pallets would not be notified of this change. This MR adds `MemberSwappedHandler` to
the collective that is implemented by both pallets.

crates:
- name: "pallet-ranked-collective"
- name: "pallet-core-fellowship"
- name: "pallet-salary"
3 changes: 3 additions & 0 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1019,6 +1019,9 @@ impl pallet_ranked_collective::Config for Runtime {
type Polls = RankedPolls;
type MinRankOfClass = traits::Identity;
type VoteWeight = pallet_ranked_collective::Geometric;
type MemberSwappedHandler = (CoreFellowship, Salary);
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkSetup = (CoreFellowship, Salary);
}

impl pallet_remark::Config for Runtime {
Expand Down
5 changes: 5 additions & 0 deletions substrate/frame/core-fellowship/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,18 @@ sp-core = { path = "../../primitives/core", default-features = false }
sp-io = { path = "../../primitives/io", default-features = false }
sp-runtime = { path = "../../primitives/runtime", default-features = false }
sp-std = { path = "../../primitives/std", default-features = false }
pallet-ranked-collective = { path = "../ranked-collective", default-features = false, optional = true }

[features]
default = ["std"]
std = [
"codec/std",
"frame-benchmarking?/std",
"frame-support/experimental",
"frame-support/std",
"frame-system/std",
"log/std",
"pallet-ranked-collective/std",
"scale-info/std",
"sp-arithmetic/std",
"sp-core/std",
Expand All @@ -47,10 +50,12 @@ runtime-benchmarks = [
"frame-benchmarking/runtime-benchmarks",
"frame-support/runtime-benchmarks",
"frame-system/runtime-benchmarks",
"pallet-ranked-collective/runtime-benchmarks",
"sp-runtime/runtime-benchmarks",
]
try-runtime = [
"frame-support/try-runtime",
"frame-system/try-runtime",
"pallet-ranked-collective?/try-runtime",
"sp-runtime/try-runtime",
]
4 changes: 2 additions & 2 deletions substrate/frame/core-fellowship/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}
39 changes: 39 additions & 0 deletions substrate/frame/core-fellowship/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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() });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do as follow up

}
}

#[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();
}
}
Loading
Loading