Removing without_storage_info from scored-pool pallet. (#11996)

* Removing without_storage_info from scored-pool pallet.

* Addressing PR feedback

* typo

* typo

* Addressing PR comments and formatting code

* Removing unwanted import

* Adding a map_err

* cargo fmt

Co-authored-by: parity-processbot <>

substrate/frame/scored-pool/src/lib.rs

@@ -98,10 +98,11 @@ mod mock;
 #[cfg(test)]
 mod tests;
 
-use codec::FullCodec;
+use codec::{FullCodec, MaxEncodedLen};
 use frame_support::{
 	ensure,
 	traits::{ChangeMembers, Currency, Get, InitializeMembers, ReservableCurrency},
+	BoundedVec,
 };
 pub use pallet::*;
 use sp_runtime::traits::{AtLeast32Bit, StaticLookup, Zero};
@@ -109,7 +110,12 @@ use sp_std::{fmt::Debug, prelude::*};
 
 type BalanceOf<T, I> =
 	<<T as Config<I>>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
-type PoolT<T, I> = Vec<(<T as frame_system::Config>::AccountId, Option<<T as Config<I>>::Score>)>;
+type PoolT<T, I> = BoundedVec<
+	(<T as frame_system::Config>::AccountId, Option<<T as Config<I>>::Score>),
+	<T as Config<I>>::MaximumMembers,
+>;
+type MembersT<T, I> =
+	BoundedVec<<T as frame_system::Config>::AccountId, <T as Config<I>>::MaximumMembers>;
 type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;
 
 /// The enum is supplied when refreshing the members set.
@@ -130,7 +136,6 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
-	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(_);
 
 	#[pallet::config]
@@ -138,6 +143,10 @@ pub mod pallet {
 		/// The currency used for deposits.
 		type Currency: Currency<Self::AccountId> + ReservableCurrency<Self::AccountId>;
 
+		/// Maximum members length allowed.
+		#[pallet::constant]
+		type MaximumMembers: Get<u32>;
+
 		/// The score attributed to a member or candidate.
 		type Score: AtLeast32Bit
 			+ Clone
@@ -146,7 +155,8 @@ pub mod pallet {
 			+ FullCodec
 			+ MaybeSerializeDeserialize
 			+ Debug
-			+ scale_info::TypeInfo;
+			+ scale_info::TypeInfo
+			+ MaxEncodedLen;
 
 		/// The overarching event type.
 		type Event: From<Event<Self, I>> + IsType<<Self as frame_system::Config>::Event>;
@@ -207,9 +217,11 @@ pub mod pallet {
 		InvalidIndex,
 		/// Index does not match requested account.
 		WrongAccountIndex,
+		/// Number of members exceeds `MaximumMembers`.
+		TooManyMembers,
 	}
 
-	/// The current pool of candidates, stored as an ordered Vec
+	/// The current pool of candidates, stored as an ordered Bounded Vec
 	/// (ordered descending by score, `None` last, highest first).
 	#[pallet::storage]
 	#[pallet::getter(fn pool)]
@@ -229,7 +241,7 @@ pub mod pallet {
 	#[pallet::storage]
 	#[pallet::getter(fn members)]
 	pub(crate) type Members<T: Config<I>, I: 'static = ()> =
-		StorageValue<_, Vec<T::AccountId>, ValueQuery>;
+		StorageValue<_, MembersT<T, I>, ValueQuery>;
 
 	/// Size of the `Members` set.
 	#[pallet::storage]
@@ -263,10 +275,10 @@ pub mod pallet {
 			});
 
 			// Sorts the `Pool` by score in a descending order. Entities which
-			// have a score of `None` are sorted to the beginning of the vec.
+			// have a score of `None` are sorted to the end of the bounded vec.
 			pool.sort_by_key(|(_, maybe_score)| Reverse(maybe_score.unwrap_or_default()));
-
-			<MemberCount<T, I>>::put(self.member_count);
+			<Pallet<T, I>>::update_member_count(self.member_count)
+				.expect("Number of allowed members exceeded");
 			<Pool<T, I>>::put(&pool);
 			<Pallet<T, I>>::refresh_members(pool, ChangeReceiver::MembershipInitialized);
 		}
@@ -308,7 +320,8 @@ pub mod pallet {
 
 			// can be inserted as last element in pool, since entities with
 			// `None` are always sorted to the end.
-			<Pool<T, I>>::append((who.clone(), Option::<<T as Config<I>>::Score>::None));
+			<Pool<T, I>>::try_append((who.clone(), Option::<<T as Config<I>>::Score>::None))
+				.map_err(|_| Error::<T, I>::TooManyMembers)?;
 
 			<CandidateExists<T, I>>::insert(&who, true);
 
@@ -394,7 +407,7 @@ pub mod pallet {
 					Reverse(maybe_score.unwrap_or_default())
 				})
 				.unwrap_or_else(|l| l);
-			pool.insert(location, item);
+			pool.try_insert(location, item).map_err(|_| Error::<T, I>::TooManyMembers)?;
 
 			<Pool<T, I>>::put(&pool);
 			Self::deposit_event(Event::<T, I>::CandidateScored);
@@ -410,8 +423,7 @@ pub mod pallet {
 		#[pallet::weight(0)]
 		pub fn change_member_count(origin: OriginFor<T>, count: u32) -> DispatchResult {
 			ensure_root(origin)?;
-			MemberCount::<T, I>::put(&count);
-			Ok(())
+			Self::update_member_count(count).map_err(Into::into)
 		}
 	}
 }
@@ -424,23 +436,28 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// type function to invoke at the end of the method.
 	fn refresh_members(pool: PoolT<T, I>, notify: ChangeReceiver) {
 		let count = MemberCount::<T, I>::get();
+		let old_members = <Members<T, I>>::get();
 
-		let mut new_members: Vec<T::AccountId> = pool
+		let new_members: Vec<T::AccountId> = pool
 			.into_iter()
 			.filter(|(_, score)| score.is_some())
 			.take(count as usize)
 			.map(|(account_id, _)| account_id)
 			.collect();
-		new_members.sort();
 
-		let old_members = <Members<T, I>>::get();
-		<Members<T, I>>::put(&new_members);
+		// It's safe to truncate_from at this point since MemberCount
+		// is verified that it does not exceed the MaximumMembers value
+		let mut new_members_bounded: MembersT<T, I> = BoundedVec::truncate_from(new_members);
+
+		new_members_bounded.sort();
+
+		<Members<T, I>>::put(&new_members_bounded);
 
 		match notify {
 			ChangeReceiver::MembershipInitialized =>
-				T::MembershipInitialized::initialize_members(&new_members),
+				T::MembershipInitialized::initialize_members(&new_members_bounded),
 			ChangeReceiver::MembershipChanged =>
-				T::MembershipChanged::set_members_sorted(&new_members[..], &old_members[..]),
+				T::MembershipChanged::set_members_sorted(&new_members_bounded[..], &old_members[..]),
 		}
 	}
 
@@ -486,4 +503,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		Ok(())
 	}
