From 333f4c78109345debb5cf6e8c8b3fe75a7bbe3fb Mon Sep 17 00:00:00 2001 From: "polka.dom" Date: Mon, 1 Jul 2024 05:25:26 -0700 Subject: [PATCH] Remove getters from pallet-membership (#4840) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As per #3326 , removes pallet::getter macro usage from pallet-membership. The syntax StorageItem::::get() should be used instead. Also converts some syntax to turbo and reimplements the removed getters, following #223 cc @muraca --------- Co-authored-by: Dónal Murray Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- prdoc/pr_4840.prdoc | 14 ++ substrate/frame/membership/src/lib.rs | 176 +++++++++++++++----------- 2 files changed, 118 insertions(+), 72 deletions(-) create mode 100644 prdoc/pr_4840.prdoc diff --git a/prdoc/pr_4840.prdoc b/prdoc/pr_4840.prdoc new file mode 100644 index 000000000000..265e1f41c3f3 --- /dev/null +++ b/prdoc/pr_4840.prdoc @@ -0,0 +1,14 @@ +# 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: Removed `pallet::getter` usage from pallet-membership + +doc: + - audience: Runtime Dev + description: | + This PR removed the `pallet::getter`s from `pallet-membership`. + The syntax `StorageItem::::get()` should be used instead. + +crates: + - name: pallet-membership + bump: minor \ No newline at end of file diff --git a/substrate/frame/membership/src/lib.rs b/substrate/frame/membership/src/lib.rs index 8deb4fc022f3..d5dad68e811b 100644 --- a/substrate/frame/membership/src/lib.rs +++ b/substrate/frame/membership/src/lib.rs @@ -95,13 +95,11 @@ pub mod pallet { /// The current membership, stored as an ordered Vec. #[pallet::storage] - #[pallet::getter(fn members)] pub type Members, I: 'static = ()> = StorageValue<_, BoundedVec, ValueQuery>; /// The current prime member, if one exists. #[pallet::storage] - #[pallet::getter(fn prime)] pub type Prime, I: 'static = ()> = StorageValue<_, T::AccountId, OptionQuery>; #[pallet::genesis_config] @@ -126,7 +124,7 @@ pub mod pallet { let mut members = self.members.clone(); members.sort(); T::MembershipInitialized::initialize_members(&members); - >::put(members); + Members::::put(members); } } @@ -171,14 +169,14 @@ pub mod pallet { T::AddOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; - let mut members = >::get(); + let mut members = Members::::get(); let init_length = members.len(); let location = members.binary_search(&who).err().ok_or(Error::::AlreadyMember)?; members .try_insert(location, who.clone()) .map_err(|_| Error::::TooManyMembers)?; - >::put(&members); + Members::::put(&members); T::MembershipChanged::change_members_sorted(&[who], &[], &members[..]); @@ -199,12 +197,12 @@ pub mod pallet { T::RemoveOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; - let mut members = >::get(); + let mut members = Members::::get(); let init_length = members.len(); let location = members.binary_search(&who).ok().ok_or(Error::::NotMember)?; members.remove(location); - >::put(&members); + Members::::put(&members); T::MembershipChanged::change_members_sorted(&[], &[who], &members[..]); Self::rejig_prime(&members); @@ -233,13 +231,13 @@ pub mod pallet { return Ok(().into()); } - let mut members = >::get(); + let mut members = Members::::get(); let location = members.binary_search(&remove).ok().ok_or(Error::::NotMember)?; let _ = members.binary_search(&add).err().ok_or(Error::::AlreadyMember)?; members[location] = add.clone(); members.sort(); - >::put(&members); + Members::::put(&members); T::MembershipChanged::change_members_sorted(&[add], &[remove], &members[..]); Self::rejig_prime(&members); @@ -260,7 +258,7 @@ pub mod pallet { let mut members: BoundedVec = BoundedVec::try_from(members).map_err(|_| Error::::TooManyMembers)?; members.sort(); - >::mutate(|m| { + Members::::mutate(|m| { T::MembershipChanged::set_members_sorted(&members[..], m); Self::rejig_prime(&members); *m = members; @@ -288,14 +286,14 @@ pub mod pallet { return Ok(().into()); } - let mut members = >::get(); + let mut members = Members::::get(); let members_length = members.len() as u32; let location = members.binary_search(&remove).ok().ok_or(Error::::NotMember)?; let _ = members.binary_search(&new).err().ok_or(Error::::AlreadyMember)?; members[location] = new.clone(); members.sort(); - >::put(&members); + Members::::put(&members); T::MembershipChanged::change_members_sorted( &[new.clone()], @@ -323,7 +321,7 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { T::PrimeOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; - let members = Self::members(); + let members = Members::::get(); members.binary_search(&who).ok().ok_or(Error::::NotMember)?; Prime::::put(&who); T::MembershipChanged::set_prime(Some(who)); @@ -345,6 +343,16 @@ pub mod pallet { } impl, I: 'static> Pallet { + /// The current membership, stored as an ordered `Vec`. + pub fn members() -> BoundedVec { + Members::::get() + } + + /// The current prime member, if one exists. + pub fn prime() -> Option { + Prime::::get() + } + fn rejig_prime(members: &[T::AccountId]) { if let Some(prime) = Prime::::get() { match members.binary_search(&prime) { @@ -357,7 +365,7 @@ impl, I: 'static> Pallet { impl, I: 'static> Contains for Pallet { fn contains(t: &T::AccountId) -> bool { - Self::members().binary_search(t).is_ok() + Members::::get().binary_search(t).is_ok() } } @@ -374,7 +382,7 @@ impl ContainsLengthBound for Pallet { impl, I: 'static> SortedMembers for Pallet { fn sorted_members() -> Vec { - Self::members().to_vec() + Members::::get().to_vec() } fn count() -> usize { @@ -409,12 +417,12 @@ mod benchmark { let prime_origin = T::PrimeOrigin::try_successful_origin() .expect("PrimeOrigin has no successful origin required for the benchmark"); - assert_ok!(>::reset_members(reset_origin, members.clone())); + assert_ok!(Membership::::reset_members(reset_origin, members.clone())); if let Some(prime) = prime.map(|i| members[i].clone()) { let prime_lookup = T::Lookup::unlookup(prime); - assert_ok!(>::set_prime(prime_origin, prime_lookup)); + assert_ok!(Membership::::set_prime(prime_origin, prime_lookup)); } else { - assert_ok!(>::clear_prime(prime_origin)); + assert_ok!(Membership::::clear_prime(prime_origin)); } } @@ -427,12 +435,12 @@ mod benchmark { let new_member = account::("add", m, SEED); let new_member_lookup = T::Lookup::unlookup(new_member.clone()); }: { - assert_ok!(>::add_member( + assert_ok!(Membership::::add_member( T::AddOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?, new_member_lookup, )); } verify { - assert!(>::get().contains(&new_member)); + assert!(Members::::get().contains(&new_member)); #[cfg(test)] crate::tests::clean(); } @@ -447,14 +455,14 @@ mod benchmark { let to_remove = members.first().cloned().unwrap(); let to_remove_lookup = T::Lookup::unlookup(to_remove.clone()); }: { - assert_ok!(>::remove_member( + assert_ok!(Membership::::remove_member( T::RemoveOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?, to_remove_lookup, )); } verify { - assert!(!>::get().contains(&to_remove)); + assert!(!Members::::get().contains(&to_remove)); // prime is rejigged - assert!(>::get().is_some() && T::MembershipChanged::get_prime().is_some()); + assert!(Prime::::get().is_some() && T::MembershipChanged::get_prime().is_some()); #[cfg(test)] crate::tests::clean(); } @@ -469,16 +477,16 @@ mod benchmark { let remove = members.first().cloned().unwrap(); let remove_lookup = T::Lookup::unlookup(remove.clone()); }: { - assert_ok!(>::swap_member( + assert_ok!(Membership::::swap_member( T::SwapOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?, remove_lookup, add_lookup, )); } verify { - assert!(!>::get().contains(&remove)); - assert!(>::get().contains(&add)); + assert!(!Members::::get().contains(&remove)); + assert!(Members::::get().contains(&add)); // prime is rejigged - assert!(>::get().is_some() && T::MembershipChanged::get_prime().is_some()); + assert!(Prime::::get().is_some() && T::MembershipChanged::get_prime().is_some()); #[cfg(test)] crate::tests::clean(); } @@ -490,15 +498,15 @@ mod benchmark { set_members::(members.clone(), Some(members.len() - 1)); let mut new_members = (m..2*m).map(|i| account("member", i, SEED)).collect::>(); }: { - assert_ok!(>::reset_members( + assert_ok!(Membership::::reset_members( T::ResetOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?, new_members.clone(), )); } verify { new_members.sort(); - assert_eq!(>::get(), new_members); + assert_eq!(Members::::get(), new_members); // prime is rejigged - assert!(>::get().is_some() && T::MembershipChanged::get_prime().is_some()); + assert!(Prime::::get().is_some() && T::MembershipChanged::get_prime().is_some()); #[cfg(test)] crate::tests::clean(); } @@ -514,12 +522,12 @@ mod benchmark { let add_lookup = T::Lookup::unlookup(add.clone()); whitelist!(prime); }: { - assert_ok!(>::change_key(RawOrigin::Signed(prime.clone()).into(), add_lookup)); + assert_ok!(Membership::::change_key(RawOrigin::Signed(prime.clone()).into(), add_lookup)); } verify { - assert!(!>::get().contains(&prime)); - assert!(>::get().contains(&add)); + assert!(!Members::::get().contains(&prime)); + assert!(Members::::get().contains(&add)); // prime is rejigged - assert_eq!(>::get().unwrap(), add); + assert_eq!(Prime::::get().unwrap(), add); #[cfg(test)] crate::tests::clean(); } @@ -530,12 +538,12 @@ mod benchmark { let prime_lookup = T::Lookup::unlookup(prime.clone()); set_members::(members, None); }: { - assert_ok!(>::set_prime( + assert_ok!(Membership::::set_prime( T::PrimeOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?, prime_lookup, )); } verify { - assert!(>::get().is_some()); + assert!(Prime::::get().is_some()); assert!(::get_prime().is_some()); #[cfg(test)] crate::tests::clean(); } @@ -545,11 +553,11 @@ mod benchmark { let prime = members.last().cloned().unwrap(); set_members::(members, None); }: { - assert_ok!(>::clear_prime( + assert_ok!(Membership::::clear_prime( T::PrimeOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?, )); } verify { - assert!(>::get().is_none()); + assert!(Prime::::get().is_none()); assert!(::get_prime().is_none()); #[cfg(test)] crate::tests::clean(); } @@ -666,7 +674,7 @@ mod tests { #[test] fn query_membership_works() { new_test_ext().execute_with(|| { - assert_eq!(Membership::members(), vec![10, 20, 30]); + assert_eq!(crate::Members::::get(), vec![10, 20, 30]); assert_eq!(MEMBERS.with(|m| m.borrow().clone()), vec![10, 20, 30]); }); } @@ -680,12 +688,12 @@ mod tests { Error::::NotMember ); assert_ok!(Membership::set_prime(RuntimeOrigin::signed(5), 20)); - assert_eq!(Membership::prime(), Some(20)); - assert_eq!(PRIME.with(|m| *m.borrow()), Membership::prime()); + assert_eq!(crate::Prime::::get(), Some(20)); + assert_eq!(PRIME.with(|m| *m.borrow()), crate::Prime::::get()); assert_ok!(Membership::clear_prime(RuntimeOrigin::signed(5))); - assert_eq!(Membership::prime(), None); - assert_eq!(PRIME.with(|m| *m.borrow()), Membership::prime()); + assert_eq!(crate::Prime::::get(), None); + assert_eq!(PRIME.with(|m| *m.borrow()), crate::Prime::::get()); }); } @@ -698,8 +706,11 @@ mod tests { Error::::AlreadyMember ); assert_ok!(Membership::add_member(RuntimeOrigin::signed(1), 15)); - assert_eq!(Membership::members(), vec![10, 15, 20, 30]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); + assert_eq!(crate::Members::::get(), vec![10, 15, 20, 30]); + assert_eq!( + MEMBERS.with(|m| m.borrow().clone()), + crate::Members::::get().to_vec() + ); }); } @@ -713,10 +724,13 @@ mod tests { ); assert_ok!(Membership::set_prime(RuntimeOrigin::signed(5), 20)); assert_ok!(Membership::remove_member(RuntimeOrigin::signed(2), 20)); - assert_eq!(Membership::members(), vec![10, 30]); - 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()); + assert_eq!(crate::Members::::get(), vec![10, 30]); + assert_eq!( + MEMBERS.with(|m| m.borrow().clone()), + crate::Members::::get().to_vec() + ); + assert_eq!(crate::Prime::::get(), None); + assert_eq!(PRIME.with(|m| *m.borrow()), crate::Prime::::get()); }); } @@ -735,16 +749,19 @@ mod tests { assert_ok!(Membership::set_prime(RuntimeOrigin::signed(5), 20)); assert_ok!(Membership::swap_member(RuntimeOrigin::signed(3), 20, 20)); - assert_eq!(Membership::members(), vec![10, 20, 30]); - assert_eq!(Membership::prime(), Some(20)); - assert_eq!(PRIME.with(|m| *m.borrow()), Membership::prime()); + assert_eq!(crate::Members::::get(), vec![10, 20, 30]); + assert_eq!(crate::Prime::::get(), Some(20)); + assert_eq!(PRIME.with(|m| *m.borrow()), crate::Prime::::get()); assert_ok!(Membership::set_prime(RuntimeOrigin::signed(5), 10)); assert_ok!(Membership::swap_member(RuntimeOrigin::signed(3), 10, 25)); - assert_eq!(Membership::members(), vec![20, 25, 30]); - 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()); + assert_eq!(crate::Members::::get(), vec![20, 25, 30]); + assert_eq!( + MEMBERS.with(|m| m.borrow().clone()), + crate::Members::::get().to_vec() + ); + assert_eq!(crate::Prime::::get(), None); + assert_eq!(PRIME.with(|m| *m.borrow()), crate::Prime::::get()); }); } @@ -752,8 +769,11 @@ mod tests { fn swap_member_works_that_does_not_change_order() { new_test_ext().execute_with(|| { assert_ok!(Membership::swap_member(RuntimeOrigin::signed(3), 10, 5)); - assert_eq!(Membership::members(), vec![5, 20, 30]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); + assert_eq!(crate::Members::::get(), vec![5, 20, 30]); + assert_eq!( + MEMBERS.with(|m| m.borrow().clone()), + crate::Members::::get().to_vec() + ); }); } @@ -781,10 +801,13 @@ mod tests { Error::::AlreadyMember ); assert_ok!(Membership::change_key(RuntimeOrigin::signed(10), 40)); - assert_eq!(Membership::members(), vec![20, 30, 40]); - 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()); + assert_eq!(crate::Members::::get(), vec![20, 30, 40]); + assert_eq!( + MEMBERS.with(|m| m.borrow().clone()), + crate::Members::::get().to_vec() + ); + assert_eq!(crate::Prime::::get(), Some(40)); + assert_eq!(PRIME.with(|m| *m.borrow()), crate::Prime::::get()); }); } @@ -792,8 +815,11 @@ mod tests { fn change_key_works_that_does_not_change_order() { new_test_ext().execute_with(|| { assert_ok!(Membership::change_key(RuntimeOrigin::signed(10), 5)); - assert_eq!(Membership::members(), vec![5, 20, 30]); - assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members().to_vec()); + assert_eq!(crate::Members::::get(), vec![5, 20, 30]); + assert_eq!( + MEMBERS.with(|m| m.borrow().clone()), + crate::Members::::get().to_vec() + ); }); } @@ -814,16 +840,22 @@ mod tests { ); assert_ok!(Membership::reset_members(RuntimeOrigin::signed(4), vec![20, 40, 30])); - assert_eq!(Membership::members(), vec![20, 30, 40]); - 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_eq!(crate::Members::::get(), vec![20, 30, 40]); + assert_eq!( + MEMBERS.with(|m| m.borrow().clone()), + crate::Members::::get().to_vec() + ); + assert_eq!(crate::Prime::::get(), Some(20)); + assert_eq!(PRIME.with(|m| *m.borrow()), crate::Prime::::get()); assert_ok!(Membership::reset_members(RuntimeOrigin::signed(4), vec![10, 40, 30])); - assert_eq!(Membership::members(), vec![10, 30, 40]); - 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()); + assert_eq!(crate::Members::::get(), vec![10, 30, 40]); + assert_eq!( + MEMBERS.with(|m| m.borrow().clone()), + crate::Members::::get().to_vec() + ); + assert_eq!(crate::Prime::::get(), None); + assert_eq!(PRIME.with(|m| *m.borrow()), crate::Prime::::get()); }); }