From 1dd17e3674489d6f8447a7634446f43c70761307 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 7 Jun 2022 19:49:36 +0200 Subject: [PATCH] Remove `without_storage_info` for membership pallet (#11591) --- frame/membership/src/lib.rs | 70 +++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 38 deletions(-) diff --git a/frame/membership/src/lib.rs b/frame/membership/src/lib.rs index 8e7ea9eeec313..24ecfd5333c66 100644 --- a/frame/membership/src/lib.rs +++ b/frame/membership/src/lib.rs @@ -23,7 +23,10 @@ // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] -use frame_support::traits::{ChangeMembers, Contains, Get, InitializeMembers, SortedMembers}; +use frame_support::{ + traits::{ChangeMembers, Contains, Get, InitializeMembers, SortedMembers}, + BoundedVec, +}; use sp_std::prelude::*; pub mod migrations; @@ -44,7 +47,6 @@ pub mod pallet { #[pallet::pallet] #[pallet::generate_store(pub(super) trait Store)] #[pallet::storage_version(STORAGE_VERSION)] - #[pallet::without_storage_info] pub struct Pallet(PhantomData<(T, I)>); #[pallet::config] @@ -79,7 +81,7 @@ pub mod pallet { /// /// This is used for benchmarking. Re-run the benchmarks if this changes. /// - /// This is not enforced in the code; the membership size can exceed this limit. + /// This is enforced in the code; the membership size can not exceed this limit. type MaxMembers: Get; /// Weight information for extrinsics in this pallet. @@ -90,7 +92,7 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn members)] pub type Members, I: 'static = ()> = - StorageValue<_, Vec, ValueQuery>; + StorageValue<_, BoundedVec, ValueQuery>; /// The current prime member, if one exists. #[pallet::storage] @@ -99,14 +101,14 @@ pub mod pallet { #[pallet::genesis_config] pub struct GenesisConfig, I: 'static = ()> { - pub members: Vec, + pub members: BoundedVec, pub phantom: PhantomData, } #[cfg(feature = "std")] impl, I: 'static> Default for GenesisConfig { fn default() -> Self { - Self { members: Vec::new(), phantom: Default::default() } + Self { members: Default::default(), phantom: Default::default() } } } @@ -151,6 +153,8 @@ pub mod pallet { AlreadyMember, /// Not a member. NotMember, + /// Too many members. + TooManyMembers, } #[pallet::call] @@ -164,9 +168,10 @@ pub mod pallet { let mut members = >::get(); let location = members.binary_search(&who).err().ok_or(Error::::AlreadyMember)?; - members.insert(location, who.clone()); + members + .try_insert(location, who.clone()) + .map_err(|_| Error::::TooManyMembers)?; - Self::maybe_warn_max_members(&members); >::put(&members); T::MembershipChanged::change_members_sorted(&[who], &[], &members[..]); @@ -186,7 +191,6 @@ pub mod pallet { let location = members.binary_search(&who).ok().ok_or(Error::::NotMember)?; members.remove(location); - Self::maybe_warn_max_members(&members); >::put(&members); T::MembershipChanged::change_members_sorted(&[], &[who], &members[..]); @@ -219,7 +223,6 @@ pub mod pallet { members[location] = add.clone(); members.sort(); - Self::maybe_warn_max_members(&members); >::put(&members); T::MembershipChanged::change_members_sorted(&[add], &[remove], &members[..]); @@ -237,12 +240,12 @@ pub mod pallet { pub fn reset_members(origin: OriginFor, members: Vec) -> DispatchResult { T::ResetOrigin::ensure_origin(origin)?; - let mut members = members; + let mut members: BoundedVec = + BoundedVec::try_from(members).map_err(|_| Error::::TooManyMembers)?; members.sort(); >::mutate(|m| { T::MembershipChanged::set_members_sorted(&members[..], m); Self::rejig_prime(&members); - Self::maybe_warn_max_members(&members); *m = members; }); @@ -267,7 +270,6 @@ pub mod pallet { members[location] = new.clone(); members.sort(); - Self::maybe_warn_max_members(&members); >::put(&members); T::MembershipChanged::change_members_sorted( @@ -320,17 +322,6 @@ impl, I: 'static> Pallet { } } } - - fn maybe_warn_max_members(members: &[T::AccountId]) { - if members.len() as u32 > T::MaxMembers::get() { - log::error!( - target: "runtime::membership", - "maximum number of members used for weight is exceeded, weights can be underestimated [{} > {}].", - members.len(), - T::MaxMembers::get(), - ) - } - } } impl, I: 'static> Contains for Pallet { @@ -341,7 +332,7 @@ impl, I: 'static> Contains for Pallet { impl, I: 'static> SortedMembers for Pallet { fn sorted_members() -> Vec { - Self::members() + Self::members().to_vec() } fn count() -> usize { @@ -372,7 +363,7 @@ mod benchmark { benchmarks_instance_pallet! { add_member { - let m in 1 .. T::MaxMembers::get(); + let m in 1 .. (T::MaxMembers::get() - 1); let members = (0..m).map(|i| account("member", i, SEED)).collect::>(); set_members::(members, None); @@ -504,7 +495,7 @@ mod tests { }; use frame_support::{ - assert_noop, assert_ok, ord_parameter_types, parameter_types, + assert_noop, assert_ok, bounded_vec, ord_parameter_types, parameter_types, traits::{ConstU32, ConstU64, GenesisBuild, StorageVersion}, }; use frame_system::EnsureSignedBy; @@ -609,7 +600,7 @@ mod tests { let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); // We use default for brevity, but you can configure as desired if needed. pallet_membership::GenesisConfig:: { - members: vec![10, 20, 30], + members: bounded_vec![10, 20, 30], ..Default::default() } .assimilate_storage(&mut t) @@ -661,7 +652,7 @@ mod tests { ); assert_ok!(Membership::add_member(Origin::signed(1), 15)); assert_eq!(Membership::members(), vec![10, 15, 20, 30]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); + assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); }); } @@ -676,7 +667,7 @@ mod tests { assert_ok!(Membership::set_prime(Origin::signed(5), 20)); assert_ok!(Membership::remove_member(Origin::signed(2), 20)); assert_eq!(Membership::members(), vec![10, 30]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); + assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); assert_eq!(Membership::prime(), None); assert_eq!(PRIME.with(|m| *m.borrow()), Membership::prime()); }); @@ -704,7 +695,7 @@ mod tests { assert_ok!(Membership::set_prime(Origin::signed(5), 10)); assert_ok!(Membership::swap_member(Origin::signed(3), 10, 25)); assert_eq!(Membership::members(), vec![20, 25, 30]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); + assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); assert_eq!(Membership::prime(), None); assert_eq!(PRIME.with(|m| *m.borrow()), Membership::prime()); }); @@ -715,7 +706,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Membership::swap_member(Origin::signed(3), 10, 5)); assert_eq!(Membership::members(), vec![5, 20, 30]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); + assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); }); } @@ -733,7 +724,7 @@ mod tests { ); assert_ok!(Membership::change_key(Origin::signed(10), 40)); assert_eq!(Membership::members(), vec![20, 30, 40]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); + assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); assert_eq!(Membership::prime(), Some(40)); assert_eq!(PRIME.with(|m| *m.borrow()), Membership::prime()); }); @@ -744,7 +735,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Membership::change_key(Origin::signed(10), 5)); assert_eq!(Membership::members(), vec![5, 20, 30]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); + assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); }); } @@ -752,17 +743,20 @@ mod tests { fn reset_members_works() { new_test_ext().execute_with(|| { assert_ok!(Membership::set_prime(Origin::signed(5), 20)); - assert_noop!(Membership::reset_members(Origin::signed(1), vec![20, 40, 30]), BadOrigin); + assert_noop!( + Membership::reset_members(Origin::signed(1), bounded_vec![20, 40, 30]), + BadOrigin + ); assert_ok!(Membership::reset_members(Origin::signed(4), vec![20, 40, 30])); assert_eq!(Membership::members(), vec![20, 30, 40]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); + assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); assert_eq!(Membership::prime(), Some(20)); assert_eq!(PRIME.with(|m| *m.borrow()), Membership::prime()); assert_ok!(Membership::reset_members(Origin::signed(4), vec![10, 40, 30])); assert_eq!(Membership::members(), vec![10, 30, 40]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members()); + assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); assert_eq!(Membership::prime(), None); assert_eq!(PRIME.with(|m| *m.borrow()), Membership::prime()); }); @@ -772,7 +766,7 @@ mod tests { #[should_panic(expected = "Members cannot contain duplicate accounts.")] fn genesis_build_panics_with_duplicate_members() { pallet_membership::GenesisConfig:: { - members: vec![1, 2, 3, 1], + members: bounded_vec![1, 2, 3, 1], phantom: Default::default(), } .build_storage()