Adds MaxRank Config in pallet-core-fellowship (paritytech#3393)

resolves paritytech#3315

---------

Co-authored-by: doordashcon <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <[email protected]>

cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs

@@ -220,6 +220,7 @@ impl pallet_core_fellowship::Config<AmbassadorCoreInstance> for Runtime {
 	type ApproveOrigin = PromoteOrigin;
 	type PromoteOrigin = PromoteOrigin;
 	type EvidenceSize = ConstU32<65536>;
+	type MaxRank = ConstU32<9>;
 }
 
 pub type AmbassadorSalaryInstance = pallet_salary::Instance2;

cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs

@@ -210,6 +210,7 @@ impl pallet_core_fellowship::Config<FellowshipCoreInstance> for Runtime {
 		EnsureCanPromoteTo,
 	>;
 	type EvidenceSize = ConstU32<65536>;
+	type MaxRank = ConstU32<9>;
 }
 
 pub type FellowshipSalaryInstance = pallet_salary::Instance1;

cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs

@@ -44,8 +44,9 @@ pub mod xcm_config;
 pub mod fellowship;
 pub use ambassador::pallet_ambassador_origins;
 
+use ambassador::AmbassadorCoreInstance;
 use cumulus_pallet_parachain_system::RelayNumberMonotonicallyIncreases;
-use fellowship::{pallet_fellowship_origins, Fellows};
+use fellowship::{pallet_fellowship_origins, Fellows, FellowshipCoreInstance};
 use impls::{AllianceProposalProvider, EqualOrGreatestRootCmp};
 use sp_api::impl_runtime_apis;
 use sp_core::{crypto::KeyTypeId, OpaqueMetadata};
@@ -739,6 +740,10 @@ type Migrations = (
 	cumulus_pallet_xcmp_queue::migration::v5::MigrateV4ToV5<Runtime>,
 	// permanent
 	pallet_xcm::migration::MigrateToLatestXcmVersion<Runtime>,
+	// unreleased
+	pallet_core_fellowship::migration::MigrateV0ToV1<Runtime, FellowshipCoreInstance>,
+	// unreleased
+	pallet_core_fellowship::migration::MigrateV0ToV1<Runtime, AmbassadorCoreInstance>,
 );
 
 /// Executive: handles dispatch to the various modules.

prdoc/pr_3393.prdoc

@@ -0,0 +1,12 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Add `MaxRank` Config to `pallet-core-fellowship` 
+
+doc:
+  - audience: Runtime User
+    description: |
+      This PR adds a new Config `MaxRank` to the core fellowship pallet. Initially, the maximum rank was set to IX (Grand Master) on the core-fellowship pallet, corresponding to the establishment of the Technical Fellowship and setting the default member count to nine. However, with the introduction of new collectives, this maximum rank is expected to evolve.
+
+crates:
+  - name: pallet-core-fellowship

substrate/bin/node/runtime/src/lib.rs

@@ -1870,6 +1870,7 @@ impl pallet_core_fellowship::Config for Runtime {
 	type ApproveOrigin = EnsureRootWithSuccess<AccountId, ConstU16<9>>;
 	type PromoteOrigin = EnsureRootWithSuccess<AccountId, ConstU16<9>>;
 	type EvidenceSize = ConstU32<16_384>;
+	type MaxRank = ConstU32<9>;
 }
 
 parameter_types! {

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

@@ -54,11 +54,12 @@ mod benchmarks {
 	}
 
 	fn set_benchmark_params<T: Config<I>, I: 'static>() -> Result<(), BenchmarkError> {
+		let max_rank = T::MaxRank::get().try_into().unwrap();
 		let params = ParamsType {
-			active_salary: [100u32.into(); 9],
-			passive_salary: [10u32.into(); 9],
-			demotion_period: [100u32.into(); 9],
-			min_promotion_period: [100u32.into(); 9],
+			active_salary: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(),
+			passive_salary: BoundedVec::try_from(vec![10u32.into(); max_rank]).unwrap(),
+			demotion_period: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(),
+			min_promotion_period: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(),
 			offboard_timeout: 1u32.into(),
 		};
 
@@ -68,11 +69,12 @@ mod benchmarks {
 
 	#[benchmark]
 	fn set_params() -> Result<(), BenchmarkError> {
+		let max_rank = T::MaxRank::get().try_into().unwrap();
 		let params = ParamsType {
-			active_salary: [100u32.into(); 9],
-			passive_salary: [10u32.into(); 9],
-			demotion_period: [100u32.into(); 9],
-			min_promotion_period: [100u32.into(); 9],
+			active_salary: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(),
+			passive_salary: BoundedVec::try_from(vec![10u32.into(); max_rank]).unwrap(),
+			demotion_period: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(),
+			min_promotion_period: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(),
 			offboard_timeout: 1u32.into(),
 		};
 
@@ -151,10 +153,14 @@ mod benchmarks {
 	fn promote() -> Result<(), BenchmarkError> {
 		// Ensure that the `min_promotion_period` wont get in our way.
 		let mut params = Params::<T, I>::get();
-		params.min_promotion_period = [Zero::zero(); RANK_COUNT];
+		let max_rank = T::MaxRank::get().try_into().unwrap();
+		params.min_promotion_period = BoundedVec::try_from(vec![Zero::zero(); max_rank]).unwrap();
 		Params::<T, I>::put(&params);
 
 		let member = make_member::<T, I>(1)?;
+
+		// Set it to the max value to ensure that any possible auto-demotion period has passed.
+		frame_system::Pallet::<T>::set_block_number(BlockNumberFor::<T>::max_value());
 		ensure_evidence::<T, I>(&member)?;
 
 		#[extrinsic_call]

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

@@ -61,7 +61,7 @@ use codec::{Decode, Encode, MaxEncodedLen};
 use scale_info::TypeInfo;
 use sp_arithmetic::traits::{Saturating, Zero};
 use sp_runtime::RuntimeDebug;
-use sp_std::{marker::PhantomData, prelude::*};
+use sp_std::{fmt::Debug, marker::PhantomData, prelude::*};
 
 use frame_support::{
 	defensive,
@@ -71,18 +71,19 @@ use frame_support::{
 		tokens::Balance as BalanceTrait, EnsureOrigin, EnsureOriginWithArg, Get, RankedMembers,
 		RankedMembersSwapHandler,
 	},
-	BoundedVec,
+	BoundedVec, CloneNoBound, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound,
 };
 
 #[cfg(test)]
 mod tests;
 
 #[cfg(feature = "runtime-benchmarks")]
 mod benchmarking;
+pub mod migration;
 pub mod weights;
 
 pub use pallet::*;
-pub use weights::WeightInfo;
+pub use weights::*;
 
 /// The desired outcome for which evidence is presented.
 #[derive(Encode, Decode, Eq, PartialEq, Copy, Clone, TypeInfo, MaxEncodedLen, RuntimeDebug)]
@@ -100,29 +101,46 @@ pub enum Wish {
 pub type Evidence<T, I> = BoundedVec<u8, <T as Config<I>>::EvidenceSize>;
 
 /// The status of the pallet instance.
-#[derive(Encode, Decode, Eq, PartialEq, Clone, TypeInfo, MaxEncodedLen, RuntimeDebug)]
-pub struct ParamsType<Balance, BlockNumber, const RANKS: usize> {
+#[derive(
+	Encode,
+	Decode,
+	CloneNoBound,
+	EqNoBound,
+	PartialEqNoBound,
+	RuntimeDebugNoBound,
+	TypeInfo,
+	MaxEncodedLen,
+)]
+#[scale_info(skip_type_params(Ranks))]
+pub struct ParamsType<
+	Balance: Clone + Eq + PartialEq + Debug,
+	BlockNumber: Clone + Eq + PartialEq + Debug,
+	Ranks: Get<u32>,
+> {
 	/// The amounts to be paid when a member of a given rank (-1) is active.
-	active_salary: [Balance; RANKS],
+	pub active_salary: BoundedVec<Balance, Ranks>,
 	/// The amounts to be paid when a member of a given rank (-1) is passive.
-	passive_salary: [Balance; RANKS],
+	pub passive_salary: BoundedVec<Balance, Ranks>,
 	/// The period between which unproven members become demoted.
-	demotion_period: [BlockNumber; RANKS],
+	pub demotion_period: BoundedVec<BlockNumber, Ranks>,
 	/// The period between which members must wait before they may proceed to this rank.
-	min_promotion_period: [BlockNumber; RANKS],
+	pub min_promotion_period: BoundedVec<BlockNumber, Ranks>,
 	/// Amount by which an account can remain at rank 0 (candidate before being offboard entirely).
-	offboard_timeout: BlockNumber,
+	pub offboard_timeout: BlockNumber,
 }
 
-impl<Balance: Default + Copy, BlockNumber: Default + Copy, const RANKS: usize> Default
-	for ParamsType<Balance, BlockNumber, RANKS>
+impl<
+		Balance: Default + Copy + Eq + Debug,
+		BlockNumber: Default + Copy + Eq + Debug,
+		Ranks: Get<u32>,
+	> Default for ParamsType<Balance, BlockNumber, Ranks>
 {
 	fn default() -> Self {
 		Self {
-			active_salary: [Balance::default(); RANKS],
-			passive_salary: [Balance::default(); RANKS],
-			demotion_period: [BlockNumber::default(); RANKS],
-			min_promotion_period: [BlockNumber::default(); RANKS],
+			active_salary: Default::default(),
+			passive_salary: Default::default(),
+			demotion_period: Default::default(),
+			min_promotion_period: Default::default(),
 			offboard_timeout: BlockNumber::default(),
 		}
 	}
@@ -148,11 +166,11 @@ pub mod pallet {
 		traits::{tokens::GetSalary, EnsureOrigin},
 	};
 	use frame_system::{ensure_root, pallet_prelude::*};
-
-	/// Number of available ranks.
-	pub(crate) const RANK_COUNT: usize = 9;
+	/// The in-code storage version.
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
 
 	#[pallet::pallet]
+	#[pallet::storage_version(STORAGE_VERSION)]
 	pub struct Pallet<T, I = ()>(PhantomData<(T, I)>);
 
 	#[pallet::config]
@@ -194,9 +212,16 @@ pub mod pallet {
 		/// The maximum size in bytes submitted evidence is allowed to be.
 		#[pallet::constant]
 		type EvidenceSize: Get<u32>;
+
+		/// Represents the highest possible rank in this pallet.
+		///
+		/// Increasing this value is supported, but decreasing it may lead to a broken state.
+		#[pallet::constant]
+		type MaxRank: Get<u32>;
 	}
 
-	pub type ParamsOf<T, I> = ParamsType<<T as Config<I>>::Balance, BlockNumberFor<T>, RANK_COUNT>;
+	pub type ParamsOf<T, I> =
+		ParamsType<<T as Config<I>>::Balance, BlockNumberFor<T>, <T as Config<I>>::MaxRank>;
 	pub type MemberStatusOf<T> = MemberStatus<BlockNumberFor<T>>;
 	pub type RankOf<T, I> = <<T as Config<I>>::Members as RankedMembers>::Rank;
 
@@ -338,8 +363,10 @@ pub mod pallet {
 		#[pallet::call_index(1)]
 		pub fn set_params(origin: OriginFor<T>, params: Box<ParamsOf<T, I>>) -> DispatchResult {
 			T::ParamsOrigin::ensure_origin_or_root(origin)?;
+
 			Params::<T, I>::put(params.as_ref());
 			Self::deposit_event(Event::<T, I>::ParamsChanged { params: *params });
+
 			Ok(())
 		}
 
@@ -540,7 +567,7 @@ pub mod pallet {
 		/// in the range `1..=RANK_COUNT` is `None`.
 		pub(crate) fn rank_to_index(rank: RankOf<T, I>) -> Option<usize> {
 			match TryInto::<usize>::try_into(rank) {
-				Ok(r) if r <= RANK_COUNT && r > 0 => Some(r - 1),
+				Ok(r) if r as u32 <= <T as Config<I>>::MaxRank::get() && r > 0 => Some(r - 1),
 				_ => return None,
 			}
 		}

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

@@ -0,0 +1,111 @@
+// 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.
+
+//! Storage migrations for the core-fellowship pallet.
+use super::*;
+use frame_support::{
+	pallet_prelude::*,
+	storage_alias,
+	traits::{DefensiveTruncateFrom, UncheckedOnRuntimeUpgrade},
+	BoundedVec,
+};
+
+#[cfg(feature = "try-runtime")]
+use sp_runtime::TryRuntimeError;
+
+mod v0 {
+	use frame_system::pallet_prelude::BlockNumberFor;
+
+	use super::*;
+
+	#[derive(Encode, Decode, Eq, PartialEq, Clone, TypeInfo, MaxEncodedLen, RuntimeDebug)]
+	pub struct ParamsType<Balance, BlockNumber, const RANKS: usize> {
+		pub active_salary: [Balance; RANKS],
+		pub passive_salary: [Balance; RANKS],
+		pub demotion_period: [BlockNumber; RANKS],
+		pub min_promotion_period: [BlockNumber; RANKS],
+		pub offboard_timeout: BlockNumber,
+	}
+
+	impl<Balance: Default + Copy, BlockNumber: Default + Copy, const RANKS: usize> Default
+		for ParamsType<Balance, BlockNumber, RANKS>
+	{
+		fn default() -> Self {
+			Self {
+				active_salary: [Balance::default(); RANKS],
+				passive_salary: [Balance::default(); RANKS],
+				demotion_period: [BlockNumber::default(); RANKS],
+				min_promotion_period: [BlockNumber::default(); RANKS],
+				offboard_timeout: BlockNumber::default(),
+			}
+		}
+	}
+
+	/// Number of available ranks from old version.
+	pub(crate) const RANK_COUNT: usize = 9;
+
+	pub type ParamsOf<T, I> = ParamsType<<T as Config<I>>::Balance, BlockNumberFor<T>, RANK_COUNT>;
+
+	/// V0 type for [`crate::Params`].
+	#[storage_alias]
+	pub type Params<T: Config<I>, I: 'static> =
+		StorageValue<Pallet<T, I>, ParamsOf<T, I>, ValueQuery>;
+}
+
+pub struct MigrateToV1<T, I = ()>(PhantomData<(T, I)>);
+impl<T: Config<I>, I: 'static> UncheckedOnRuntimeUpgrade for MigrateToV1<T, I> {
+	#[cfg(feature = "try-runtime")]
+	fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
+		ensure!(
+			T::MaxRank::get() >= v0::RANK_COUNT as u32,
+			"pallet-core-fellowship: new bound should not truncate"
+		);
+		Ok(Default::default())
+	}
+
+	fn on_runtime_upgrade() -> frame_support::weights::Weight {
+		// Read the old value from storage
+		let old_value = v0::Params::<T, I>::take();
+		// Write the new value to storage
+		let new = crate::ParamsType {
+			active_salary: BoundedVec::defensive_truncate_from(old_value.active_salary.to_vec()),
+			passive_salary: BoundedVec::defensive_truncate_from(old_value.passive_salary.to_vec()),
+			demotion_period: BoundedVec::defensive_truncate_from(
+				old_value.demotion_period.to_vec(),
+			),
+			min_promotion_period: BoundedVec::defensive_truncate_from(
+				old_value.min_promotion_period.to_vec(),
+			),
+			offboard_timeout: old_value.offboard_timeout,
+		};
+		crate::Params::<T, I>::put(new);
+		T::DbWeight::get().reads_writes(1, 1)
+	}
+}
+
+/// [`UncheckedOnRuntimeUpgrade`] implementation [`MigrateToV1`] wrapped in a
+/// [`VersionedMigration`](frame_support::migrations::VersionedMigration), which ensures that:
+/// - The migration only runs once when the on-chain storage version is 0
+/// - The on-chain storage version is updated to `1` after the migration executes
+/// - Reads/Writes from checking/settings the on-chain storage version are accounted for
+pub type MigrateV0ToV1<T, I> = frame_support::migrations::VersionedMigration<
+	0, // The migration will only execute when the on-chain storage version is 0
+	1, // The on-chain storage version will be set to 1 after the migration is complete
+	MigrateToV1<T, I>,
+	crate::pallet::Pallet<T, I>,
+	<T as frame_system::Config>::DbWeight,
+>;