pallet-membership weights (#3324)

## 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>

prdoc/pr_3324.prdoc

@@ -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

substrate/frame/membership/src/lib.rs

@@ -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;
@@ -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())
@@ -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);
 
@@ -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`.
@@ -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();
@@ -236,7 +245,7 @@ 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
@@ -244,7 +253,7 @@ pub mod pallet {
 		///
 		/// 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)?;
 
@@ -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();
+			if remove == new {
+				return Ok(().into());
+			}
 
-				<Members<T, I>>::put(&members);
+			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();
 
-				T::MembershipChanged::change_members_sorted(
-					&[new.clone()],
-					&[remove.clone()],
-					&members[..],
-				);
+			<Members<T, I>>::put(&members);
 
-				if Prime::<T, I>::get() == Some(remove) {
-					Prime::<T, I>::put(&new);
-					T::MembershipChanged::set_prime(Some(new));
-				}
+			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();
@@ -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>>();
@@ -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);
 		}: {
@@ -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;
@@ -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(|| {
@@ -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(|| {

substrate/frame/membership/src/weights.rs

@@ -55,10 +55,10 @@ pub trait WeightInfo {
 	fn add_member(m: u32, ) -> Weight;
 	fn remove_member(m: u32, ) -> Weight;
 	fn swap_member(m: u32, ) -> Weight;
-	fn reset_member(m: u32, ) -> Weight;
+	fn reset_members(m: u32, ) -> Weight;
 	fn change_key(m: u32, ) -> Weight;
 	fn set_prime(m: u32, ) -> Weight;
-	fn clear_prime(m: u32, ) -> Weight;
+	fn clear_prime() -> Weight;
 }
 
 /// Weights for pallet_membership using the Substrate node and recommended hardware.
@@ -142,7 +142,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	/// Storage: TechnicalCommittee Prime (r:0 w:1)
 	/// Proof Skipped: TechnicalCommittee Prime (max_values: Some(1), max_size: None, mode: Measured)
 	/// The range of component `m` is `[1, 100]`.
-	fn reset_member(m: u32, ) -> Weight {
+	fn reset_members(m: u32, ) -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `312 + m * (64 ±0)`
 		//  Estimated: `4687 + m * (64 ±0)`
@@ -200,15 +200,12 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	/// Proof: TechnicalMembership Prime (max_values: Some(1), max_size: Some(32), added: 527, mode: MaxEncodedLen)
 	/// Storage: TechnicalCommittee Prime (r:0 w:1)
 	/// Proof Skipped: TechnicalCommittee Prime (max_values: Some(1), max_size: None, mode: Measured)
-	/// The range of component `m` is `[1, 100]`.
-	fn clear_prime(m: u32, ) -> Weight {
+	fn clear_prime() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `0`
 		// Minimum execution time: 3_373_000 picoseconds.
 		Weight::from_parts(3_750_452, 0)
-			// Standard Error: 142
-			.saturating_add(Weight::from_parts(505, 0).saturating_mul(m.into()))
 			.saturating_add(T::DbWeight::get().writes(2_u64))
 	}
 }
@@ -293,7 +290,7 @@ impl WeightInfo for () {
 	/// Storage: TechnicalCommittee Prime (r:0 w:1)
 	/// Proof Skipped: TechnicalCommittee Prime (max_values: Some(1), max_size: None, mode: Measured)
 	/// The range of component `m` is `[1, 100]`.
-	fn reset_member(m: u32, ) -> Weight {
+	fn reset_members(m: u32, ) -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `312 + m * (64 ±0)`
 		//  Estimated: `4687 + m * (64 ±0)`
@@ -351,15 +348,12 @@ impl WeightInfo for () {
 	/// Proof: TechnicalMembership Prime (max_values: Some(1), max_size: Some(32), added: 527, mode: MaxEncodedLen)
 	/// Storage: TechnicalCommittee Prime (r:0 w:1)
 	/// Proof Skipped: TechnicalCommittee Prime (max_values: Some(1), max_size: None, mode: Measured)
-	/// The range of component `m` is `[1, 100]`.
-	fn clear_prime(m: u32, ) -> Weight {
+	fn clear_prime() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `0`
 		// Minimum execution time: 3_373_000 picoseconds.
 		Weight::from_parts(3_750_452, 0)
-			// Standard Error: 142
-			.saturating_add(Weight::from_parts(505, 0).saturating_mul(m.into()))
 			.saturating_add(RocksDbWeight::get().writes(2_u64))
 	}
 }