From f9fdee10ef4b37dd820406491bb526d3c470e823 Mon Sep 17 00:00:00 2001 From: Hector Bulgarini Date: Thu, 8 Sep 2022 17:57:42 +0200 Subject: [PATCH] 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 <> --- frame/scored-pool/src/lib.rs | 62 +++++++++++++++++++++++----------- frame/scored-pool/src/mock.rs | 39 ++++++++++----------- frame/scored-pool/src/tests.rs | 17 ++++++++++ 3 files changed, 80 insertions(+), 38 deletions(-) diff --git a/frame/scored-pool/src/lib.rs b/frame/scored-pool/src/lib.rs index dd96a5df2baf9..8d4c4715096b7 100644 --- a/frame/scored-pool/src/lib.rs +++ b/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 = <>::Currency as Currency<::AccountId>>::Balance; -type PoolT = Vec<(::AccountId, Option<>::Score>)>; +type PoolT = BoundedVec< + (::AccountId, Option<>::Score>), + >::MaximumMembers, +>; +type MembersT = + BoundedVec<::AccountId, >::MaximumMembers>; type AccountIdLookupOf = <::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(_); #[pallet::config] @@ -138,6 +143,10 @@ pub mod pallet { /// The currency used for deposits. type Currency: Currency + ReservableCurrency; + /// Maximum members length allowed. + #[pallet::constant] + type MaximumMembers: Get; + /// 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> + IsType<::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, I: 'static = ()> = - StorageValue<_, Vec, ValueQuery>; + StorageValue<_, MembersT, 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())); - - >::put(self.member_count); + >::update_member_count(self.member_count) + .expect("Number of allowed members exceeded"); >::put(&pool); >::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. - >::append((who.clone(), Option::<>::Score>::None)); + >::try_append((who.clone(), Option::<>::Score>::None)) + .map_err(|_| Error::::TooManyMembers)?; >::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::::TooManyMembers)?; >::put(&pool); Self::deposit_event(Event::::CandidateScored); @@ -410,8 +423,7 @@ pub mod pallet { #[pallet::weight(0)] pub fn change_member_count(origin: OriginFor, count: u32) -> DispatchResult { ensure_root(origin)?; - MemberCount::::put(&count); - Ok(()) + Self::update_member_count(count).map_err(Into::into) } } } @@ -424,23 +436,28 @@ impl, I: 'static> Pallet { /// type function to invoke at the end of the method. fn refresh_members(pool: PoolT, notify: ChangeReceiver) { let count = MemberCount::::get(); + let old_members = >::get(); - let mut new_members: Vec = pool + let new_members: Vec = pool .into_iter() .filter(|(_, score)| score.is_some()) .take(count as usize) .map(|(account_id, _)| account_id) .collect(); - new_members.sort(); - let old_members = >::get(); - >::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 = BoundedVec::truncate_from(new_members); + + new_members_bounded.sort(); + + >::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, I: 'static> Pallet { Ok(()) } + + /// Make sure the new member count value does not exceed the MaximumMembers + fn update_member_count(new_member_count: u32) -> Result<(), Error> { + ensure!(new_member_count <= T::MaximumMembers::get(), Error::::TooManyMembers); + >::put(new_member_count); + Ok(()) + } } diff --git a/frame/scored-pool/src/mock.rs b/frame/scored-pool/src/mock.rs index 00c66c3bdefe8..74f084673e4c2 100644 --- a/frame/scored-pool/src/mock.rs +++ b/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; type Block = frame_system::mocking::MockBlock; -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 = vec![]; + pub static MembersTestValue: BoundedVec> = bounded_vec![0,10]; } pub struct TestChangeMembers; impl ChangeMembers 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 for TestChangeMembers { assert_eq!(old_plus_incoming, new_plus_outgoing); - MembersTestValue::mutate(|m| *m = new.to_vec()); + MembersTestValue::set(>>::truncate_from(new.to_vec())); } } impl InitializeMembers for TestChangeMembers { fn initialize_members(new_members: &[u64]) { - MembersTestValue::mutate(|m| *m = new_members.to_vec()); + MembersTestValue::set(>>::truncate_from( + new_members.to_vec(), + )); } } @@ -132,25 +134,24 @@ impl Config for Test { type Period = ConstU64<4>; type Score = u64; type ScoreOrigin = EnsureSignedBy; + type MaximumMembers = ConstU32<10>; } pub fn new_test_ext() -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); - pallet_balances::GenesisConfig:: { - 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:: { balances } + .assimilate_storage(&mut t) + .unwrap(); pallet_scored_pool::GenesisConfig:: { - 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) diff --git a/frame/scored-pool/src/tests.rs b/frame/scored-pool/src/tests.rs index 42f0b47e8b940..1a68891be0f50 100644 --- a/frame/scored-pool/src/tests.rs +++ b/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::::TooManyMembers + ); + }); +}