Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Remove without_storage_info for membership pallet #11591

Merged
merged 5 commits into from
Jun 7, 2022
Merged

Remove without_storage_info for membership pallet #11591

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
05c1115
Add StorageInfo to membership pallet
skunert Jun 3, 2022
f857d04
Clean up
skunert Jun 3, 2022
420dd67
Fix build error
skunert Jun 7, 2022
ae2e37e
Fix benchmarks
skunert Jun 7, 2022
4c30112
Adjust benchmark boundary
skunert Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 32 additions & 38 deletions frame/membership/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<T, I = ()>(PhantomData<(T, I)>);

#[pallet::config]
Expand Down Expand Up @@ -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<u32>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we intentionally made it so that it's unenforceable, someone else with more context needs to review this to ensure we're not breaking anything here, and if so, we'd have to use a WeakBoundedVec rather than a BoundedVec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is just because it was not bounded. We want to enforce this limit.


/// Weight information for extrinsics in this pallet.
Expand All @@ -90,7 +92,7 @@ pub mod pallet {
#[pallet::storage]
#[pallet::getter(fn members)]
pub type Members<T: Config<I>, I: 'static = ()> =
StorageValue<_, Vec<T::AccountId>, ValueQuery>;
StorageValue<_, BoundedVec<T::AccountId, T::MaxMembers>, ValueQuery>;

/// The current prime member, if one exists.
#[pallet::storage]
Expand All @@ -99,14 +101,14 @@ pub mod pallet {

#[pallet::genesis_config]
pub struct GenesisConfig<T: Config<I>, I: 'static = ()> {
pub members: Vec<T::AccountId>,
pub members: BoundedVec<T::AccountId, T::MaxMembers>,
pub phantom: PhantomData<I>,
}

#[cfg(feature = "std")]
impl<T: Config<I>, I: 'static> Default for GenesisConfig<T, I> {
fn default() -> Self {
Self { members: Vec::new(), phantom: Default::default() }
Self { members: Default::default(), phantom: Default::default() }
}
}

Expand Down Expand Up @@ -151,6 +153,8 @@ pub mod pallet {
AlreadyMember,
/// Not a member.
NotMember,
/// Too many members.
TooManyMembers,
}

#[pallet::call]
Expand All @@ -164,9 +168,10 @@ pub mod pallet {

let mut members = <Members<T, I>>::get();
let location = members.binary_search(&who).err().ok_or(Error::<T, I>::AlreadyMember)?;
members.insert(location, who.clone());
members
.try_insert(location, who.clone())
.map_err(|_| Error::<T, I>::TooManyMembers)?;

Self::maybe_warn_max_members(&members);
<Members<T, I>>::put(&members);

T::MembershipChanged::change_members_sorted(&[who], &[], &members[..]);
Expand All @@ -186,7 +191,6 @@ pub mod pallet {
let location = members.binary_search(&who).ok().ok_or(Error::<T, I>::NotMember)?;
members.remove(location);

Self::maybe_warn_max_members(&members);
<Members<T, I>>::put(&members);

T::MembershipChanged::change_members_sorted(&[], &[who], &members[..]);
Expand Down Expand Up @@ -219,7 +223,6 @@ pub mod pallet {
members[location] = add.clone();
members.sort();

Self::maybe_warn_max_members(&members);
<Members<T, I>>::put(&members);

T::MembershipChanged::change_members_sorted(&[add], &[remove], &members[..]);
Expand All @@ -237,12 +240,12 @@ pub mod pallet {
pub fn reset_members(origin: OriginFor<T>, members: Vec<T::AccountId>) -> DispatchResult {
T::ResetOrigin::ensure_origin(origin)?;

let mut members = members;
let mut members: BoundedVec<T::AccountId, T::MaxMembers> =
BoundedVec::try_from(members).map_err(|_| Error::<T, I>::TooManyMembers)?;
members.sort();
<Members<T, I>>::mutate(|m| {
T::MembershipChanged::set_members_sorted(&members[..], m);
Self::rejig_prime(&members);
Self::maybe_warn_max_members(&members);
*m = members;
});

Expand All @@ -267,7 +270,6 @@ pub mod pallet {
members[location] = new.clone();
members.sort();

Self::maybe_warn_max_members(&members);
<Members<T, I>>::put(&members);

T::MembershipChanged::change_members_sorted(
Expand Down Expand Up @@ -320,17 +322,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
}
}
}

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<T: Config<I>, I: 'static> Contains<T::AccountId> for Pallet<T, I> {
Expand All @@ -341,7 +332,7 @@ impl<T: Config<I>, I: 'static> Contains<T::AccountId> for Pallet<T, I> {

impl<T: Config<I>, I: 'static> SortedMembers<T::AccountId> for Pallet<T, I> {
fn sorted_members() -> Vec<T::AccountId> {
Self::members()
Self::members().to_vec()
}

fn count() -> usize {
Expand Down Expand Up @@ -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::<Vec<T::AccountId>>();
set_members::<T, I>(members, None);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -609,7 +600,7 @@ mod tests {
let mut t = frame_system::GenesisConfig::default().build_storage::<Test>().unwrap();
// We use default for brevity, but you can configure as desired if needed.
pallet_membership::GenesisConfig::<Test> {
members: vec![10, 20, 30],
members: bounded_vec![10, 20, 30],
..Default::default()
}
.assimilate_storage(&mut t)
Expand Down Expand Up @@ -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());
});
}

Expand All @@ -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());
});
Expand Down Expand Up @@ -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());
});
Expand All @@ -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());
});
}

Expand All @@ -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());
});
Expand All @@ -744,25 +735,28 @@ 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());
});
}

#[test]
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());
});
Expand All @@ -772,7 +766,7 @@ mod tests {
#[should_panic(expected = "Members cannot contain duplicate accounts.")]
fn genesis_build_panics_with_duplicate_members() {
pallet_membership::GenesisConfig::<Test> {
members: vec![1, 2, 3, 1],
members: bounded_vec![1, 2, 3, 1],
phantom: Default::default(),
}
.build_storage()
Expand Down