Skip to content

Commit

Permalink
pallet-membership weights (#3324)
Browse files Browse the repository at this point in the history 
## Summary
* use benchamarked weights instead of hardcoded ones for
`pallet-membership`
* rename benchmark to match extrinsic name
* remove unnecessary dependency from `clear_prime`

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
  • Loading branch information
@Dinonard @ggwpez
Dinonard and ggwpez authored Feb 23, 2024
1 parent f2645ee commit 5fc6d67
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 56 deletions.
13 changes: 13 additions & 0 deletions prdoc/pr_3324.prdoc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# 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: "Fix weight calculation and event emission in pallet-membership"

doc:
- audience: Runtime Dev
description: |
Bug fix for the membership pallet to use correct weights. Also no event will be emitted
anymore when `change_key` is called with identical accounts.

crates:
- name: pallet-membership
124 changes: 80 additions & 44 deletions substrate/frame/membership/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use frame_support::{
traits::{ChangeMembers, Contains, Get, InitializeMembers, SortedMembers},
BoundedVec,
};
use sp_runtime::traits::StaticLookup;
use sp_runtime::traits::{StaticLookup, UniqueSaturatedInto};
use sp_std::prelude::*;

pub mod migrations;
Expand Down Expand Up @@ -163,12 +163,16 @@ pub mod pallet {
///
/// May only be called from `T::AddOrigin`.
#[pallet::call_index(0)]
#[pallet::weight({50_000_000})]
pub fn add_member(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
#[pallet::weight(T::WeightInfo::add_member(T::MaxMembers::get()))]
pub fn add_member(
origin: OriginFor<T>,
who: AccountIdLookupOf<T>,
) -> DispatchResultWithPostInfo {
T::AddOrigin::ensure_origin(origin)?;
let who = T::Lookup::lookup(who)?;

let mut members = <Members<T, I>>::get();
let init_length = members.len();
let location = members.binary_search(&who).err().ok_or(Error::<T, I>::AlreadyMember)?;
members
.try_insert(location, who.clone())
Expand All @@ -179,19 +183,24 @@ pub mod pallet {
T::MembershipChanged::change_members_sorted(&[who], &[], &members[..]);

Self::deposit_event(Event::MemberAdded);
Ok(())

Ok(Some(T::WeightInfo::add_member(init_length as u32)).into())
}

/// Remove a member `who` from the set.
///
/// May only be called from `T::RemoveOrigin`.
#[pallet::call_index(1)]
#[pallet::weight({50_000_000})]
pub fn remove_member(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
#[pallet::weight(T::WeightInfo::remove_member(T::MaxMembers::get()))]
pub fn remove_member(
origin: OriginFor<T>,
who: AccountIdLookupOf<T>,
) -> DispatchResultWithPostInfo {
T::RemoveOrigin::ensure_origin(origin)?;
let who = T::Lookup::lookup(who)?;

let mut members = <Members<T, I>>::get();
let init_length = members.len();
let location = members.binary_search(&who).ok().ok_or(Error::<T, I>::NotMember)?;
members.remove(location);

Expand All @@ -201,7 +210,7 @@ pub mod pallet {
Self::rejig_prime(&members);

Self::deposit_event(Event::MemberRemoved);
Ok(())
Ok(Some(T::WeightInfo::remove_member(init_length as u32)).into())
}

/// Swap out one member `remove` for another `add`.
Expand All @@ -210,18 +219,18 @@ pub mod pallet {
///
/// Prime membership is *not* passed from `remove` to `add`, if extant.
#[pallet::call_index(2)]
#[pallet::weight({50_000_000})]
#[pallet::weight(T::WeightInfo::swap_member(T::MaxMembers::get()))]
pub fn swap_member(
origin: OriginFor<T>,
remove: AccountIdLookupOf<T>,
add: AccountIdLookupOf<T>,
) -> DispatchResult {
) -> DispatchResultWithPostInfo {
T::SwapOrigin::ensure_origin(origin)?;
let remove = T::Lookup::lookup(remove)?;
let add = T::Lookup::lookup(add)?;

if remove == add {
return Ok(())
return Ok(().into());
}

let mut members = <Members<T, I>>::get();
Expand All @@ -236,15 +245,15 @@ pub mod pallet {
Self::rejig_prime(&members);

Self::deposit_event(Event::MembersSwapped);
Ok(())
Ok(Some(T::WeightInfo::swap_member(members.len() as u32)).into())
}

/// Change the membership to a new set, disregarding the existing membership. Be nice and
/// pass `members` pre-sorted.
///
/// May only be called from `T::ResetOrigin`.
#[pallet::call_index(3)]
#[pallet::weight({50_000_000})]
#[pallet::weight(T::WeightInfo::reset_members(members.len().unique_saturated_into()))]
pub fn reset_members(origin: OriginFor<T>, members: Vec<T::AccountId>) -> DispatchResult {
T::ResetOrigin::ensure_origin(origin)?;

Expand All @@ -267,56 +276,65 @@ pub mod pallet {
///
/// Prime membership is passed from the origin account to `new`, if extant.
#[pallet::call_index(4)]
#[pallet::weight({50_000_000})]
pub fn change_key(origin: OriginFor<T>, new: AccountIdLookupOf<T>) -> DispatchResult {
#[pallet::weight(T::WeightInfo::change_key(T::MaxMembers::get()))]
pub fn change_key(
origin: OriginFor<T>,
new: AccountIdLookupOf<T>,
) -> DispatchResultWithPostInfo {
let remove = ensure_signed(origin)?;
let new = T::Lookup::lookup(new)?;

if remove != new {
let mut members = <Members<T, I>>::get();
let location =
members.binary_search(&remove).ok().ok_or(Error::<T, I>::NotMember)?;
let _ = members.binary_search(&new).err().ok_or(Error::<T, I>::AlreadyMember)?;
members[location] = new.clone();
members.sort();

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

T::MembershipChanged::change_members_sorted(
&[new.clone()],
&[remove.clone()],
&members[..],
);

if Prime::<T, I>::get() == Some(remove) {
Prime::<T, I>::put(&new);
T::MembershipChanged::set_prime(Some(new));
}
if remove == new {
return Ok(().into());
}

let mut members = <Members<T, I>>::get();
let members_length = members.len() as u32;
let location = members.binary_search(&remove).ok().ok_or(Error::<T, I>::NotMember)?;
let _ = members.binary_search(&new).err().ok_or(Error::<T, I>::AlreadyMember)?;
members[location] = new.clone();
members.sort();

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

T::MembershipChanged::change_members_sorted(
&[new.clone()],
&[remove.clone()],
&members[..],
);

if Prime::<T, I>::get() == Some(remove) {
Prime::<T, I>::put(&new);
T::MembershipChanged::set_prime(Some(new));
}

Self::deposit_event(Event::KeyChanged);
Ok(())
Ok(Some(T::WeightInfo::change_key(members_length)).into())
}

/// Set the prime member. Must be a current member.
///
/// May only be called from `T::PrimeOrigin`.
#[pallet::call_index(5)]
#[pallet::weight({50_000_000})]
pub fn set_prime(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
#[pallet::weight(T::WeightInfo::set_prime(T::MaxMembers::get()))]
pub fn set_prime(
origin: OriginFor<T>,
who: AccountIdLookupOf<T>,
) -> DispatchResultWithPostInfo {
T::PrimeOrigin::ensure_origin(origin)?;
let who = T::Lookup::lookup(who)?;
Self::members().binary_search(&who).ok().ok_or(Error::<T, I>::NotMember)?;
let members = Self::members();
members.binary_search(&who).ok().ok_or(Error::<T, I>::NotMember)?;
Prime::<T, I>::put(&who);
T::MembershipChanged::set_prime(Some(who));
Ok(())
Ok(Some(T::WeightInfo::set_prime(members.len() as u32)).into())
}

/// Remove the prime member if it exists.
///
/// May only be called from `T::PrimeOrigin`.
#[pallet::call_index(6)]
#[pallet::weight({50_000_000})]
#[pallet::weight(T::WeightInfo::clear_prime())]
pub fn clear_prime(origin: OriginFor<T>) -> DispatchResult {
T::PrimeOrigin::ensure_origin(origin)?;
Prime::<T, I>::kill();
Expand Down Expand Up @@ -442,7 +460,7 @@ mod benchmark {
}

// er keep the prime common between incoming and outgoing to make sure it is rejigged.
reset_member {
reset_members {
let m in 1 .. T::MaxMembers::get();

let members = (1..m+1).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
Expand Down Expand Up @@ -500,8 +518,7 @@ mod benchmark {
}

clear_prime {
let m in 1 .. T::MaxMembers::get();
let members = (0..m).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
let members = (0..T::MaxMembers::get()).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
let prime = members.last().cloned().unwrap();
set_members::<T, I>(members, None);
}: {
Expand All @@ -526,7 +543,8 @@ mod tests {
use sp_runtime::{bounded_vec, traits::BadOrigin, BuildStorage};

use frame_support::{
assert_noop, assert_ok, derive_impl, ord_parameter_types, parameter_types,
assert_noop, assert_ok, assert_storage_noop, derive_impl, ord_parameter_types,
parameter_types,
traits::{ConstU32, StorageVersion},
};
use frame_system::EnsureSignedBy;
Expand Down Expand Up @@ -716,6 +734,17 @@ mod tests {
});
}

#[test]
fn swap_member_with_identical_arguments_changes_nothing() {
new_test_ext().execute_with(|| {
assert_storage_noop!(assert_ok!(Membership::swap_member(
RuntimeOrigin::signed(3),
10,
10
)));
});
}

#[test]
fn change_key_works() {
new_test_ext().execute_with(|| {
Expand Down Expand Up @@ -745,6 +774,13 @@ mod tests {
});
}

#[test]
fn change_key_with_same_caller_as_argument_changes_nothing() {
new_test_ext().execute_with(|| {
assert_storage_noop!(assert_ok!(Membership::change_key(RuntimeOrigin::signed(10), 10)));
});
}

#[test]
fn reset_members_works() {
new_test_ext().execute_with(|| {
Expand Down
18 changes: 6 additions & 12 deletions substrate/frame/membership/src/weights.rs
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 5fc6d67

Please sign in to comment.