From 1eedd898cd731f81a9571ad80448d8f33f207fa9 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Sat, 10 Aug 2024 17:06:55 +0100 Subject: [PATCH 01/17] salary & core-fellowship --- .../core-fellowship/src/tests/integration.rs | 33 +++++++++++-------- .../frame/salary/src/tests/integration.rs | 30 ++++++++++------- 2 files changed, 37 insertions(+), 26 deletions(-) diff --git a/substrate/frame/core-fellowship/src/tests/integration.rs b/substrate/frame/core-fellowship/src/tests/integration.rs index bcf70c7beb10..b50e9310e746 100644 --- a/substrate/frame/core-fellowship/src/tests/integration.rs +++ b/substrate/frame/core-fellowship/src/tests/integration.rs @@ -23,8 +23,8 @@ use frame_support::{ parameter_types, traits::{ConstU16, EitherOf, IsInVec, MapSuccess, PollStatus, Polling, TryMapSuccess}, }; -use frame_system::EnsureSignedBy; -use pallet_ranked_collective::{EnsureRanked, Geometric, Rank, TallyOf, Votes}; +use frame_system::{pallet_prelude::BlockNumberFor, EnsureSignedBy}; +use pallet_ranked_collective::{EnsureRanked, Geometric, MemberIndex, Rank, TallyOf, Votes}; use sp_core::{ConstU32, Get}; use sp_runtime::{ bounded_vec, @@ -85,40 +85,45 @@ impl Config for Test { pub struct TestPolls; impl Polling> for TestPolls { - type Index = u8; + type Index = MemberIndex; type Votes = Votes; - type Moment = u64; - type Class = Class; + type Moment = BlockNumberFor; + type Class = u16; fn classes() -> Vec { - unimplemented!() + vec![] } - fn as_ongoing(_: u8) -> Option<(TallyOf, Self::Class)> { - unimplemented!() + fn as_ongoing(_index: Self::Index) -> Option<(TallyOf, Self::Class)> { + None } fn access_poll( _: Self::Index, - _: impl FnOnce(PollStatus<&mut TallyOf, Self::Moment, Self::Class>) -> R, + f: impl FnOnce(PollStatus<&mut TallyOf, Self::Moment, Self::Class>) -> R, ) -> R { - unimplemented!() + f(PollStatus::None) } fn try_access_poll( _: Self::Index, - _: impl FnOnce( + f: impl FnOnce( PollStatus<&mut TallyOf, Self::Moment, Self::Class>, ) -> Result, ) -> Result { - unimplemented!() + f(PollStatus::None) } #[cfg(feature = "runtime-benchmarks")] fn create_ongoing(_: Self::Class) -> Result { - unimplemented!() + Err(()) } #[cfg(feature = "runtime-benchmarks")] fn end_ongoing(_: Self::Index, _: bool) -> Result<(), ()> { - unimplemented!() + Err(()) + } + + #[cfg(feature = "runtime-benchmarks")] + fn max_ongoing() -> (Self::Class, u32) { + (0, 0) } } diff --git a/substrate/frame/salary/src/tests/integration.rs b/substrate/frame/salary/src/tests/integration.rs index 69f218943ade..7fd5ac94f4dc 100644 --- a/substrate/frame/salary/src/tests/integration.rs +++ b/substrate/frame/salary/src/tests/integration.rs @@ -23,7 +23,8 @@ use frame_support::{ parameter_types, traits::{ConstU64, EitherOf, MapSuccess, PollStatus, Polling}, }; -use pallet_ranked_collective::{EnsureRanked, Geometric, TallyOf, Votes}; +use frame_system::pallet_prelude::BlockNumberFor; +use pallet_ranked_collective::{EnsureRanked, Geometric, MemberIndex, TallyOf, Votes}; use sp_core::{ConstU16, Get}; use sp_runtime::{ traits::{Convert, ReduceBy, ReplaceWithDefault}, @@ -57,40 +58,45 @@ impl frame_system::Config for Test { pub struct TestPolls; impl Polling> for TestPolls { - type Index = u8; + type Index = MemberIndex; type Votes = Votes; - type Moment = u64; + type Moment = BlockNumberFor; type Class = Rank; fn classes() -> Vec { - unimplemented!() + vec![] } - fn as_ongoing(_index: u8) -> Option<(TallyOf, Self::Class)> { - unimplemented!() + fn as_ongoing(_index: Self::Index) -> Option<(TallyOf, Self::Class)> { + None } fn access_poll( _index: Self::Index, - _f: impl FnOnce(PollStatus<&mut TallyOf, Self::Moment, Self::Class>) -> R, + f: impl FnOnce(PollStatus<&mut TallyOf, Self::Moment, Self::Class>) -> R, ) -> R { - unimplemented!() + f(PollStatus::None) } fn try_access_poll( _index: Self::Index, - _f: impl FnOnce( + f: impl FnOnce( PollStatus<&mut TallyOf, Self::Moment, Self::Class>, ) -> Result, ) -> Result { - unimplemented!() + f(PollStatus::None) } #[cfg(feature = "runtime-benchmarks")] fn create_ongoing(_class: Self::Class) -> Result { - unimplemented!() + Err(()) } #[cfg(feature = "runtime-benchmarks")] fn end_ongoing(_index: Self::Index, _approved: bool) -> Result<(), ()> { - unimplemented!() + Err(()) + } + + #[cfg(feature = "runtime-benchmarks")] + fn max_ongoing() -> (Self::Class, u32) { + (0, 0) } } From 4304153e3cb6f0c5e3382dbcab26838d3e77b63f Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Mon, 9 Sep 2024 12:01:27 +0100 Subject: [PATCH 02/17] Skip None Poll::classes() --- substrate/frame/ranked-collective/src/benchmarking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index dc7f4aaca773..3cf4617cdafe 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -126,7 +126,7 @@ benchmarks_instance_pallet! { } vote { - let class = T::Polls::classes().into_iter().next().unwrap(); + let class = T::Polls::classes().into_iter().next().ok_or(BenchmarkError::Skip)?; let rank = T::MinRankOfClass::convert(class.clone()); let caller = make_member::(rank); @@ -147,7 +147,7 @@ benchmarks_instance_pallet! { let n in 0 .. 100; // Create a poll - let class = T::Polls::classes().into_iter().next().unwrap(); + let class = T::Polls::classes().into_iter().next().ok_or(BenchmarkError::Skip)?; let rank = T::MinRankOfClass::convert(class.clone()); let poll = T::Polls::create_ongoing(class).expect("Must always be able to create a poll"); From fef9f086c0c76f8e172c33605388dc457c017fe6 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Mon, 9 Sep 2024 15:02:20 +0100 Subject: [PATCH 03/17] adds pr docs --- prdoc/pr_5311.prdoc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 prdoc/pr_5311.prdoc diff --git a/prdoc/pr_5311.prdoc b/prdoc/pr_5311.prdoc new file mode 100644 index 000000000000..267efa636838 --- /dev/null +++ b/prdoc/pr_5311.prdoc @@ -0,0 +1,14 @@ +title: No-op Impl Polling Trait + +doc: + - audience: Runtime Dev + description: | + Provide no-op implementations for Polling trait in test contexts, skipping benchmarks that necessitate trait definition. + +crates: + - name: pallet-core-fellowship + bump: minor + - name: pallet-ranked-collective + bump: minor + - name: pallet-salary + bump: minor From 15512b141d4b766f3d6c48cdcd0b1b9276a8bdb8 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Tue, 10 Sep 2024 13:24:54 +0100 Subject: [PATCH 04/17] NoOp unit --- prdoc/pr_5311.prdoc | 4 +- .../core-fellowship/src/tests/integration.rs | 46 +------------------ .../frame/salary/src/tests/integration.rs | 46 +------------------ substrate/frame/support/src/traits/voting.rs | 45 ++++++++++++++++++ 4 files changed, 50 insertions(+), 91 deletions(-) diff --git a/prdoc/pr_5311.prdoc b/prdoc/pr_5311.prdoc index 267efa636838..07affa5cb2ee 100644 --- a/prdoc/pr_5311.prdoc +++ b/prdoc/pr_5311.prdoc @@ -3,7 +3,7 @@ title: No-op Impl Polling Trait doc: - audience: Runtime Dev description: | - Provide no-op implementations for Polling trait in test contexts, skipping benchmarks that necessitate trait definition. + 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 @@ -12,3 +12,5 @@ crates: bump: minor - name: pallet-salary bump: minor + - name: frame-support + bump: minor diff --git a/substrate/frame/core-fellowship/src/tests/integration.rs b/substrate/frame/core-fellowship/src/tests/integration.rs index b50e9310e746..9f2018d94f48 100644 --- a/substrate/frame/core-fellowship/src/tests/integration.rs +++ b/substrate/frame/core-fellowship/src/tests/integration.rs @@ -83,50 +83,6 @@ impl Config for Test { type MaxRank = ConstU32<9>; } -pub struct TestPolls; -impl Polling> for TestPolls { - type Index = MemberIndex; - type Votes = Votes; - type Moment = BlockNumberFor; - type Class = u16; - - fn classes() -> Vec { - vec![] - } - fn as_ongoing(_index: Self::Index) -> Option<(TallyOf, Self::Class)> { - None - } - fn access_poll( - _: Self::Index, - f: impl FnOnce(PollStatus<&mut TallyOf, Self::Moment, Self::Class>) -> R, - ) -> R { - f(PollStatus::None) - } - fn try_access_poll( - _: Self::Index, - f: impl FnOnce( - PollStatus<&mut TallyOf, Self::Moment, Self::Class>, - ) -> Result, - ) -> Result { - f(PollStatus::None) - } - - #[cfg(feature = "runtime-benchmarks")] - fn create_ongoing(_: Self::Class) -> Result { - Err(()) - } - - #[cfg(feature = "runtime-benchmarks")] - fn end_ongoing(_: Self::Index, _: bool) -> Result<(), ()> { - Err(()) - } - - #[cfg(feature = "runtime-benchmarks")] - fn max_ongoing() -> (Self::Class, u32) { - (0, 0) - } -} - /// Convert the tally class into the minimum rank required to vote on the poll. /// MinRank(Class) = Class - Delta pub struct MinRankOfClass(PhantomData); @@ -159,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/salary/src/tests/integration.rs b/substrate/frame/salary/src/tests/integration.rs index 7fd5ac94f4dc..3d05aa6c82f0 100644 --- a/substrate/frame/salary/src/tests/integration.rs +++ b/substrate/frame/salary/src/tests/integration.rs @@ -56,50 +56,6 @@ impl frame_system::Config for Test { type Block = Block; } -pub struct TestPolls; -impl Polling> for TestPolls { - type Index = MemberIndex; - type Votes = Votes; - type Moment = BlockNumberFor; - type Class = Rank; - - fn classes() -> Vec { - vec![] - } - fn as_ongoing(_index: Self::Index) -> Option<(TallyOf, Self::Class)> { - None - } - fn access_poll( - _index: Self::Index, - f: impl FnOnce(PollStatus<&mut TallyOf, Self::Moment, Self::Class>) -> R, - ) -> R { - f(PollStatus::None) - } - fn try_access_poll( - _index: Self::Index, - f: impl FnOnce( - PollStatus<&mut TallyOf, 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) - } -} - pub struct MinRankOfClass(PhantomData); impl> Convert for MinRankOfClass { fn convert(a: u16) -> Rank { @@ -182,7 +138,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..63f89fbb3375 100644 --- a/substrate/frame/support/src/traits/voting.rs +++ b/substrate/frame/support/src/traits/voting.rs @@ -23,6 +23,7 @@ use alloc::vec::Vec; use codec::{HasCompact, MaxEncodedLen}; use sp_arithmetic::Perbill; use sp_runtime::{traits::Member, DispatchError}; +use alloc::vec; pub trait VoteTally { /// Initializes a new tally. @@ -126,3 +127,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) + } +} From 9167e7a4c6c1cf26a7ae1c62323fddad1bc00f12 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Tue, 10 Sep 2024 14:00:19 +0100 Subject: [PATCH 05/17] fmt --- substrate/frame/support/src/traits/voting.rs | 25 ++++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/substrate/frame/support/src/traits/voting.rs b/substrate/frame/support/src/traits/voting.rs index 63f89fbb3375..789d6e642875 100644 --- a/substrate/frame/support/src/traits/voting.rs +++ b/substrate/frame/support/src/traits/voting.rs @@ -19,11 +19,10 @@ //! votes. use crate::dispatch::Parameter; -use alloc::vec::Vec; +use alloc::vec; use codec::{HasCompact, MaxEncodedLen}; use sp_arithmetic::Perbill; use sp_runtime::{traits::Member, DispatchError}; -use alloc::vec; pub trait VoteTally { /// Initializes a new tally. @@ -130,9 +129,9 @@ pub trait Polling { impl Polling for () { type Index = u32; - type Votes = u32; - type Class = u16; - type Moment = u64; + type Votes = u32; + type Class = u16; + type Moment = u64; fn classes() -> Vec { vec![] @@ -143,17 +142,17 @@ impl Polling for () { } fn access_poll( - _index: Self::Index, - f: impl FnOnce(PollStatus<&mut Tally, Self::Moment, Self::Class>) -> R, - ) -> R { - f(PollStatus::None) + _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) + _index: Self::Index, + f: impl FnOnce(PollStatus<&mut Tally, Self::Moment, Self::Class>) -> Result, + ) -> Result { + f(PollStatus::None) } #[cfg(feature = "runtime-benchmarks")] From d2fc8b122d6624bc15f1b0b534e119099dce86bc Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Tue, 10 Sep 2024 14:03:17 +0100 Subject: [PATCH 06/17] remove imports --- substrate/frame/core-fellowship/src/tests/integration.rs | 8 ++++---- substrate/frame/salary/src/tests/integration.rs | 7 +++---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/substrate/frame/core-fellowship/src/tests/integration.rs b/substrate/frame/core-fellowship/src/tests/integration.rs index 9f2018d94f48..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::{pallet_prelude::BlockNumberFor, EnsureSignedBy}; -use pallet_ranked_collective::{EnsureRanked, Geometric, MemberIndex, Rank, TallyOf, Votes}; +use frame_system::EnsureSignedBy; +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; diff --git a/substrate/frame/salary/src/tests/integration.rs b/substrate/frame/salary/src/tests/integration.rs index 3d05aa6c82f0..cde81f8701ce 100644 --- a/substrate/frame/salary/src/tests/integration.rs +++ b/substrate/frame/salary/src/tests/integration.rs @@ -21,14 +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 frame_system::pallet_prelude::BlockNumberFor; -use pallet_ranked_collective::{EnsureRanked, Geometric, MemberIndex, 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; From 23c2983374599d5d285d15ef5f28d53400f24f0d Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Tue, 10 Sep 2024 14:40:53 +0100 Subject: [PATCH 07/17] vec::Vec --- substrate/frame/support/src/traits/voting.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/support/src/traits/voting.rs b/substrate/frame/support/src/traits/voting.rs index 789d6e642875..65941b24e68a 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; +use alloc::{vec, vec::Vec}; use codec::{HasCompact, MaxEncodedLen}; use sp_arithmetic::Perbill; use sp_runtime::{traits::Member, DispatchError}; From 63a4bfaf298969ec3904012b20fdbbcf41714ec4 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Fri, 13 Sep 2024 18:55:29 +0100 Subject: [PATCH 08/17] Default Poll Classes --- substrate/frame/ranked-collective/src/benchmarking.rs | 2 +- substrate/frame/referenda/src/types.rs | 5 +++-- substrate/frame/support/src/traits/voting.rs | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index 3cf4617cdafe..d0ce1cfde5ca 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -126,7 +126,7 @@ benchmarks_instance_pallet! { } vote { - let class = T::Polls::classes().into_iter().next().ok_or(BenchmarkError::Skip)?; + let class = T::Polls::classes().into_iter().next().unwrap_or_else(|| Default::default()); let rank = T::MinRankOfClass::convert(class.clone()); let caller = make_member::(rank); diff --git a/substrate/frame/referenda/src/types.rs b/substrate/frame/referenda/src/types.rs index 1039b288b2ae..49ee8dce2130 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; @@ -258,7 +258,8 @@ impl< Tally: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, AccountId: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, ScheduleAddress: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, - > ReferendumInfo + > + ReferendumInfo { /// Take the Decision Deposit from `self`, if there is one. Returns an `Err` if `self` is not /// in a valid state for the Decision Deposit to be refunded. diff --git a/substrate/frame/support/src/traits/voting.rs b/substrate/frame/support/src/traits/voting.rs index 65941b24e68a..7f23a35bf905 100644 --- a/substrate/frame/support/src/traits/voting.rs +++ b/substrate/frame/support/src/traits/voting.rs @@ -84,7 +84,7 @@ impl> sp_runtime::traits::Get for ClassCountOf { pub trait Polling { type Index: Parameter + Member + Ord + PartialOrd + Copy + HasCompact + MaxEncodedLen; 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. From c9460f61470d71e76532fbf81da86bfb6f36ec30 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Fri, 13 Sep 2024 18:59:54 +0100 Subject: [PATCH 09/17] update pr doc --- prdoc/pr_5311.prdoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/prdoc/pr_5311.prdoc b/prdoc/pr_5311.prdoc index 07affa5cb2ee..b800a20774fd 100644 --- a/prdoc/pr_5311.prdoc +++ b/prdoc/pr_5311.prdoc @@ -14,3 +14,5 @@ crates: bump: minor - name: frame-support bump: minor + - name: pallet-referenda + bump: minor From 6f1bbeb8a50068707d37895c7ea0d218d5f2f6b6 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Fri, 13 Sep 2024 19:29:48 +0100 Subject: [PATCH 10/17] nit --- substrate/frame/ranked-collective/src/benchmarking.rs | 2 +- substrate/frame/referenda/src/types.rs | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index d0ce1cfde5ca..74dc904fa6ff 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -147,7 +147,7 @@ benchmarks_instance_pallet! { let n in 0 .. 100; // Create a poll - let class = T::Polls::classes().into_iter().next().ok_or(BenchmarkError::Skip)?; + let class = T::Polls::classes().into_iter().next().unwrap_or_else(|| Default::default()); let rank = T::MinRankOfClass::convert(class.clone()); let poll = T::Polls::create_ongoing(class).expect("Must always be able to create a poll"); diff --git a/substrate/frame/referenda/src/types.rs b/substrate/frame/referenda/src/types.rs index 49ee8dce2130..878e0f30ce93 100644 --- a/substrate/frame/referenda/src/types.rs +++ b/substrate/frame/referenda/src/types.rs @@ -258,8 +258,7 @@ impl< Tally: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, AccountId: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, ScheduleAddress: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, - > - ReferendumInfo + > ReferendumInfo { /// Take the Decision Deposit from `self`, if there is one. Returns an `Err` if `self` is not /// in a valid state for the Decision Deposit to be refunded. From ec8598ebd544e4a6f637f57a22d164adffc464d9 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Thu, 19 Sep 2024 14:36:25 +0100 Subject: [PATCH 11/17] NoOp benchmark inclusion --- .../ranked-collective/src/benchmarking.rs | 287 ++++++++++++++---- substrate/frame/support/src/traits/voting.rs | 2 +- 2 files changed, 228 insertions(+), 61 deletions(-) diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index 74dc904fa6ff..3afe61c8e50f 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_or_else(|| Default::default()); - 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_or_else(|| Default::default()); + // 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/support/src/traits/voting.rs b/substrate/frame/support/src/traits/voting.rs index 7f23a35bf905..8daccbff6900 100644 --- a/substrate/frame/support/src/traits/voting.rs +++ b/substrate/frame/support/src/traits/voting.rs @@ -82,7 +82,7 @@ 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 + Default; type Moment; From 387c00e9bbf73c87ba06ac0ee2c9f5dd8d949060 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Thu, 19 Sep 2024 15:16:28 +0100 Subject: [PATCH 12/17] not a function --- substrate/frame/ranked-collective/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index 3afe61c8e50f..5411f1e00c8b 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -240,7 +240,7 @@ mod benchmarks { } // Vote again with a different decision (false). - assert_ok(Pallet::::vote(SystemOrigin::Signed(caller.clone()).into(), poll, 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 { From b5d2b4fbd4916fc51fcdac96ada1235863dd13ae Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Mon, 23 Sep 2024 15:15:02 +0100 Subject: [PATCH 13/17] fmt --- substrate/frame/ranked-collective/src/benchmarking.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index 5411f1e00c8b..a5b3a20b67cd 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -234,11 +234,20 @@ mod benchmarks { 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). + let vote_false = + Pallet::::vote(SystemOrigin::Signed(caller.clone()).into(), poll, false); + + if class_exists { + assert_ok!(vote_false); + } else { + assert_err!(vote_false, crate::Error::::NotPolling); + } + // Vote again with a different decision (false). assert_ok!(Pallet::::vote(SystemOrigin::Signed(caller.clone()).into(), poll, false)); From e788014b4cc737b6db910f81021a3b53b9f51383 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Mon, 23 Sep 2024 15:30:25 +0100 Subject: [PATCH 14/17] rm log crate --- substrate/frame/ranked-collective/src/benchmarking.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index a5b3a20b67cd..b406935a2d85 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -21,7 +21,6 @@ use super::*; #[allow(unused_imports)] use crate::Pallet as RankedCollective; use alloc::vec::Vec; -use log::info; use frame_benchmarking::{ v1::{account, BenchmarkError}, From 8f10911d29f42769a7965f4fb9646e76d8a1a6a8 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Tue, 24 Sep 2024 20:45:59 +0100 Subject: [PATCH 15/17] rm .clone(), add #[extrinsic] --- .../ranked-collective/src/benchmarking.rs | 85 +++++-------------- 1 file changed, 22 insertions(+), 63 deletions(-) diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index b406935a2d85..04ecf426dc92 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -72,20 +72,14 @@ mod benchmarks { 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 }; - - // Dispatch the call and measure execution. - #[block] - { - call.dispatch_bypass_filter(origin)?; - } + #[extrinsic_call] + _(origin as T::RuntimeOrigin, who_lookup); // Ensure the member count has increased (or is 1 for rank 0). assert_eq!(MemberCount::::get(0), 1); // Check that the correct event was emitted. - assert_last_event::(Event::MemberAdded { who: who.clone() }.into()); + assert_last_event::(Event::MemberAdded { who }.into()); Ok(()) } @@ -94,7 +88,6 @@ mod benchmarks { 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 who = make_member::(rank); let who_lookup = T::Lookup::unlookup(who.clone()); let last = make_member::(rank); @@ -107,22 +100,16 @@ mod benchmarks { 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 }; - - // Dispatch the call and measure its execution. - #[block] - { - call.dispatch_bypass_filter(origin)?; - } + #[extrinsic_call] + _(origin as T::RuntimeOrigin, who_lookup, rank); for r in 0..=rank { - assert_eq!(MemberCount::::get(r), 2); + assert_eq!(MemberCount::::get(r), 1); assert_ne!(last_index[r as usize], IdToIndex::::get(r, &last).unwrap()); } // Ensure the correct event was emitted for the member removal. - assert_last_event::(Event::MemberRemoved { who: who.clone(), rank }.into()); + assert_last_event::(Event::MemberRemoved { who, rank }.into()); Ok(()) } @@ -138,20 +125,14 @@ mod benchmarks { 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 }; - - // Dispatch the call and measure its execution. - #[block] - { - call.dispatch_bypass_filter(origin)?; - } + #[extrinsic_call] + _(origin as T::RuntimeOrigin, who_lookup); // Ensure the member's rank has increased by 1. assert_eq!(Members::::get(&who).unwrap().rank, rank + 1); // Ensure the correct event was emitted for the rank change. - assert_last_event::(Event::RankChanged { who: who.clone(), rank: rank + 1 }.into()); + assert_last_event::(Event::RankChanged { who, rank: rank + 1 }.into()); Ok(()) } @@ -160,7 +141,6 @@ mod benchmarks { 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 who = make_member::(rank); let who_lookup = T::Lookup::unlookup(who.clone()); let last = make_member::(rank); @@ -172,20 +152,14 @@ mod benchmarks { 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 }; - - // Dispatch the call and measure its execution. - #[block] - { - call.dispatch_bypass_filter(origin)?; - } + #[extrinsic_call] + _(origin as T::RuntimeOrigin, who_lookup); // 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); + assert_eq!(MemberCount::::get(rank), 1); // Ensure the index of the last member has changed. assert_ne!(last_index, IdToIndex::::get(rank, &last).unwrap()); @@ -193,8 +167,8 @@ mod benchmarks { // 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 }, + 0 => Event::MemberRemoved { who, rank: 0 }, + r => Event::RankChanged { who, rank: r - 1 }, } .into(), ); @@ -247,9 +221,6 @@ mod benchmarks { assert_err!(vote_false, 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); @@ -268,6 +239,8 @@ mod benchmarks { .next() .map(|c| (true, c)) .unwrap_or_else(|| (false, Default::default())); + let alice: T::AccountId = whitelisted_caller(); + let origin = SystemOrigin::Signed(alice.clone()); // Get rank and create a poll if the class exists. let rank = T::MinRankOfClass::convert(class.clone()); @@ -301,14 +274,8 @@ mod benchmarks { assert_eq!(Voting::::iter_prefix(poll).count(), expected_votes); // Benchmark the cleanup function. - #[block] - { - let _ = Pallet::::cleanup_poll( - SystemOrigin::Signed(whitelisted_caller()).into(), - poll, - n, - ); - } + #[extrinsic_call] + _(origin, poll, n); // Ensure all votes are cleaned up. assert_eq!(Voting::::iter().count(), 0); @@ -330,14 +297,8 @@ mod benchmarks { 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 }; - - // Dispatch the call and measure execution. - #[block] - { - call.dispatch_bypass_filter(origin)?; - } + #[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::::get(&new_who).unwrap().rank, 1); @@ -346,9 +307,7 @@ mod benchmarks { assert_eq!(Members::::get(&who), None); // Ensure the correct event was emitted. - assert_has_event::( - Event::MemberExchanged { who: who.clone(), new_who: new_who.clone() }.into(), - ); + assert_has_event::(Event::MemberExchanged { who, new_who }.into()); Ok(()) } From a1d7801c2c2d0938e3da6cad2cbc9caf10a15b7e Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Tue, 24 Sep 2024 21:18:44 +0100 Subject: [PATCH 16/17] fmt --- substrate/frame/ranked-collective/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index 04ecf426dc92..ca0c0df9c3d0 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -27,7 +27,7 @@ use frame_benchmarking::{ v2::*, }; -use frame_support::{assert_err, assert_ok, traits::UnfilteredDispatchable}; +use frame_support::{assert_err, assert_ok}; use frame_system::RawOrigin as SystemOrigin; const SEED: u32 = 0; From 4a1e087842a70b6a3082a98c84ada5932a6ca555 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Sat, 12 Oct 2024 21:13:13 +0100 Subject: [PATCH 17/17] u8 Index --- .../core-fellowship/src/tests/integration.rs | 4 +- .../ranked-collective/src/benchmarking.rs | 101 ++++++++++-------- .../frame/salary/src/tests/integration.rs | 9 +- substrate/frame/support/src/traits.rs | 2 +- substrate/frame/support/src/traits/voting.rs | 9 +- 5 files changed, 69 insertions(+), 56 deletions(-) diff --git a/substrate/frame/core-fellowship/src/tests/integration.rs b/substrate/frame/core-fellowship/src/tests/integration.rs index a41534cf81ba..7a48ed9783e7 100644 --- a/substrate/frame/core-fellowship/src/tests/integration.rs +++ b/substrate/frame/core-fellowship/src/tests/integration.rs @@ -21,7 +21,7 @@ use frame_support::{ assert_noop, assert_ok, derive_impl, hypothetically, ord_parameter_types, pallet_prelude::Weight, parameter_types, - traits::{ConstU16, EitherOf, IsInVec, MapSuccess, TryMapSuccess}, + traits::{ConstU16, EitherOf, IsInVec, MapSuccess, NoOpPoll, TryMapSuccess}, }; use frame_system::EnsureSignedBy; use pallet_ranked_collective::{EnsureRanked, Geometric, Rank}; @@ -115,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 = (); + type Polls = NoOpPoll; 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 ca0c0df9c3d0..120d7ceb7044 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -21,13 +21,12 @@ use super::*; #[allow(unused_imports)] use crate::Pallet as RankedCollective; use alloc::vec::Vec; - use frame_benchmarking::{ v1::{account, BenchmarkError}, v2::*, }; -use frame_support::{assert_err, assert_ok}; +use frame_support::{assert_err, assert_ok, traits::NoOpPoll}; use frame_system::RawOrigin as SystemOrigin; const SEED: u32 = 0; @@ -178,51 +177,60 @@ mod benchmarks { #[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()); + // 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( + as frame_support::traits::RankedMembers>::Rank::default(), + |class| T::MinRankOfClass::convert(class.clone()), + ); + + // Create a caller based on the rank. let caller = make_member::(rank); - // If the class exists, create an ongoing poll, otherwise use a default poll. - let poll = if class_exists { + // 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("Should always be able to create a poll for rank 0") + .expect("Poll creation should succeed for rank 0") } else { - Default::default() + >::Index::MAX.into() }; - // Benchmark the vote logic. + // Benchmark the vote logic for a positive vote (true). #[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 { - assert_err!(vote_result, crate::Error::::NotPolling); + // If the class exists, expect success; otherwise expect a "NotPolling" error. + match class { + Some(_) => { + assert_ok!(vote_result); + }, + None => { + assert_err!(vote_result, crate::Error::::NotPolling); + }, } } - // Vote again with a different decision (false). - let vote_false = + // Vote logic for a negative vote (false). + let vote_result = Pallet::::vote(SystemOrigin::Signed(caller.clone()).into(), poll, false); - if class_exists { - assert_ok!(vote_false); - } else { - assert_err!(vote_false, crate::Error::::NotPolling); + // Check the result of the negative vote. + match class { + Some(_) => { + assert_ok!(vote_result); + }, + None => { + assert_err!(vote_result, crate::Error::::NotPolling); + }, } // If the class exists, verify the vote event and tally. - if class_exists { + 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::(vote_event.into()); @@ -233,31 +241,34 @@ mod benchmarks { #[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())); let alice: T::AccountId = whitelisted_caller(); let origin = SystemOrigin::Signed(alice.clone()); - // Get rank and create a poll if the class exists. - let rank = T::MinRankOfClass::convert(class.clone()); - let poll = if class_exists { + // 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( + as frame_support::traits::RankedMembers>::Rank::default(), + |class| T::MinRankOfClass::convert(class.clone()), + ); + + // 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 { - Default::default() + >::Index::MAX.into() }; - // Simulate voting in the poll by `n` members. + // Simulate voting 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 { + if class.is_some() { assert_ok!(result); } else { assert_err!(result, crate::Error::::NotPolling); @@ -265,20 +276,22 @@ mod benchmarks { } // End the poll if the class exists. - if class_exists { - T::Polls::end_ongoing(poll, false).expect("Poll should be able to end"); + if class.is_some() { + T::Polls::end_ongoing(poll, false) + .map_err(|_| BenchmarkError::Stop("Failed to end poll"))?; } // Verify the number of votes cast. - let expected_votes = if class_exists { n as usize } else { 0 }; + let expected_votes = if class.is_some() { n as usize } else { 0 }; assert_eq!(Voting::::iter_prefix(poll).count(), expected_votes); // Benchmark the cleanup function. #[extrinsic_call] _(origin, poll, n); - // Ensure all votes are cleaned up. + // Ensure all votes are cleaned up after the extrinsic call. assert_eq!(Voting::::iter().count(), 0); + Ok(()) } diff --git a/substrate/frame/salary/src/tests/integration.rs b/substrate/frame/salary/src/tests/integration.rs index cde81f8701ce..0c1fb8bbdcba 100644 --- a/substrate/frame/salary/src/tests/integration.rs +++ b/substrate/frame/salary/src/tests/integration.rs @@ -17,11 +17,13 @@ //! 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}, + traits::{ConstU64, EitherOf, MapSuccess, NoOpPoll}, }; use pallet_ranked_collective::{EnsureRanked, Geometric}; use sp_core::{ConstU16, Get}; @@ -30,9 +32,6 @@ use sp_runtime::{ BuildStorage, }; -use crate as pallet_salary; -use crate::*; - type Rank = u16; type Block = frame_system::mocking::MockBlock; @@ -137,7 +136,7 @@ impl pallet_ranked_collective::Config for Test { // Members can exchange up to the rank of 2 below them. MapSuccess, ReduceBy>>, >; - type Polls = (); + type Polls = NoOpPoll; type MinRankOfClass = MinRankOfClass; type MemberSwappedHandler = Salary; type VoteWeight = Geometric; diff --git a/substrate/frame/support/src/traits.rs b/substrate/frame/support/src/traits.rs index 6e59ff9d030b..bb0c821202b9 100644 --- a/substrate/frame/support/src/traits.rs +++ b/substrate/frame/support/src/traits.rs @@ -109,7 +109,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}; diff --git a/substrate/frame/support/src/traits/voting.rs b/substrate/frame/support/src/traits/voting.rs index 8daccbff6900..afa473270392 100644 --- a/substrate/frame/support/src/traits/voting.rs +++ b/substrate/frame/support/src/traits/voting.rs @@ -82,9 +82,9 @@ impl> sp_runtime::traits::Get for ClassCountOf { } pub trait Polling { - type Index: Parameter + Member + Ord + PartialOrd + Copy + HasCompact + MaxEncodedLen + Default; + type Index: Parameter + Member + Ord + PartialOrd + Copy + HasCompact + MaxEncodedLen + From; type Votes: Parameter + Member + Ord + PartialOrd + Copy + HasCompact + MaxEncodedLen; - type Class: Parameter + Member + Ord + PartialOrd + MaxEncodedLen + Default; + type Class: Parameter + Member + Ord + PartialOrd + MaxEncodedLen; type Moment; /// Provides a vec of values that `T` may take. @@ -127,8 +127,9 @@ pub trait Polling { } } -impl Polling for () { - type Index = u32; +pub struct NoOpPoll; +impl Polling for NoOpPoll { + type Index = u8; type Votes = u32; type Class = u16; type Moment = u64;