NoOp Impl Polling Trait (#5311)

Adds NoOp implementation for the `Polling` trait and updates benchmarks
in `pallet-ranked-collective`.

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
(cherry picked from commit 9f603b1a)

prdoc/pr_5311.prdoc

+title: No-op Impl Polling Trait
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Provide a NoOp implementation of the Polling trait for unit where the trait is defined and skiping benchmarks that necessitate it's definition.
+
+crates:
+  - name: pallet-core-fellowship
+    bump: minor
+  - name: pallet-ranked-collective
+    bump: minor
+  - name: pallet-salary
+    bump: minor
+  - name: frame-support
+    bump: minor

substrate/frame/core-fellowship/src/tests/integration.rs

@@ -21,15 +21,15 @@ use frame_support::{
 	assert_noop, assert_ok, derive_impl, hypothetically, ord_parameter_types,
 	pallet_prelude::Weight,
 	parameter_types,
-	traits::{ConstU16, EitherOf, IsInVec, MapSuccess, PollStatus, Polling, TryMapSuccess},
+	traits::{ConstU16, EitherOf, IsInVec, MapSuccess, NoOpPoll, TryMapSuccess},
 };
 use frame_system::EnsureSignedBy;
-use pallet_ranked_collective::{EnsureRanked, Geometric, Rank, TallyOf, Votes};
+use pallet_ranked_collective::{EnsureRanked, Geometric, Rank};
 use sp_core::{ConstU32, Get};
 use sp_runtime::{
 	bounded_vec,
 	traits::{Convert, ReduceBy, ReplaceWithDefault, TryMorphInto},
-	BuildStorage, DispatchError,
+	BuildStorage,
 };
 type Class = Rank;
 
@@ -83,45 +83,6 @@ impl Config for Test {
 	type MaxRank = ConstU32<9>;
 }
 
-pub struct TestPolls;
-impl Polling<TallyOf<Test>> for TestPolls {
-	type Index = u8;
-	type Votes = Votes;
-	type Moment = u64;
-	type Class = Class;
-
-	fn classes() -> Vec<Self::Class> {
-		unimplemented!()
-	}
-	fn as_ongoing(_: u8) -> Option<(TallyOf<Test>, Self::Class)> {
-		unimplemented!()
-	}
-	fn access_poll<R>(
-		_: Self::Index,
-		_: impl FnOnce(PollStatus<&mut TallyOf<Test>, Self::Moment, Self::Class>) -> R,
-	) -> R {
-		unimplemented!()
-	}
-	fn try_access_poll<R>(
-		_: Self::Index,
-		_: impl FnOnce(
-			PollStatus<&mut TallyOf<Test>, Self::Moment, Self::Class>,
-		) -> Result<R, DispatchError>,
-	) -> Result<R, DispatchError> {
-		unimplemented!()
-	}
-
-	#[cfg(feature = "runtime-benchmarks")]
-	fn create_ongoing(_: Self::Class) -> Result<Self::Index, ()> {
-		unimplemented!()
-	}
-
-	#[cfg(feature = "runtime-benchmarks")]
-	fn end_ongoing(_: Self::Index, _: bool) -> Result<(), ()> {
-		unimplemented!()
-	}
-}
-
 /// Convert the tally class into the minimum rank required to vote on the poll.
 /// MinRank(Class) = Class - Delta
 pub struct MinRankOfClass<Delta>(PhantomData<Delta>);
@@ -154,7 +115,7 @@ impl pallet_ranked_collective::Config for Test {
 		// Members can exchange up to the rank of 2 below them.
 		MapSuccess<EnsureRanked<Test, (), 2>, ReduceBy<ConstU16<2>>>,
 	>;
-	type Polls = TestPolls;
+	type Polls = NoOpPoll;
 	type MinRankOfClass = MinRankOfClass<MinRankOfClassDelta>;
 	type MemberSwappedHandler = CoreFellowship;
 	type VoteWeight = Geometric;

substrate/frame/ranked-collective/src/benchmarking.rs

@@ -21,11 +21,12 @@ use super::*;
 #[allow(unused_imports)]
 use crate::Pallet as RankedCollective;
 use alloc::vec::Vec;
-
-use frame_benchmarking::v1::{
-	account, benchmarks_instance_pallet, whitelisted_caller, BenchmarkError,
+use frame_benchmarking::{
+	v1::{account, BenchmarkError},
+	v2::*,
 };
-use frame_support::{assert_ok, traits::UnfilteredDispatchable};
+
+use frame_support::{assert_err, assert_ok, traits::NoOpPoll};
 use frame_system::RawOrigin as SystemOrigin;
 
 const SEED: u32 = 0;
@@ -56,131 +57,273 @@ fn make_member<T: Config<I>, I: 'static>(rank: Rank) -> T::AccountId {
 	who
 }
 
-benchmarks_instance_pallet! {
-	add_member {
+#[instance_benchmarks(
+where <<T as pallet::Config<I>>::Polls as frame_support::traits::Polling<Tally<T, I, pallet::Pallet<T, I>>>>::Index: From<u8>
+)]
+mod benchmarks {
+	use super::*;
+
+	#[benchmark]
+	fn add_member() -> Result<(), BenchmarkError> {
+		// Generate a test account for the new member.
 		let who = account::<T::AccountId>("member", 0, SEED);
 		let who_lookup = T::Lookup::unlookup(who.clone());
+
+		// Attempt to get the successful origin for adding a member.
 		let origin =
 			T::AddOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
-		let call = Call::<T, I>::add_member { who: who_lookup };
-	}: { call.dispatch_bypass_filter(origin)? }
-	verify {
+
+		#[extrinsic_call]
+		_(origin as T::RuntimeOrigin, who_lookup);
+
+		// Ensure the member count has increased (or is 1 for rank 0).
 		assert_eq!(MemberCount::<T, I>::get(0), 1);
+
+		// Check that the correct event was emitted.
 		assert_last_event::<T, I>(Event::MemberAdded { who }.into());
+
+		Ok(())
 	}
 
-	remove_member {
-		let r in 0 .. 10;
+	#[benchmark]
+	fn remove_member(r: Linear<0, 10>) -> Result<(), BenchmarkError> {
+		// Convert `r` to a rank and create members.
 		let rank = r as u16;
-		let first = make_member::<T, I>(rank);
 		let who = make_member::<T, I>(rank);
 		let who_lookup = T::Lookup::unlookup(who.clone());
 		let last = make_member::<T, I>(rank);
-		let last_index = (0..=rank).map(|r| IdToIndex::<T, I>::get(r, &last).unwrap()).collect::<Vec<_>>();
+
+		// Collect the index of the `last` member for each rank.
+		let last_index: Vec<_> =
+			(0..=rank).map(|r| IdToIndex::<T, I>::get(r, &last).unwrap()).collect();
+
+		// Fetch the remove origin.
 		let origin =
 			T::RemoveOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
-		let call = Call::<T, I>::remove_member { who: who_lookup, min_rank: rank };
-	}: { call.dispatch_bypass_filter(origin)? }
-	verify {
+
+		#[extrinsic_call]
+		_(origin as T::RuntimeOrigin, who_lookup, rank);
+
 		for r in 0..=rank {
-			assert_eq!(MemberCount::<T, I>::get(r), 2);
+			assert_eq!(MemberCount::<T, I>::get(r), 1);
 			assert_ne!(last_index[r as usize], IdToIndex::<T, I>::get(r, &last).unwrap());
 		}
+
+		// Ensure the correct event was emitted for the member removal.
 		assert_last_event::<T, I>(Event::MemberRemoved { who, rank }.into());
+
+		Ok(())
 	}
 
-	promote_member {
-		let r in 0 .. 10;
+	#[benchmark]
+	fn promote_member(r: Linear<0, 10>) -> Result<(), BenchmarkError> {
+		// Convert `r` to a rank and create the member.
 		let rank = r as u16;
 		let who = make_member::<T, I>(rank);
 		let who_lookup = T::Lookup::unlookup(who.clone());
+
+		// Try to fetch the promotion origin.
 		let origin =
 			T::PromoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
-		let call = Call::<T, I>::promote_member { who: who_lookup };
-	}: { call.dispatch_bypass_filter(origin)? }
-	verify {
+
+		#[extrinsic_call]
+		_(origin as T::RuntimeOrigin, who_lookup);
+
+		// Ensure the member's rank has increased by 1.
 		assert_eq!(Members::<T, I>::get(&who).unwrap().rank, rank + 1);
+
+		// Ensure the correct event was emitted for the rank change.
 		assert_last_event::<T, I>(Event::RankChanged { who, rank: rank + 1 }.into());
+
+		Ok(())
 	}
 
-	demote_member {
-		let r in 0 .. 10;
+	#[benchmark]
+	fn demote_member(r: Linear<0, 10>) -> Result<(), BenchmarkError> {
+		// Convert `r` to a rank and create necessary members for the benchmark.
 		let rank = r as u16;
-		let first = make_member::<T, I>(rank);
 		let who = make_member::<T, I>(rank);
 		let who_lookup = T::Lookup::unlookup(who.clone());
 		let last = make_member::<T, I>(rank);
+
+		// Get the last index for the member.
 		let last_index = IdToIndex::<T, I>::get(rank, &last).unwrap();
+
+		// Try to fetch the demotion origin.
 		let origin =
 			T::DemoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
-		let call = Call::<T, I>::demote_member { who: who_lookup };
-	}: { call.dispatch_bypass_filter(origin)? }
-	verify {
+
+		#[extrinsic_call]
+		_(origin as T::RuntimeOrigin, who_lookup);
+
+		// Ensure the member's rank has decreased by 1.
 		assert_eq!(Members::<T, I>::get(&who).map(|x| x.rank), rank.checked_sub(1));
-		assert_eq!(MemberCount::<T, I>::get(rank), 2);
+
+		// Ensure the member count remains as expected.
+		assert_eq!(MemberCount::<T, I>::get(rank), 1);
+
+		// Ensure the index of the last member has changed.
 		assert_ne!(last_index, IdToIndex::<T, I>::get(rank, &last).unwrap());
-		assert_last_event::<T, I>(match rank {
-			0 => Event::MemberRemoved { who, rank: 0 },
-			r => Event::RankChanged { who, rank: r - 1 },
-		}.into());
+
+		// Ensure the correct event was emitted depending on the member's rank.
+		assert_last_event::<T, I>(
+			match rank {
+				0 => Event::MemberRemoved { who, rank: 0 },
+				r => Event::RankChanged { who, rank: r - 1 },
+			}
+			.into(),
+		);
+
+		Ok(())
 	}
 
-	vote {
-		let class = T::Polls::classes().into_iter().next().unwrap();
-		let rank = T::MinRankOfClass::convert(class.clone());
+	#[benchmark]
+	fn vote() -> Result<(), BenchmarkError> {
+		// Get the first available class or set it to None if no class exists.
+		let class = T::Polls::classes().into_iter().next();
+
+		// Convert the class to a rank if it exists, otherwise use the default rank.
+		let rank = class.as_ref().map_or(
+			<Pallet<T, I> as frame_support::traits::RankedMembers>::Rank::default(),
+			|class| T::MinRankOfClass::convert(class.clone()),
+		);
 
+		// Create a caller based on the rank.
 		let caller = make_member::<T, I>(rank);
-		let caller_lookup = T::Lookup::unlookup(caller.clone());
 
-		let poll = T::Polls::create_ongoing(class).expect("Must always be able to create a poll for rank 0");
+		// Determine the poll to use: create an ongoing poll if class exists, or use an invalid
+		// poll.
+		let poll = if let Some(ref class) = class {
+			T::Polls::create_ongoing(class.clone())
+				.expect("Poll creation should succeed for rank 0")
+		} else {
+			<NoOpPoll as Polling<T>>::Index::MAX.into()
+		};
+
+		// Benchmark the vote logic for a positive vote (true).
+		#[block]
+		{
+			let vote_result =
+				Pallet::<T, I>::vote(SystemOrigin::Signed(caller.clone()).into(), poll, true);
+
+			// If the class exists, expect success; otherwise expect a "NotPolling" error.
+			if class.is_some() {
+				assert_ok!(vote_result);
+			} else {
+				assert_err!(vote_result, crate::Error::<T, I>::NotPolling);
+			};
+		}
+
+		// Vote logic for a negative vote (false).
+		let vote_result =
+			Pallet::<T, I>::vote(SystemOrigin::Signed(caller.clone()).into(), poll, false);
+
+		// Check the result of the negative vote.
+		if class.is_some() {
+			assert_ok!(vote_result);
+		} else {
+			assert_err!(vote_result, crate::Error::<T, I>::NotPolling);
+		};
+
+		// If the class exists, verify the vote event and tally.
+		if let Some(_) = class {
+			let tally = Tally::from_parts(0, 0, 1);
+			let vote_event = Event::Voted { who: caller, poll, vote: VoteRecord::Nay(1), tally };
+			assert_last_event::<T, I>(vote_event.into());
+		}
 
-		// Vote once.
-		assert_ok!(Pallet::<T, I>::vote(SystemOrigin::Signed(caller.clone()).into(), poll, true));
-	}: _(SystemOrigin::Signed(caller.clone()), poll, false)
-	verify {
-		let tally = Tally::from_parts(0, 0, 1);
-		let ev = Event::Voted { who: caller, poll, vote: VoteRecord::Nay(1), tally };
-		assert_last_event::<T, I>(ev.into());
+		Ok(())
 	}
 
-	cleanup_poll {
-		let n in 0 .. 100;
+	#[benchmark]
+	fn cleanup_poll(n: Linear<0, 100>) -> Result<(), BenchmarkError> {
+		let alice: T::AccountId = whitelisted_caller();
+		let origin = SystemOrigin::Signed(alice.clone());
+
+		// Try to retrieve the first class if it exists.
+		let class = T::Polls::classes().into_iter().next();
+
+		// Convert the class to a rank, or use a default rank if no class exists.
+		let rank = class.as_ref().map_or(
+			<Pallet<T, I> as frame_support::traits::RankedMembers>::Rank::default(),
+			|class| T::MinRankOfClass::convert(class.clone()),
+		);
 
-		// Create a poll
-		let class = T::Polls::classes().into_iter().next().unwrap();
-		let rank = T::MinRankOfClass::convert(class.clone());
-		let poll = T::Polls::create_ongoing(class).expect("Must always be able to create a poll");
+		// Determine the poll to use: create an ongoing poll if class exists, or use an invalid
+		// poll.
+		let poll = if let Some(ref class) = class {
+			T::Polls::create_ongoing(class.clone())
+				.expect("Poll creation should succeed for rank 0")
+		} else {
+			<NoOpPoll as Polling<T>>::Index::MAX.into()
+		};
 
-		// Vote in the poll by each of `n` members
-		for i in 0..n {
-			let who = make_member::<T, I>(rank);
-			assert_ok!(Pallet::<T, I>::vote(SystemOrigin::Signed(who).into(), poll, true));
+		// Simulate voting by `n` members.
+		for _ in 0..n {
+			let voter = make_member::<T, I>(rank);
+			let result = Pallet::<T, I>::vote(SystemOrigin::Signed(voter).into(), poll, true);
+
+			// Check voting results based on class existence.
+			if class.is_some() {
+				assert_ok!(result);
+			} else {
+				assert_err!(result, crate::Error::<T, I>::NotPolling);
+			}
+		}
+
+		// End the poll if the class exists.
+		if class.is_some() {
+			T::Polls::end_ongoing(poll, false)
+				.map_err(|_| BenchmarkError::Stop("Failed to end poll"))?;
 		}
 
-		// End the poll.
-		T::Polls::end_ongoing(poll, false).expect("Must always be able to end a poll");
+		// Verify the number of votes cast.
+		let expected_votes = if class.is_some() { n as usize } else { 0 };
+		assert_eq!(Voting::<T, I>::iter_prefix(poll).count(), expected_votes);
 
-		assert_eq!(Voting::<T, I>::iter_prefix(poll).count(), n as usize);
-	}: _(SystemOrigin::Signed(whitelisted_caller()), poll, n)
-	verify {
+		// Benchmark the cleanup function.
+		#[extrinsic_call]
+		_(origin, poll, n);
+
+		// Ensure all votes are cleaned up after the extrinsic call.
 		assert_eq!(Voting::<T, I>::iter().count(), 0);
+
+		Ok(())
 	}
 
-	exchange_member {
+	#[benchmark]
+	fn exchange_member() -> Result<(), BenchmarkError> {
+		// Create an existing member.
 		let who = make_member::<T, I>(1);
 		T::BenchmarkSetup::ensure_member(&who);
 		let who_lookup = T::Lookup::unlookup(who.clone());
+
+		// Create a new account for the new member.
 		let new_who = account::<T::AccountId>("new-member", 0, SEED);
 		let new_who_lookup = T::Lookup::unlookup(new_who.clone());
+
+		// Attempt to get the successful origin for exchanging a member.
 		let origin =
 			T::ExchangeOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
-		let call = Call::<T, I>::exchange_member { who: who_lookup, new_who: new_who_lookup };
-	}: { call.dispatch_bypass_filter(origin)? }
-	verify {
+
+		#[extrinsic_call]
+		_(origin as T::RuntimeOrigin, who_lookup, new_who_lookup);
+
+		// Check that the new member was successfully exchanged and holds the correct rank.
 		assert_eq!(Members::<T, I>::get(&new_who).unwrap().rank, 1);
+
+		// Ensure the old member no longer exists.
 		assert_eq!(Members::<T, I>::get(&who), None);
+
+		// Ensure the correct event was emitted.
 		assert_has_event::<T, I>(Event::MemberExchanged { who, new_who }.into());
+
+		Ok(())
 	}
 
-	impl_benchmark_test_suite!(RankedCollective, crate::tests::ExtBuilder::default().build(), crate::tests::Test);
+	impl_benchmark_test_suite!(
+		RankedCollective,
+		crate::tests::ExtBuilder::default().build(),
+		crate::tests::Test
+	);
 }

substrate/frame/salary/src/tests/integration.rs

@@ -17,22 +17,21 @@
 
 //! The crate's tests.
 
+use crate as pallet_salary;
+use crate::*;
 use frame_support::{
 	assert_noop, assert_ok, derive_impl, hypothetically,
 	pallet_prelude::Weight,
 	parameter_types,
-	traits::{ConstU64, EitherOf, MapSuccess, PollStatus, Polling},
+	traits::{ConstU64, EitherOf, MapSuccess, NoOpPoll},
 };
-use pallet_ranked_collective::{EnsureRanked, Geometric, TallyOf, Votes};
+use pallet_ranked_collective::{EnsureRanked, Geometric};
 use sp_core::{ConstU16, Get};
 use sp_runtime::{
 	traits::{Convert, ReduceBy, ReplaceWithDefault},
-	BuildStorage, DispatchError,
+	BuildStorage,
 };
 
-use crate as pallet_salary;
-use crate::*;
-
 type Rank = u16;
 type Block = frame_system::mocking::MockBlock<Test>;
 
@@ -55,45 +54,6 @@ impl frame_system::Config for Test {
 	type Block = Block;
 }
 
-pub struct TestPolls;
-impl Polling<TallyOf<Test>> for TestPolls {
-	type Index = u8;
-	type Votes = Votes;
-	type Moment = u64;
-	type Class = Rank;
-
-	fn classes() -> Vec<Self::Class> {
-		unimplemented!()
-	}
-	fn as_ongoing(_index: u8) -> Option<(TallyOf<Test>, Self::Class)> {
-		unimplemented!()
-	}
-	fn access_poll<R>(
-		_index: Self::Index,
-		_f: impl FnOnce(PollStatus<&mut TallyOf<Test>, Self::Moment, Self::Class>) -> R,
-	) -> R {
-		unimplemented!()
-	}
-	fn try_access_poll<R>(
-		_index: Self::Index,
-		_f: impl FnOnce(
-			PollStatus<&mut TallyOf<Test>, Self::Moment, Self::Class>,
-		) -> Result<R, DispatchError>,
-	) -> Result<R, DispatchError> {
-		unimplemented!()
-	}
-
-	#[cfg(feature = "runtime-benchmarks")]
-	fn create_ongoing(_class: Self::Class) -> Result<Self::Index, ()> {
-		unimplemented!()
-	}
-
-	#[cfg(feature = "runtime-benchmarks")]
-	fn end_ongoing(_index: Self::Index, _approved: bool) -> Result<(), ()> {
-		unimplemented!()
-	}
-}
-
 pub struct MinRankOfClass<Delta>(PhantomData<Delta>);
 impl<Delta: Get<Rank>> Convert<u16, Rank> for MinRankOfClass<Delta> {
 	fn convert(a: u16) -> Rank {
@@ -176,7 +136,7 @@ impl pallet_ranked_collective::Config for Test {
 		// Members can exchange up to the rank of 2 below them.
 		MapSuccess<EnsureRanked<Test, (), 2>, ReduceBy<ConstU16<2>>>,
 	>;
-	type Polls = TestPolls;
+	type Polls = NoOpPoll;
 	type MinRankOfClass = MinRankOfClass<MinRankOfClassDelta>;
 	type MemberSwappedHandler = Salary;
 	type VoteWeight = Geometric;

substrate/frame/support/src/traits.rs

@@ -110,7 +110,7 @@ pub use dispatch::{
 };
 
 mod voting;
-pub use voting::{ClassCountOf, PollStatus, Polling, VoteTally};
+pub use voting::{ClassCountOf, NoOpPoll, PollStatus, Polling, VoteTally};
 
 mod preimages;
 pub use preimages::{Bounded, BoundedInline, FetchResult, QueryPreimage, StorePreimage};

substrate/frame/support/src/traits/voting.rs

@@ -19,7 +19,7 @@
 //! votes.
 
 use crate::dispatch::Parameter;
-use alloc::vec::Vec;
+use alloc::{vec, vec::Vec};
 use codec::{HasCompact, MaxEncodedLen};
 use sp_arithmetic::Perbill;
 use sp_runtime::{traits::Member, DispatchError};
@@ -126,3 +126,49 @@ pub trait Polling<Tally> {
 		(Self::classes().into_iter().next().expect("Always one class"), u32::max_value())
 	}
 }
+
+/// NoOp polling is required if pallet-referenda functionality not needed.
+pub struct NoOpPoll;
+impl<Tally> Polling<Tally> for NoOpPoll {
+	type Index = u8;
+	type Votes = u32;
+	type Class = u16;
+	type Moment = u64;
+
+	fn classes() -> Vec<Self::Class> {
+		vec![]
+	}
+
+	fn as_ongoing(_index: Self::Index) -> Option<(Tally, Self::Class)> {
+		None
+	}
+
+	fn access_poll<R>(
+		_index: Self::Index,
+		f: impl FnOnce(PollStatus<&mut Tally, Self::Moment, Self::Class>) -> R,
+	) -> R {
+		f(PollStatus::None)
+	}
+
+	fn try_access_poll<R>(
+		_index: Self::Index,
+		f: impl FnOnce(PollStatus<&mut Tally, Self::Moment, Self::Class>) -> Result<R, DispatchError>,
+	) -> Result<R, DispatchError> {
+		f(PollStatus::None)
+	}
+
+	#[cfg(feature = "runtime-benchmarks")]
+	fn create_ongoing(_class: Self::Class) -> Result<Self::Index, ()> {
+		Err(())
+	}
+
+	#[cfg(feature = "runtime-benchmarks")]
+	fn end_ongoing(_index: Self::Index, _approved: bool) -> Result<(), ()> {
+		Err(())
+	}
+
+	#[cfg(feature = "runtime-benchmarks")]
+	fn max_ongoing() -> (Self::Class, u32) {
+		(0, 0)
+	}
+}