diff --git a/prdoc/pr_5311.prdoc b/prdoc/pr_5311.prdoc new file mode 100644 index 000000000000..b800a20774fd --- /dev/null +++ b/prdoc/pr_5311.prdoc @@ -0,0 +1,18 @@ +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 + - name: pallet-referenda + bump: minor diff --git a/substrate/frame/core-fellowship/src/tests/integration.rs b/substrate/frame/core-fellowship/src/tests/integration.rs index bcf70c7beb10..a41534cf81ba 100644 --- a/substrate/frame/core-fellowship/src/tests/integration.rs +++ b/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, 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> for TestPolls { - type Index = u8; - type Votes = Votes; - type Moment = u64; - type Class = Class; - - fn classes() -> Vec { - unimplemented!() - } - fn as_ongoing(_: u8) -> Option<(TallyOf, Self::Class)> { - unimplemented!() - } - fn access_poll( - _: Self::Index, - _: impl FnOnce(PollStatus<&mut TallyOf, Self::Moment, Self::Class>) -> R, - ) -> R { - unimplemented!() - } - fn try_access_poll( - _: Self::Index, - _: impl FnOnce( - PollStatus<&mut TallyOf, Self::Moment, Self::Class>, - ) -> Result, - ) -> Result { - unimplemented!() - } - - #[cfg(feature = "runtime-benchmarks")] - fn create_ongoing(_: Self::Class) -> Result { - 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(PhantomData); @@ -154,7 +115,7 @@ impl pallet_ranked_collective::Config for Test { // Members can exchange up to the rank of 2 below them. MapSuccess, ReduceBy>>, >; - type Polls = TestPolls; + type Polls = (); type MinRankOfClass = MinRankOfClass; type MemberSwappedHandler = CoreFellowship; type VoteWeight = Geometric; diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index dc7f4aaca773..5411f1e00c8b 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -21,11 +21,14 @@ use super::*; #[allow(unused_imports)] use crate::Pallet as RankedCollective; use alloc::vec::Vec; +use log::info; -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::UnfilteredDispatchable}; use frame_system::RawOrigin as SystemOrigin; const SEED: u32 = 0; @@ -56,131 +59,295 @@ fn make_member, I: 'static>(rank: Rank) -> T::AccountId { who } -benchmarks_instance_pallet! { - add_member { +#[instance_benchmarks] +mod benchmarks { + use super::*; + + #[benchmark] + fn add_member() -> Result<(), BenchmarkError> { + // Generate a test account for the new member. let who = account::("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)?; + + // Create the `Call` to add the member. let call = Call::::add_member { who: who_lookup }; - }: { call.dispatch_bypass_filter(origin)? } - verify { + + // Dispatch the call and measure execution. + #[block] + { + call.dispatch_bypass_filter(origin)?; + } + + // Ensure the member count has increased (or is 1 for rank 0). assert_eq!(MemberCount::::get(0), 1); - assert_last_event::(Event::MemberAdded { who }.into()); + + // Check that the correct event was emitted. + assert_last_event::(Event::MemberAdded { who: who.clone() }.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::(rank); + let _first = make_member::(rank); let who = make_member::(rank); let who_lookup = T::Lookup::unlookup(who.clone()); let last = make_member::(rank); - let last_index = (0..=rank).map(|r| IdToIndex::::get(r, &last).unwrap()).collect::>(); + + // Collect the index of the `last` member for each rank. + let last_index: Vec<_> = + (0..=rank).map(|r| IdToIndex::::get(r, &last).unwrap()).collect(); + + // Fetch the remove origin. let origin = T::RemoveOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; + + // Create the `Call` for removing the member. let call = Call::::remove_member { who: who_lookup, min_rank: rank }; - }: { call.dispatch_bypass_filter(origin)? } - verify { + + // Dispatch the call and measure its execution. + #[block] + { + call.dispatch_bypass_filter(origin)?; + } + for r in 0..=rank { assert_eq!(MemberCount::::get(r), 2); assert_ne!(last_index[r as usize], IdToIndex::::get(r, &last).unwrap()); } - assert_last_event::(Event::MemberRemoved { who, rank }.into()); + + // Ensure the correct event was emitted for the member removal. + assert_last_event::(Event::MemberRemoved { who: who.clone(), 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::(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)?; + + // Create the `Call` for promoting the member. let call = Call::::promote_member { who: who_lookup }; - }: { call.dispatch_bypass_filter(origin)? } - verify { + + // Dispatch the call and measure its execution. + #[block] + { + call.dispatch_bypass_filter(origin)?; + } + + // Ensure the member's rank has increased by 1. assert_eq!(Members::::get(&who).unwrap().rank, rank + 1); - assert_last_event::(Event::RankChanged { who, rank: rank + 1 }.into()); + + // Ensure the correct event was emitted for the rank change. + assert_last_event::(Event::RankChanged { who: who.clone(), 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::(rank); + let _first = make_member::(rank); let who = make_member::(rank); let who_lookup = T::Lookup::unlookup(who.clone()); let last = make_member::(rank); + + // Get the last index for the member. let last_index = IdToIndex::::get(rank, &last).unwrap(); + + // Try to fetch the demotion origin. let origin = T::DemoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; + + // Create the `Call` for demoting the member. let call = Call::::demote_member { who: who_lookup }; - }: { call.dispatch_bypass_filter(origin)? } - verify { + + // Dispatch the call and measure its execution. + #[block] + { + call.dispatch_bypass_filter(origin)?; + } + + // Ensure the member's rank has decreased by 1. assert_eq!(Members::::get(&who).map(|x| x.rank), rank.checked_sub(1)); + + // Ensure the member count remains as expected. assert_eq!(MemberCount::::get(rank), 2); + + // Ensure the index of the last member has changed. assert_ne!(last_index, IdToIndex::::get(rank, &last).unwrap()); - assert_last_event::(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::( + match rank { + 0 => Event::MemberRemoved { who: who.clone(), rank: 0 }, + r => Event::RankChanged { who: who.clone(), 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> { + // Find the first class or use a default one if none exists. + let (class_exists, class) = T::Polls::classes() + .into_iter() + .next() + .map(|c| (true, c)) + .unwrap_or_else(|| (false, Default::default())); + // Convert the class to a rank and create a caller based on that rank. + let rank = T::MinRankOfClass::convert(class.clone()); let caller = make_member::(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"); + // If the class exists, create an ongoing poll, otherwise use a default poll. + let poll = if class_exists { + T::Polls::create_ongoing(class.clone()) + .expect("Should always be able to create a poll for rank 0") + } else { + Default::default() + }; + + // Benchmark the vote logic. + #[block] + { + let vote_result = + Pallet::::vote(SystemOrigin::Signed(caller.clone()).into(), poll, true); + + // If the class exists, expect a successful vote, otherwise expect a `NotPolling` error. + if class_exists { + assert_ok!(vote_result); + } else { + info!("Expected error: {:?}", vote_result); + assert_err!(vote_result, crate::Error::::NotPolling); + } + } + + // Vote again with a different decision (false). + assert_ok!(Pallet::::vote(SystemOrigin::Signed(caller.clone()).into(), poll, false)); + + // If the class exists, verify the vote event and tally. + if class_exists { + let tally = Tally::from_parts(0, 0, 1); + let vote_event = Event::Voted { who: caller, poll, vote: VoteRecord::Nay(1), tally }; + assert_last_event::(vote_event.into()); + } - // Vote once. - assert_ok!(Pallet::::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::(ev.into()); + Ok(()) } - cleanup_poll { - let n in 0 .. 100; + #[benchmark] + fn cleanup_poll(n: Linear<0, 100>) -> Result<(), BenchmarkError> { + // Try to find an existing class or default to a new one. + let (class_exists, class) = T::Polls::classes() + .into_iter() + .next() + .map(|c| (true, c)) + .unwrap_or_else(|| (false, Default::default())); - // Create a poll - let class = T::Polls::classes().into_iter().next().unwrap(); + // Get rank and create a poll if the class exists. let rank = T::MinRankOfClass::convert(class.clone()); - let poll = T::Polls::create_ongoing(class).expect("Must always be able to create a poll"); + let poll = if class_exists { + T::Polls::create_ongoing(class.clone()) + .expect("Poll creation should succeed for rank 0") + } else { + Default::default() + }; - // Vote in the poll by each of `n` members - for i in 0..n { - let who = make_member::(rank); - assert_ok!(Pallet::::vote(SystemOrigin::Signed(who).into(), poll, true)); + // Simulate voting in the poll by `n` members. + for _ in 0..n { + let voter = make_member::(rank); + let result = Pallet::::vote(SystemOrigin::Signed(voter).into(), poll, true); + + // Check voting results based on class existence. + if class_exists { + assert_ok!(result); + } else { + assert_err!(result, crate::Error::::NotPolling); + } + } + + // End the poll if the class exists. + if class_exists { + T::Polls::end_ongoing(poll, false).expect("Poll should be able to end"); } - // 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_exists { n as usize } else { 0 }; + assert_eq!(Voting::::iter_prefix(poll).count(), expected_votes); - assert_eq!(Voting::::iter_prefix(poll).count(), n as usize); - }: _(SystemOrigin::Signed(whitelisted_caller()), poll, n) - verify { + // Benchmark the cleanup function. + #[block] + { + let _ = Pallet::::cleanup_poll( + SystemOrigin::Signed(whitelisted_caller()).into(), + poll, + n, + ); + } + + // Ensure all votes are cleaned up. assert_eq!(Voting::::iter().count(), 0); + Ok(()) } - exchange_member { + #[benchmark] + fn exchange_member() -> Result<(), BenchmarkError> { + // Create an existing member. let who = make_member::(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::("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)?; + + // Create the `Call` to exchange the member. let call = Call::::exchange_member { who: who_lookup, new_who: new_who_lookup }; - }: { call.dispatch_bypass_filter(origin)? } - verify { + + // Dispatch the call and measure execution. + #[block] + { + call.dispatch_bypass_filter(origin)?; + } + + // Check that the new member was successfully exchanged and holds the correct rank. assert_eq!(Members::::get(&new_who).unwrap().rank, 1); + + // Ensure the old member no longer exists. assert_eq!(Members::::get(&who), None); - assert_has_event::(Event::MemberExchanged { who, new_who }.into()); + + // Ensure the correct event was emitted. + assert_has_event::( + Event::MemberExchanged { who: who.clone(), new_who: new_who.clone() }.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 + ); } diff --git a/substrate/frame/referenda/src/types.rs b/substrate/frame/referenda/src/types.rs index 1039b288b2ae..878e0f30ce93 100644 --- a/substrate/frame/referenda/src/types.rs +++ b/substrate/frame/referenda/src/types.rs @@ -139,7 +139,7 @@ pub struct TrackInfo { /// Information on the voting tracks. pub trait TracksInfo { /// The identifier for a track. - type Id: Copy + Parameter + Ord + PartialOrd + Send + Sync + 'static + MaxEncodedLen; + type Id: Copy + Parameter + Ord + PartialOrd + Send + Sync + 'static + MaxEncodedLen + Default; /// The origin type from which a track is implied. type RuntimeOrigin; diff --git a/substrate/frame/salary/src/tests/integration.rs b/substrate/frame/salary/src/tests/integration.rs index 69f218943ade..cde81f8701ce 100644 --- a/substrate/frame/salary/src/tests/integration.rs +++ b/substrate/frame/salary/src/tests/integration.rs @@ -21,13 +21,13 @@ 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}, }; -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; @@ -55,45 +55,6 @@ impl frame_system::Config for Test { type Block = Block; } -pub struct TestPolls; -impl Polling> for TestPolls { - type Index = u8; - type Votes = Votes; - type Moment = u64; - type Class = Rank; - - fn classes() -> Vec { - unimplemented!() - } - fn as_ongoing(_index: u8) -> Option<(TallyOf, Self::Class)> { - unimplemented!() - } - fn access_poll( - _index: Self::Index, - _f: impl FnOnce(PollStatus<&mut TallyOf, Self::Moment, Self::Class>) -> R, - ) -> R { - unimplemented!() - } - fn try_access_poll( - _index: Self::Index, - _f: impl FnOnce( - PollStatus<&mut TallyOf, Self::Moment, Self::Class>, - ) -> Result, - ) -> Result { - unimplemented!() - } - - #[cfg(feature = "runtime-benchmarks")] - fn create_ongoing(_class: Self::Class) -> Result { - unimplemented!() - } - - #[cfg(feature = "runtime-benchmarks")] - fn end_ongoing(_index: Self::Index, _approved: bool) -> Result<(), ()> { - unimplemented!() - } -} - pub struct MinRankOfClass(PhantomData); impl> Convert for MinRankOfClass { fn convert(a: u16) -> Rank { @@ -176,7 +137,7 @@ impl pallet_ranked_collective::Config for Test { // Members can exchange up to the rank of 2 below them. MapSuccess, ReduceBy>>, >; - type Polls = TestPolls; + type Polls = (); type MinRankOfClass = MinRankOfClass; type MemberSwappedHandler = Salary; type VoteWeight = Geometric; diff --git a/substrate/frame/support/src/traits/voting.rs b/substrate/frame/support/src/traits/voting.rs index 958ef5dce6c1..8daccbff6900 100644 --- a/substrate/frame/support/src/traits/voting.rs +++ b/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}; @@ -82,9 +82,9 @@ impl> sp_runtime::traits::Get for ClassCountOf { } pub trait Polling { - type Index: Parameter + Member + Ord + PartialOrd + Copy + HasCompact + MaxEncodedLen; + type Index: Parameter + Member + Ord + PartialOrd + Copy + HasCompact + MaxEncodedLen + Default; type Votes: Parameter + Member + Ord + PartialOrd + Copy + HasCompact + MaxEncodedLen; - type Class: Parameter + Member + Ord + PartialOrd + MaxEncodedLen; + type Class: Parameter + Member + Ord + PartialOrd + MaxEncodedLen + Default; type Moment; /// Provides a vec of values that `T` may take. @@ -126,3 +126,47 @@ pub trait Polling { (Self::classes().into_iter().next().expect("Always one class"), u32::max_value()) } } + +impl Polling for () { + type Index = u32; + type Votes = u32; + type Class = u16; + type Moment = u64; + + fn classes() -> Vec { + vec![] + } + + fn as_ongoing(_index: Self::Index) -> Option<(Tally, Self::Class)> { + None + } + + fn access_poll( + _index: Self::Index, + f: impl FnOnce(PollStatus<&mut Tally, Self::Moment, Self::Class>) -> R, + ) -> R { + f(PollStatus::None) + } + + fn try_access_poll( + _index: Self::Index, + f: impl FnOnce(PollStatus<&mut Tally, Self::Moment, Self::Class>) -> Result, + ) -> Result { + f(PollStatus::None) + } + + #[cfg(feature = "runtime-benchmarks")] + fn create_ongoing(_class: Self::Class) -> Result { + 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) + } +}