From 2ed807c1815617ecabdf7e385b770210dcf97871 Mon Sep 17 00:00:00 2001 From: Hector Bulgarini Date: Mon, 25 Jul 2022 20:34:59 +0100 Subject: [PATCH 1/8] Removing without_storage_info from scored-pool pallet. --- frame/scored-pool/src/lib.rs | 53 ++++++++++++++++++++++------------ frame/scored-pool/src/mock.rs | 42 +++++++++++++++------------ frame/scored-pool/src/tests.rs | 22 +++++++++++++- 3 files changed, 80 insertions(+), 37 deletions(-) diff --git a/frame/scored-pool/src/lib.rs b/frame/scored-pool/src/lib.rs index abdb9b2acc9b5..89f96f17c481f 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>; /// The enum is supplied when refreshing the members set. /// Depending on the enum variant the corresponding associated @@ -129,7 +135,6 @@ pub mod pallet { #[pallet::pallet] #[pallet::generate_store(pub(super) trait Store)] - #[pallet::without_storage_info] pub struct Pallet(_); #[pallet::config] @@ -137,6 +142,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 @@ -145,7 +154,8 @@ pub mod pallet { + FullCodec + MaybeSerializeDeserialize + Debug - + scale_info::TypeInfo; + + scale_info::TypeInfo + + MaxEncodedLen; /// The overarching event type. type Event: From> + IsType<::Event>; @@ -206,9 +216,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)] @@ -228,7 +240,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] @@ -251,7 +263,7 @@ pub mod pallet { #[pallet::genesis_build] impl, I: 'static> GenesisBuild for GenesisConfig { fn build(&self) { - let mut pool = self.pool.clone(); + let mut pool = self.pool.clone().into_inner(); // reserve balance for each candidate in the pool. // panicking here is ok, since this just happens one time, pre-genesis. @@ -264,10 +276,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. pool.sort_by_key(|(_, maybe_score)| Reverse(maybe_score.unwrap_or_default())); - + let pool_bounded: PoolT = BoundedVec::truncate_from(pool); >::put(self.member_count); - >::put(&pool); - >::refresh_members(pool, ChangeReceiver::MembershipInitialized); + >::put(&pool_bounded); + >::refresh_members(pool_bounded, ChangeReceiver::MembershipInitialized); } } @@ -307,7 +319,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); @@ -393,7 +406,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); @@ -423,23 +436,27 @@ 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 pool = pool.into_inner(); - 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); + 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[..]), } } diff --git a/frame/scored-pool/src/mock.rs b/frame/scored-pool/src/mock.rs index 4fef5385eb2c5..824011debafb6 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; @@ -35,7 +35,7 @@ use std::cell::RefCell; 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, @@ -97,7 +97,9 @@ impl pallet_balances::Config for Test { } thread_local! { - pub static MEMBERS: RefCell> = RefCell::new(vec![]); + + pub static MEMBERS: RefCell>> = RefCell::new(bounded_vec![0,10]); + } pub struct TestChangeMembers; @@ -113,13 +115,18 @@ impl ChangeMembers for TestChangeMembers { assert_eq!(old_plus_incoming, new_plus_outgoing); - MEMBERS.with(|m| *m.borrow_mut() = new.to_vec()); + MEMBERS.with(|m| { + *m.borrow_mut() = >>::truncate_from(new.to_vec()) + }); } } impl InitializeMembers for TestChangeMembers { fn initialize_members(new_members: &[u64]) { - MEMBERS.with(|m| *m.borrow_mut() = new_members.to_vec()); + MEMBERS.with(|m| { + *m.borrow_mut() = + >>::truncate_from(new_members.to_vec()) + }); } } @@ -133,25 +140,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..30 { + 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![(5, None), (10, Some(1)), (20, Some(2)), (31, Some(2)), (40, Some(3))], member_count: 2, } .assimilate_storage(&mut t) diff --git a/frame/scored-pool/src/tests.rs b/frame/scored-pool/src/tests.rs index 7b431160ddfe5..1253b24974e9f 100644 --- a/frame/scored-pool/src/tests.rs +++ b/frame/scored-pool/src/tests.rs @@ -20,7 +20,7 @@ use super::*; use mock::*; -use frame_support::{assert_noop, assert_ok, traits::OnInitialize}; +use frame_support::{assert_noop, assert_ok, dispatch::Dispatchable, traits::OnInitialize}; use sp_runtime::traits::BadOrigin; type ScoredPool = Pallet; @@ -297,3 +297,23 @@ fn candidacy_resubmitting_works() { assert_eq!(ScoredPool::candidate_exists(who), true); }); } + +#[test] +fn pool_candidates_exceded() { + 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)); + } + + let submit_candidacy_call = + mock::Call::ScoredPool(crate::Call::::submit_candidacy {}); + + assert_noop!( + submit_candidacy_call.dispatch(Origin::signed(8)), + Error::::TooManyMembers + ); + }); +} From 857bbbe8652d296c045ae78d218a5fd5e804a96b Mon Sep 17 00:00:00 2001 From: Hector Bulgarini Date: Fri, 12 Aug 2022 16:19:11 +0200 Subject: [PATCH 2/8] Addressing PR feedback --- frame/scored-pool/src/lib.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/frame/scored-pool/src/lib.rs b/frame/scored-pool/src/lib.rs index 89f96f17c481f..e64bb4e64a8d7 100644 --- a/frame/scored-pool/src/lib.rs +++ b/frame/scored-pool/src/lib.rs @@ -263,7 +263,7 @@ pub mod pallet { #[pallet::genesis_build] impl, I: 'static> GenesisBuild for GenesisConfig { fn build(&self) { - let mut pool = self.pool.clone().into_inner(); + let mut pool = self.pool.clone(); // reserve balance for each candidate in the pool. // panicking here is ok, since this just happens one time, pre-genesis. @@ -274,12 +274,12 @@ 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 at the end of the bounded vec. pool.sort_by_key(|(_, maybe_score)| Reverse(maybe_score.unwrap_or_default())); - let pool_bounded: PoolT = BoundedVec::truncate_from(pool); - >::put(self.member_count); - >::put(&pool_bounded); - >::refresh_members(pool_bounded, ChangeReceiver::MembershipInitialized); + >::update_member_count(self.member_count) + .expect("Number of allowed memebers exceeded"); + >::put(&pool); + >::refresh_members(pool, ChangeReceiver::MembershipInitialized); } } @@ -422,7 +422,7 @@ pub mod pallet { #[pallet::weight(0)] pub fn change_member_count(origin: OriginFor, count: u32) -> DispatchResult { ensure_root(origin)?; - MemberCount::::put(&count); + Self::update_member_count(count).map_err(|_| Error::::TooManyMembers)?; Ok(()) } } @@ -437,7 +437,6 @@ impl, I: 'static> Pallet { fn refresh_members(pool: PoolT, notify: ChangeReceiver) { let count = MemberCount::::get(); let old_members = >::get(); - let pool = pool.into_inner(); let new_members: Vec = pool .into_iter() @@ -446,6 +445,8 @@ impl, I: 'static> Pallet { .map(|(account_id, _)| account_id) .collect(); + // 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(); @@ -502,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(()) + } } From 80e221b724315b9fd3aa4c9810a92f7b0a0bf0c4 Mon Sep 17 00:00:00 2001 From: Hector Bulgarini Date: Fri, 12 Aug 2022 16:21:22 +0200 Subject: [PATCH 3/8] typo --- frame/scored-pool/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/scored-pool/src/lib.rs b/frame/scored-pool/src/lib.rs index e64bb4e64a8d7..cbd493323f689 100644 --- a/frame/scored-pool/src/lib.rs +++ b/frame/scored-pool/src/lib.rs @@ -274,7 +274,7 @@ pub mod pallet { }); // Sorts the `Pool` by score in a descending order. Entities which - // have a score of `None` are sorted at the end of the bounded 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())); >::update_member_count(self.member_count) .expect("Number of allowed memebers exceeded"); From b4848ddf783be517c998795a7fa17dd47ff7ed99 Mon Sep 17 00:00:00 2001 From: Hector Bulgarini Date: Fri, 12 Aug 2022 16:32:50 +0200 Subject: [PATCH 4/8] typo --- frame/scored-pool/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/scored-pool/src/lib.rs b/frame/scored-pool/src/lib.rs index cbd493323f689..9dfe23ef285ab 100644 --- a/frame/scored-pool/src/lib.rs +++ b/frame/scored-pool/src/lib.rs @@ -277,7 +277,7 @@ pub mod pallet { // 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())); >::update_member_count(self.member_count) - .expect("Number of allowed memebers exceeded"); + .expect("Number of allowed members exceeded"); >::put(&pool); >::refresh_members(pool, ChangeReceiver::MembershipInitialized); } From 0f1c6e08fc19ff017957c8bd7b6bf0404de0d8ce Mon Sep 17 00:00:00 2001 From: Hector Bulgarini Date: Mon, 15 Aug 2022 19:40:50 +0200 Subject: [PATCH 5/8] Addressing PR comments and formatting code --- frame/scored-pool/src/lib.rs | 4 ++-- frame/scored-pool/src/mock.rs | 19 ++++++------------- frame/scored-pool/src/tests.rs | 7 ++----- 3 files changed, 10 insertions(+), 20 deletions(-) diff --git a/frame/scored-pool/src/lib.rs b/frame/scored-pool/src/lib.rs index 9dfe23ef285ab..51eb74056c7fe 100644 --- a/frame/scored-pool/src/lib.rs +++ b/frame/scored-pool/src/lib.rs @@ -422,7 +422,7 @@ pub mod pallet { #[pallet::weight(0)] pub fn change_member_count(origin: OriginFor, count: u32) -> DispatchResult { ensure_root(origin)?; - Self::update_member_count(count).map_err(|_| Error::::TooManyMembers)?; + Self::update_member_count(count)?; Ok(()) } } @@ -506,7 +506,7 @@ impl, I: 'static> Pallet { /// 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); + 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 824011debafb6..c5c2fdcca2817 100644 --- a/frame/scored-pool/src/mock.rs +++ b/frame/scored-pool/src/mock.rs @@ -96,16 +96,14 @@ impl pallet_balances::Config for Test { type WeightInfo = (); } -thread_local! { - - pub static MEMBERS: RefCell>> = RefCell::new(bounded_vec![0,10]); - +parameter_types! { + pub static Members: 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 = MEMBERS.with(|m| m.borrow().to_vec()); + let mut old_plus_incoming = Members::get().into_inner(); old_plus_incoming.extend_from_slice(incoming); old_plus_incoming.sort(); @@ -115,18 +113,13 @@ impl ChangeMembers for TestChangeMembers { assert_eq!(old_plus_incoming, new_plus_outgoing); - MEMBERS.with(|m| { - *m.borrow_mut() = >>::truncate_from(new.to_vec()) - }); + Members::set(>>::truncate_from(new.to_vec())); } } impl InitializeMembers for TestChangeMembers { fn initialize_members(new_members: &[u64]) { - MEMBERS.with(|m| { - *m.borrow_mut() = - >>::truncate_from(new_members.to_vec()) - }); + Members::set(>>::truncate_from(new_members.to_vec())); } } @@ -146,7 +139,7 @@ impl Config for Test { pub fn new_test_ext() -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); let mut balances = vec![]; - for i in 1..30 { + for i in 1..31 { balances.push((i, 500_000)); } balances.push((31, 500_000)); diff --git a/frame/scored-pool/src/tests.rs b/frame/scored-pool/src/tests.rs index 1253b24974e9f..d7c87bb35a396 100644 --- a/frame/scored-pool/src/tests.rs +++ b/frame/scored-pool/src/tests.rs @@ -20,7 +20,7 @@ use super::*; use mock::*; -use frame_support::{assert_noop, assert_ok, dispatch::Dispatchable, traits::OnInitialize}; +use frame_support::{assert_noop, assert_ok, traits::OnInitialize}; use sp_runtime::traits::BadOrigin; type ScoredPool = Pallet; @@ -308,11 +308,8 @@ fn pool_candidates_exceded() { assert_ok!(ScoredPool::score(Origin::signed(ScoreOrigin::get()), who, index, 99)); } - let submit_candidacy_call = - mock::Call::ScoredPool(crate::Call::::submit_candidacy {}); - assert_noop!( - submit_candidacy_call.dispatch(Origin::signed(8)), + ScoredPool::submit_candidacy(Origin::signed(8)), Error::::TooManyMembers ); }); From 52b506ae682eab316f821faa786b9f1b73e27723 Mon Sep 17 00:00:00 2001 From: Hector Bulgarini Date: Tue, 16 Aug 2022 08:41:56 +0200 Subject: [PATCH 6/8] Removing unwanted import --- frame/scored-pool/src/mock.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/scored-pool/src/mock.rs b/frame/scored-pool/src/mock.rs index c5c2fdcca2817..b4eadbc0700f3 100644 --- a/frame/scored-pool/src/mock.rs +++ b/frame/scored-pool/src/mock.rs @@ -30,7 +30,6 @@ use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, }; -use std::cell::RefCell; type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; From f1a63551823ae3be204de93352547f0f26a09765 Mon Sep 17 00:00:00 2001 From: Hector Bulgarini Date: Tue, 16 Aug 2022 12:42:19 +0200 Subject: [PATCH 7/8] Adding a map_err --- frame/scored-pool/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/scored-pool/src/lib.rs b/frame/scored-pool/src/lib.rs index 51eb74056c7fe..03049d86f977f 100644 --- a/frame/scored-pool/src/lib.rs +++ b/frame/scored-pool/src/lib.rs @@ -422,8 +422,7 @@ pub mod pallet { #[pallet::weight(0)] pub fn change_member_count(origin: OriginFor, count: u32) -> DispatchResult { ensure_root(origin)?; - Self::update_member_count(count)?; - Ok(()) + Self::update_member_count(count).map_err(Into::into) } } } From 94286dddeee8617e85ec89d2868e45d4fdba9541 Mon Sep 17 00:00:00 2001 From: Hector Bulgarini Date: Thu, 8 Sep 2022 11:45:22 +0200 Subject: [PATCH 8/8] cargo fmt --- frame/scored-pool/src/mock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/scored-pool/src/mock.rs b/frame/scored-pool/src/mock.rs index 36867151b2daf..8724fed17851c 100644 --- a/frame/scored-pool/src/mock.rs +++ b/frame/scored-pool/src/mock.rs @@ -149,7 +149,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { .assimilate_storage(&mut t) .unwrap(); pallet_scored_pool::GenesisConfig:: { - pool: bounded_vec![(10, Some(1)), (20, Some(2)), (31, Some(2)), (40, Some(3)),(5, None)], + pool: bounded_vec![(10, Some(1)), (20, Some(2)), (31, Some(2)), (40, Some(3)), (5, None)], member_count: 2, } .assimilate_storage(&mut t)