+
+	/// Make sure the new member count value does not exceed the MaximumMembers
+	fn update_member_count(new_member_count: u32) -> Result<(), Error<T, I>> {
+		ensure!(new_member_count <= T::MaximumMembers::get(), Error::<T, I>::TooManyMembers);
+		<MemberCount<T, I>>::put(new_member_count);
+		Ok(())
+	}
 }

substrate/frame/scored-pool/src/mock.rs

@@ -21,7 +21,7 @@ use super::*;
 use crate as pallet_scored_pool;
 
 use frame_support::{
-	ord_parameter_types, parameter_types,
+	bounded_vec, construct_runtime, ord_parameter_types, parameter_types,
 	traits::{ConstU32, ConstU64, GenesisBuild},
 };
 use frame_system::EnsureSignedBy;
@@ -34,7 +34,7 @@ use sp_runtime::{
 type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
 type Block = frame_system::mocking::MockBlock<Test>;
 
-frame_support::construct_runtime!(
+construct_runtime!(
 	pub enum Test where
 		Block = Block,
 		NodeBlock = Block,
@@ -96,13 +96,13 @@ impl pallet_balances::Config for Test {
 }
 
 parameter_types! {
-	pub static MembersTestValue: Vec<u64> = vec![];
+	pub static MembersTestValue: BoundedVec<u64,ConstU32<10_u32>> = bounded_vec![0,10];
 }
 
 pub struct TestChangeMembers;
 impl ChangeMembers<u64> for TestChangeMembers {
 	fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) {
-		let mut old_plus_incoming = MembersTestValue::get().to_vec();
+		let mut old_plus_incoming = MembersTestValue::get().into_inner();
 		old_plus_incoming.extend_from_slice(incoming);
 		old_plus_incoming.sort();
 
@@ -112,13 +112,15 @@ impl ChangeMembers<u64> for TestChangeMembers {
 
 		assert_eq!(old_plus_incoming, new_plus_outgoing);
 
-		MembersTestValue::mutate(|m| *m = new.to_vec());
+		MembersTestValue::set(<BoundedVec<u64, ConstU32<10_u32>>>::truncate_from(new.to_vec()));
 	}
 }
 
 impl InitializeMembers<u64> for TestChangeMembers {
 	fn initialize_members(new_members: &[u64]) {
-		MembersTestValue::mutate(|m| *m = new_members.to_vec());
+		MembersTestValue::set(<BoundedVec<u64, ConstU32<10_u32>>>::truncate_from(
+			new_members.to_vec(),
+		));
 	}
 }
 
@@ -132,25 +134,24 @@ impl Config for Test {
 	type Period = ConstU64<4>;
 	type Score = u64;
 	type ScoreOrigin = EnsureSignedBy<ScoreOrigin, u64>;
+	type MaximumMembers = ConstU32<10>;
 }
 
 pub fn new_test_ext() -> sp_io::TestExternalities {
 	let mut t = frame_system::GenesisConfig::default().build_storage::<Test>().unwrap();
-	pallet_balances::GenesisConfig::<Test> {
-		balances: vec![
-			(5, 500_000),
-			(10, 500_000),
-			(15, 500_000),
-			(20, 500_000),
-			(31, 500_000),
-			(40, 500_000),
-			(99, 1),
-		],
+	let mut balances = vec![];
+	for i in 1..31 {
+		balances.push((i, 500_000));
 	}
-	.assimilate_storage(&mut t)
-	.unwrap();
+	balances.push((31, 500_000));
+	balances.push((40, 500_000));
+	balances.push((99, 1));
+
+	pallet_balances::GenesisConfig::<Test> { balances }
+		.assimilate_storage(&mut t)
+		.unwrap();
 	pallet_scored_pool::GenesisConfig::<Test> {
-		pool: vec![(5, None), (10, Some(1)), (20, Some(2)), (31, Some(2)), (40, Some(3))],
+		pool: bounded_vec![(10, Some(1)), (20, Some(2)), (31, Some(2)), (40, Some(3)), (5, None)],
 		member_count: 2,
 	}
 	.assimilate_storage(&mut t)

substrate/frame/scored-pool/src/tests.rs

@@ -297,3 +297,20 @@ fn candidacy_resubmitting_works() {
 		assert_eq!(ScoredPool::candidate_exists(who), true);
 	});
 }
+
+#[test]
+fn pool_candidates_exceeded() {
+	new_test_ext().execute_with(|| {
+		for i in [1, 2, 3, 4, 6] {
+			let who = i as u64;
+			assert_ok!(ScoredPool::submit_candidacy(Origin::signed(who)));
+			let index = find_in_pool(who).expect("entity must be in pool") as u32;
+			assert_ok!(ScoredPool::score(Origin::signed(ScoreOrigin::get()), who, index, 99));
+		}
+
+		assert_noop!(
+			ScoredPool::submit_candidacy(Origin::signed(8)),
+			Error::<Test, _>::TooManyMembers
+		);
+	});
+}