From 5d949bbe92b7aab748006c6a59eae14df0414b80 Mon Sep 17 00:00:00 2001 From: shamb0 Date: Wed, 24 Feb 2021 17:09:10 +0530 Subject: [PATCH 01/13] wk2108 | port to frame v2 macro definition --- frame/collective/src/benchmarking.rs | 6 +- frame/collective/src/lib.rs | 637 +++++++++++++-------------- 2 files changed, 320 insertions(+), 323 deletions(-) diff --git a/frame/collective/src/benchmarking.rs b/frame/collective/src/benchmarking.rs index 1afdd14b1ad38..6406c503fe154 100644 --- a/frame/collective/src/benchmarking.rs +++ b/frame/collective/src/benchmarking.rs @@ -22,7 +22,7 @@ use super::*; use frame_system::RawOrigin as SystemOrigin; use frame_system::EventRecord; use frame_benchmarking::{ - benchmarks_instance, + benchmarks_instance_pallet, account, whitelisted_caller, impl_benchmark_test_suite, @@ -38,7 +38,7 @@ const SEED: u32 = 0; const MAX_BYTES: u32 = 1_024; -fn assert_last_event, I: Instance>(generic_event: >::Event) { +fn assert_last_event, I: 'static>(generic_event: >::Event) { let events = System::::events(); let system_event: ::Event = generic_event.into(); // compare to the last event record @@ -46,7 +46,7 @@ fn assert_last_event, I: Instance>(generic_event: >: assert_eq!(event, &system_event); } -benchmarks_instance! { +benchmarks_instance_pallet! { set_members { let m in 1 .. T::MaxMembers::get(); let n in 1 .. T::MaxMembers::get(); diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 50beb8607d61d..435912d573e45 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -48,17 +48,19 @@ use sp_io::storage; use sp_runtime::{RuntimeDebug, traits::Hash}; use frame_support::{ - codec::{Decode, Encode}, - debug, decl_error, decl_event, decl_module, decl_storage, + codec::{Decode, Encode}, debug, dispatch::{ - DispatchError, DispatchResult, DispatchResultWithPostInfo, Dispatchable, Parameter, + DispatchError, DispatchResult, DispatchResultWithPostInfo, Dispatchable, PostDispatchInfo, }, ensure, traits::{ChangeMembers, EnsureOrigin, Get, InitializeMembers}, - weights::{DispatchClass, GetDispatchInfo, Weight, Pays}, + weights::{GetDispatchInfo, Weight}, }; -use frame_system::{self as system, ensure_signed, ensure_root}; +#[cfg(feature = "std")] +use frame_support::traits::GenesisBuild; + +use frame_system::{self as system}; #[cfg(feature = "runtime-benchmarks")] mod benchmarking; @@ -66,6 +68,8 @@ mod benchmarking; pub mod weights; pub use weights::WeightInfo; +pub use pallet::*; + /// Simple index type for proposal counting. pub type ProposalIndex = u32; @@ -121,165 +125,64 @@ impl DefaultVote for MoreThanMajorityThenPrimeDefaultVote { } } -pub trait Config: frame_system::Config { - /// The outer origin type. - type Origin: From>; +#[frame_support::pallet] +pub mod pallet { + use frame_support::pallet_prelude::*; + use frame_system::pallet_prelude::*; + use super::*; - /// The outer call dispatch type. - type Proposal: Parameter - + Dispatchable>::Origin, PostInfo=PostDispatchInfo> - + From> - + GetDispatchInfo; + #[pallet::config] + pub trait Config: frame_system::Config { - /// The outer event type. - type Event: From> + Into<::Event>; + /// The outer origin type. + type Origin: From>; - /// The time-out for council motions. - type MotionDuration: Get; + /// The outer call dispatch type. + type Proposal: Parameter + + Dispatchable>::Origin, PostInfo=PostDispatchInfo> + + From> + + GetDispatchInfo; - /// Maximum number of proposals allowed to be active in parallel. - type MaxProposals: Get; + /// The overarching event type. + type Event: From> + IsType<::Event>; - /// The maximum number of members supported by the pallet. Used for weight estimation. - /// - /// NOTE: - /// + Benchmarks will need to be re-run and weights adjusted if this changes. - /// + This pallet assumes that dependents keep to the limit without enforcing it. - type MaxMembers: Get; + /// The time-out for council motions. + #[pallet::constant] + type MotionDuration: Get; - /// Default vote strategy of this collective. - type DefaultVote: DefaultVote; + /// Maximum number of proposals allowed to be active in parallel. + #[pallet::constant] + type MaxProposals: Get; - /// Weight information for extrinsics in this pallet. - type WeightInfo: WeightInfo; -} - -/// Origin for the collective module. -#[derive(PartialEq, Eq, Clone, RuntimeDebug, Encode, Decode)] -pub enum RawOrigin { - /// It has been condoned by a given number of members of the collective from a given total. - Members(MemberCount, MemberCount), - /// It has been condoned by a single member of the collective. - Member(AccountId), - /// Dummy to manage the fact we have instancing. - _Phantom(sp_std::marker::PhantomData), -} - -/// Origin for the collective module. -pub type Origin = RawOrigin<::AccountId, I>; + /// The maximum number of members supported by the pallet. Used for weight estimation. + /// + /// NOTE: + /// + Benchmarks will need to be re-run and weights adjusted if this changes. + /// + This pallet assumes that dependents keep to the limit without enforcing it. + #[pallet::constant] + type MaxMembers: Get; -#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug)] -/// Info for keeping track of a motion being voted on. -pub struct Votes { - /// The proposal's unique index. - index: ProposalIndex, - /// The number of approval votes that are needed to pass the motion. - threshold: MemberCount, - /// The current set of voters that approved it. - ayes: Vec, - /// The current set of voters that rejected it. - nays: Vec, - /// The hard end time of this vote. - end: BlockNumber, -} + /// Default vote strategy of this collective. + type DefaultVote: DefaultVote; -decl_storage! { - trait Store for Module, I: Instance=DefaultInstance> as Collective { - /// The hashes of the active proposals. - pub Proposals get(fn proposals): Vec; - /// Actual proposal for a given hash, if it's current. - pub ProposalOf get(fn proposal_of): - map hasher(identity) T::Hash => Option<>::Proposal>; - /// Votes on a given proposal, if it is ongoing. - pub Voting get(fn voting): - map hasher(identity) T::Hash => Option>; - /// Proposals so far. - pub ProposalCount get(fn proposal_count): u32; - /// The current members of the collective. This is stored sorted (just by value). - pub Members get(fn members): Vec; - /// The prime member that helps determine the default vote behavior in case of absentations. - pub Prime get(fn prime): Option; - } - add_extra_genesis { - config(phantom): sp_std::marker::PhantomData; - config(members): Vec; - build(|config| Module::::initialize_members(&config.members)) + /// Weight information for extrinsics in this pallet. + type WeightInfo: WeightInfo; } -} -decl_event! { - pub enum Event where - ::Hash, - ::AccountId, - { - /// A motion (given hash) has been proposed (by given account) with a threshold (given - /// `MemberCount`). - /// \[account, proposal_index, proposal_hash, threshold\] - Proposed(AccountId, ProposalIndex, Hash, MemberCount), - /// A motion (given hash) has been voted on by given account, leaving - /// a tally (yes votes and no votes given respectively as `MemberCount`). - /// \[account, proposal_hash, voted, yes, no\] - Voted(AccountId, Hash, bool, MemberCount, MemberCount), - /// A motion was approved by the required threshold. - /// \[proposal_hash\] - Approved(Hash), - /// A motion was not approved by the required threshold. - /// \[proposal_hash\] - Disapproved(Hash), - /// A motion was executed; result will be `Ok` if it returned without error. - /// \[proposal_hash, result\] - Executed(Hash, DispatchResult), - /// A single member did some action; result will be `Ok` if it returned without error. - /// \[proposal_hash, result\] - MemberExecuted(Hash, DispatchResult), - /// A proposal was closed because its threshold was reached or after its duration was up. - /// \[proposal_hash, yes, no\] - Closed(Hash, MemberCount, MemberCount), - } -} + #[pallet::pallet] + #[pallet::generate_store(pub(super) trait Store)] + pub struct Pallet(PhantomData<(T, I)>); -decl_error! { - pub enum Error for Module, I: Instance> { - /// Account is not a member - NotMember, - /// Duplicate proposals not allowed - DuplicateProposal, - /// Proposal must exist - ProposalMissing, - /// Mismatched index - WrongIndex, - /// Duplicate vote ignored - DuplicateVote, - /// Members are already initialized! - AlreadyInitialized, - /// The close call was made too early, before the end of the voting. - TooEarly, - /// There can only be a maximum of `MaxProposals` active proposals. - TooManyProposals, - /// The given weight bound for the proposal was too low. - WrongProposalWeight, - /// The given length bound for the proposal was too low. - WrongProposalLength, - } -} + /// Origin for the collective module. + #[pallet::origin] + pub type Origin = RawOrigin<::AccountId, I>; -/// Return the weight of a dispatch call result as an `Option`. -/// -/// Will return the weight regardless of what the state of the result is. -fn get_result_weight(result: DispatchResultWithPostInfo) -> Option { - match result { - Ok(post_info) => post_info.actual_weight, - Err(err) => err.post_info.actual_weight, + #[pallet::hooks] + impl, I: 'static> Hooks> for Pallet { } -} - -// Note that councillor operations are assigned to the operational class. -decl_module! { - pub struct Module, I: Instance=DefaultInstance> for enum Call where origin: ::Origin { - type Error = Error; - - fn deposit_event() = default; + #[pallet::call] + impl, I: 'static> Pallet { /// Set the collective's membership. /// @@ -292,28 +195,14 @@ decl_module! { /// /// NOTE: Does not enforce the expected `MaxMembers` limit on the amount of members, but /// the weight estimations rely on it to estimate dispatchable weight. - /// - /// # - /// ## Weight - /// - `O(MP + N)` where: - /// - `M` old-members-count (code- and governance-bounded) - /// - `N` new-members-count (code- and governance-bounded) - /// - `P` proposals-count (code-bounded) - /// - DB: - /// - 1 storage mutation (codec `O(M)` read, `O(N)` write) for reading and writing the members - /// - 1 storage read (codec `O(P)`) for reading the proposals - /// - `P` storage mutations (codec `O(M)`) for updating the votes for each proposal - /// - 1 storage write (codec `O(1)`) for deleting the old `prime` and setting the new one - /// # - #[weight = ( + #[pallet::weight( T::WeightInfo::set_members( *old_count, // M new_members.len() as u32, // N T::MaxProposals::get() // P - ), - DispatchClass::Operational - )] - fn set_members(origin, + ))] + pub fn set_members( + origin: OriginFor, new_members: Vec, prime: Option, old_count: MemberCount, @@ -321,7 +210,7 @@ decl_module! { ensure_root(origin)?; if new_members.len() > T::MaxMembers::get() as usize { debug::error!( - "New members count exceeds maximum amount of members expected. (expected: {}, actual: {})", + "New members count exceeds MAX members expected.(Expct: {}, Act: {})", T::MaxMembers::get(), new_members.len() ); @@ -330,7 +219,7 @@ decl_module! { let old = Members::::get(); if old.len() > old_count as usize { debug::warn!( - "Wrong count used to estimate set_members weight. (expected: {}, actual: {})", + "Wrong count used to estimate set_members weight.(Expct: {}, Act: {})", old_count, old.len() ); @@ -350,34 +239,27 @@ decl_module! { /// Dispatch a proposal from a member using the `Member` origin. /// /// Origin must be a member of the collective. - /// - /// # - /// ## Weight - /// - `O(M + P)` where `M` members-count (code-bounded) and `P` complexity of dispatching `proposal` - /// - DB: 1 read (codec `O(M)`) + DB access of `proposal` - /// - 1 event - /// # - #[weight = ( + #[pallet::weight( T::WeightInfo::execute( *length_bound, // B T::MaxMembers::get(), // M - ).saturating_add(proposal.get_dispatch_info().weight), // P - DispatchClass::Operational + ) )] - fn execute(origin, + pub fn execute( + origin: OriginFor, proposal: Box<>::Proposal>, - #[compact] length_bound: u32, + #[pallet::compact] length_bound: u32, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; let members = Self::members(); - ensure!(members.contains(&who), Error::::NotMember); + ensure!(members.contains(&who), >::NotMember); let proposal_len = proposal.using_encoded(|x| x.len()); - ensure!(proposal_len <= length_bound as usize, Error::::WrongProposalLength); + ensure!(proposal_len <= length_bound as usize, >::WrongProposalLength); let proposal_hash = T::Hashing::hash_of(&proposal); let result = proposal.dispatch(RawOrigin::Member(who).into()); Self::deposit_event( - RawEvent::MemberExecuted(proposal_hash, result.map(|_| ()).map_err(|e| e.error)) + Event::MemberExecuted(proposal_hash, result.map(|_| ()).map_err(|e| e.error)) ); Ok(get_result_weight(result).map(|w| { @@ -395,27 +277,7 @@ decl_module! { /// `threshold` determines whether `proposal` is executed directly (`threshold < 2`) /// or put up for voting. /// - /// # - /// ## Weight - /// - `O(B + M + P1)` or `O(B + M + P2)` where: - /// - `B` is `proposal` size in bytes (length-fee-bounded) - /// - `M` is members-count (code- and governance-bounded) - /// - branching is influenced by `threshold` where: - /// - `P1` is proposal execution complexity (`threshold < 2`) - /// - `P2` is proposals-count (code-bounded) (`threshold >= 2`) - /// - DB: - /// - 1 storage read `is_member` (codec `O(M)`) - /// - 1 storage read `ProposalOf::contains_key` (codec `O(1)`) - /// - DB accesses influenced by `threshold`: - /// - EITHER storage accesses done by `proposal` (`threshold < 2`) - /// - OR proposal insertion (`threshold <= 2`) - /// - 1 storage mutation `Proposals` (codec `O(P2)`) - /// - 1 storage mutation `ProposalCount` (codec `O(1)`) - /// - 1 storage write `ProposalOf` (codec `O(B)`) - /// - 1 storage write `Voting` (codec `O(M)`) - /// - 1 event - /// # - #[weight = ( + #[pallet::weight( if *threshold < 2 { T::WeightInfo::propose_execute( *length_bound, // B @@ -427,28 +289,29 @@ decl_module! { T::MaxMembers::get(), // M T::MaxProposals::get(), // P2 ) - }, - DispatchClass::Operational + } )] - fn propose(origin, - #[compact] threshold: MemberCount, - proposal: Box<>::Proposal>, - #[compact] length_bound: u32 + pub fn propose( + origin: OriginFor, + #[pallet::compact] threshold: MemberCount, + proposal: Box, + #[pallet::compact] length_bound: u32 ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; let members = Self::members(); - ensure!(members.contains(&who), Error::::NotMember); + ensure!(members.contains(&who), >::NotMember); let proposal_len = proposal.using_encoded(|x| x.len()); - ensure!(proposal_len <= length_bound as usize, Error::::WrongProposalLength); + ensure!(proposal_len <= length_bound as usize, >::WrongProposalLength); let proposal_hash = T::Hashing::hash_of(&proposal); - ensure!(!>::contains_key(proposal_hash), Error::::DuplicateProposal); + + ensure!(!>::contains_key(proposal_hash),>::DuplicateProposal); if threshold < 2 { let seats = Self::members().len() as MemberCount; let result = proposal.dispatch(RawOrigin::Members(1, seats).into()); Self::deposit_event( - RawEvent::Executed(proposal_hash, result.map(|_| ()).map_err(|e| e.error)) + Event::Executed(proposal_hash, result.map(|_| ()).map_err(|e| e.error)) ); Ok(get_result_weight(result).map(|w| { @@ -461,20 +324,18 @@ decl_module! { let active_proposals = >::try_mutate(|proposals| -> Result { proposals.push(proposal_hash); - ensure!( - proposals.len() <= T::MaxProposals::get() as usize, - Error::::TooManyProposals - ); + ensure!(proposals.len() <= T::MaxProposals::get() as usize,>::TooManyProposals); Ok(proposals.len()) })?; let index = Self::proposal_count(); - >::mutate(|i| *i += 1); + + >::mutate(|i| *i += 1); >::insert(proposal_hash, *proposal); let end = system::Module::::block_number() + T::MotionDuration::get(); let votes = Votes { index, threshold, ayes: vec![who.clone()], nays: vec![], end }; >::insert(proposal_hash, votes); - Self::deposit_event(RawEvent::Proposed(who, index, proposal_hash, threshold)); + Self::deposit_event(Event::Proposed(who, index, proposal_hash, threshold)); Ok(Some(T::WeightInfo::propose_proposed( proposal_len as u32, // B @@ -490,29 +351,19 @@ decl_module! { /// /// Transaction fees will be waived if the member is voting on any particular proposal /// for the first time and the call is successful. Subsequent vote changes will charge a fee. - /// # - /// ## Weight - /// - `O(M)` where `M` is members-count (code- and governance-bounded) - /// - DB: - /// - 1 storage read `Members` (codec `O(M)`) - /// - 1 storage mutation `Voting` (codec `O(M)`) - /// - 1 event - /// # - #[weight = ( - T::WeightInfo::vote(T::MaxMembers::get()), - DispatchClass::Operational - )] - fn vote(origin, + #[pallet::weight(T::WeightInfo::vote(T::MaxMembers::get()))] + pub fn vote( + origin: OriginFor, proposal: T::Hash, - #[compact] index: ProposalIndex, + #[pallet::compact] index: ProposalIndex, approve: bool, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; let members = Self::members(); - ensure!(members.contains(&who), Error::::NotMember); + ensure!(members.contains(&who), >::NotMember); - let mut voting = Self::voting(&proposal).ok_or(Error::::ProposalMissing)?; - ensure!(voting.index == index, Error::::WrongIndex); + let mut voting = Self::voting(&proposal).ok_or(>::ProposalMissing)?; + ensure!(voting.index == index, >::WrongIndex); let position_yes = voting.ayes.iter().position(|a| a == &who); let position_no = voting.nays.iter().position(|a| a == &who); @@ -524,7 +375,7 @@ decl_module! { if position_yes.is_none() { voting.ayes.push(who.clone()); } else { - Err(Error::::DuplicateVote)? + Err(>::DuplicateVote)? } if let Some(pos) = position_no { voting.nays.swap_remove(pos); @@ -533,7 +384,7 @@ decl_module! { if position_no.is_none() { voting.nays.push(who.clone()); } else { - Err(Error::::DuplicateVote)? + Err(>::DuplicateVote)? } if let Some(pos) = position_yes { voting.ayes.swap_remove(pos); @@ -542,9 +393,9 @@ decl_module! { let yes_votes = voting.ayes.len() as MemberCount; let no_votes = voting.nays.len() as MemberCount; - Self::deposit_event(RawEvent::Voted(who, proposal, approve, yes_votes, no_votes)); + Self::deposit_event(Event::Voted(who, proposal, approve, yes_votes, no_votes)); - Voting::::insert(&proposal, voting); + >::insert(&proposal, voting); if is_account_voting_first_time { Ok(( @@ -576,20 +427,7 @@ decl_module! { /// + `length_bound`: The upper bound for the length of the proposal in storage. Checked via /// `storage::read` so it is `size_of::() == 4` larger than the pure length. /// - /// # - /// ## Weight - /// - `O(B + M + P1 + P2)` where: - /// - `B` is `proposal` size in bytes (length-fee-bounded) - /// - `M` is members-count (code- and governance-bounded) - /// - `P1` is the complexity of `proposal` preimage. - /// - `P2` is proposal-count (code-bounded) - /// - DB: - /// - 2 storage reads (`Members`: codec `O(M)`, `Prime`: codec `O(1)`) - /// - 3 mutations (`Voting`: codec `O(M)`, `ProposalOf`: codec `O(B)`, `Proposals`: codec `O(P2)`) - /// - any mutations done while executing `proposal` (`P1`) - /// - up to 3 events - /// # - #[weight = ( + #[pallet::weight( { let b = *length_bound; let m = T::MaxMembers::get(); @@ -600,19 +438,19 @@ decl_module! { .max(T::WeightInfo::close_approved(b, m, p2)) .max(T::WeightInfo::close_disapproved(m, p2)) .saturating_add(p1) - }, - DispatchClass::Operational + } )] - fn close(origin, + pub fn close( + origin: OriginFor, proposal_hash: T::Hash, - #[compact] index: ProposalIndex, - #[compact] proposal_weight_bound: Weight, - #[compact] length_bound: u32 + #[pallet::compact] index: ProposalIndex, + #[pallet::compact] proposal_weight_bound: Weight, + #[pallet::compact] length_bound: u32 ) -> DispatchResultWithPostInfo { let _ = ensure_signed(origin)?; - let voting = Self::voting(&proposal_hash).ok_or(Error::::ProposalMissing)?; - ensure!(voting.index == index, Error::::WrongIndex); + let voting = Self::voting(&proposal_hash).ok_or(>::ProposalMissing)?; + ensure!(voting.index == index, >::WrongIndex); let mut no_votes = voting.nays.len() as MemberCount; let mut yes_votes = voting.ayes.len() as MemberCount; @@ -626,7 +464,7 @@ decl_module! { length_bound, proposal_weight_bound, )?; - Self::deposit_event(RawEvent::Closed(proposal_hash, yes_votes, no_votes)); + Self::deposit_event(Event::Closed(proposal_hash, yes_votes, no_votes)); let (proposal_weight, proposal_count) = Self::do_approve_proposal(seats, voting, proposal_hash, proposal); return Ok(( @@ -636,7 +474,7 @@ decl_module! { ).into()); } else if disapproved { - Self::deposit_event(RawEvent::Closed(proposal_hash, yes_votes, no_votes)); + Self::deposit_event(Event::Closed(proposal_hash, yes_votes, no_votes)); let proposal_count = Self::do_disapprove_proposal(proposal_hash); return Ok(( Some(T::WeightInfo::close_early_disapproved(seats, proposal_count)), @@ -645,7 +483,7 @@ decl_module! { } // Only allow actual closing of the proposal after the voting period has ended. - ensure!(system::Module::::block_number() >= voting.end, Error::::TooEarly); + ensure!(system::Module::::block_number() >= voting.end, >::TooEarly); let prime_vote = Self::prime().map(|who| voting.ayes.iter().any(|a| a == &who)); @@ -665,7 +503,7 @@ decl_module! { length_bound, proposal_weight_bound, )?; - Self::deposit_event(RawEvent::Closed(proposal_hash, yes_votes, no_votes)); + Self::deposit_event(Event::Closed(proposal_hash, yes_votes, no_votes)); let (proposal_weight, proposal_count) = Self::do_approve_proposal(seats, voting, proposal_hash, proposal); return Ok(( @@ -674,7 +512,7 @@ decl_module! { Pays::Yes, ).into()); } else { - Self::deposit_event(RawEvent::Closed(proposal_hash, yes_votes, no_votes)); + Self::deposit_event(Event::Closed(proposal_hash, yes_votes, no_votes)); let proposal_count = Self::do_disapprove_proposal(proposal_hash); return Ok(( Some(T::WeightInfo::close_disapproved(seats, proposal_count)), @@ -683,29 +521,197 @@ decl_module! { } } - /// Disapprove a proposal, close, and remove it from the system, regardless of its current state. + /// Disapprove a proposal, close, and remove it from the system, + /// regardless of its current state. /// /// Must be called by the Root origin. /// /// Parameters: /// * `proposal_hash`: The hash of the proposal that should be disapproved. - /// - /// # - /// Complexity: O(P) where P is the number of max proposals - /// DB Weight: - /// * Reads: Proposals - /// * Writes: Voting, Proposals, ProposalOf - /// # - #[weight = T::WeightInfo::disapprove_proposal(T::MaxProposals::get())] - fn disapprove_proposal(origin, proposal_hash: T::Hash) -> DispatchResultWithPostInfo { + #[pallet::weight( + T::WeightInfo::disapprove_proposal(T::MaxProposals::get()) + )] + pub fn disapprove_proposal( + origin: OriginFor, + proposal_hash: T::Hash + ) -> DispatchResultWithPostInfo { ensure_root(origin)?; let proposal_count = Self::do_disapprove_proposal(proposal_hash); Ok(Some(T::WeightInfo::disapprove_proposal(proposal_count)).into()) } } + + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + #[pallet::metadata(T::AccountId = "AccountId", T::Hash = "Hash")] + pub enum Event, I: 'static = ()> { + /// A motion (given hash) has been proposed (by given account) with a threshold (given + /// `MemberCount`). + /// \[account, proposal_index, proposal_hash, threshold\] + Proposed(T::AccountId, ProposalIndex, T::Hash, MemberCount), + /// A motion (given hash) has been voted on by given account, leaving + /// a tally (yes votes and no votes given respectively as `MemberCount`). + /// \[account, proposal_hash, voted, yes, no\] + Voted(T::AccountId, T::Hash, bool, MemberCount, MemberCount), + /// A motion was approved by the required threshold. + /// \[proposal_hash\] + Approved(T::Hash), + /// A motion was not approved by the required threshold. + /// \[proposal_hash\] + Disapproved(T::Hash), + /// A motion was executed; result will be `Ok` if it returned without error. + /// \[proposal_hash, result\] + Executed(T::Hash, DispatchResult), + /// A single member did some action; result will be `Ok` if it returned without error. + /// \[proposal_hash, result\] + MemberExecuted(T::Hash, DispatchResult), + /// A proposal was closed because its threshold was reached or after its duration was up. + /// \[proposal_hash, yes, no\] + Closed(T::Hash, MemberCount, MemberCount), + } + + /// Old name generated by `decl_event`. + // TODO :: Have to re-check is "Note message" required ? + // #[deprecated(note = "use `Event` instead")] + pub type RawEvent = Event; + + #[pallet::error] + pub enum Error { + /// Account is not a member + NotMember, + /// Duplicate proposals not allowed + DuplicateProposal, + /// Proposal must exist + ProposalMissing, + /// Mismatched index + WrongIndex, + /// Duplicate vote ignored + DuplicateVote, + /// Members are already initialized! + AlreadyInitialized, + /// The close call was made too early, before the end of the voting. + TooEarly, + /// There can only be a maximum of `MaxProposals` active proposals. + TooManyProposals, + /// The given weight bound for the proposal was too low. + WrongProposalWeight, + /// The given length bound for the proposal was too low. + WrongProposalLength, + } + + /// The hashes of the active proposals. + #[pallet::storage] + #[pallet::getter(fn proposals)] + pub type Proposals, I: 'static = ()> = StorageValue<_, Vec, ValueQuery>; + + /// Actual proposal for a given hash, if it's current. + #[pallet::storage] + #[pallet::getter(fn proposal_of)] + pub type ProposalOf, I: 'static = ()> = StorageMap< + _, + Identity, + T::Hash, + T::Proposal, + OptionQuery, + >; + + /// Votes on a given proposal, if it is ongoing. + #[pallet::storage] + #[pallet::getter(fn voting)] + pub type Voting, I: 'static = ()> = StorageMap< + _, + Identity, + T::Hash, + Votes, + OptionQuery + >; + + /// Proposals so far. + #[pallet::storage] + #[pallet::getter(fn proposal_count)] + pub type ProposalCount, I: 'static = ()> = StorageValue<_, u32, ValueQuery>; + + /// The current members of the collective. This is stored sorted (just by value). + #[pallet::storage] + #[pallet::getter(fn members)] + pub type Members, I: 'static = ()> = StorageValue<_, Vec, ValueQuery>; + + /// The prime member that helps determine the default vote behavior in case of absentations. + #[pallet::storage] + #[pallet::getter(fn prime)] + pub type Prime, I: 'static = ()> = StorageValue<_, T::AccountId, OptionQuery>; + + #[pallet::genesis_config] + pub struct GenesisConfig, I: 'static = ()>{ + pub members: Vec, + pub phantom: sp_std::marker::PhantomData<(T, I)>, + } + + #[cfg(feature = "std")] + impl, I: 'static> Default for GenesisConfig { + fn default() -> Self { + Self { + members: Default::default(), + phantom: Default::default(), + } + } + } + + #[pallet::genesis_build] + impl, I: 'static> GenesisBuild for GenesisConfig { + fn build(&self) { + Pallet::::initialize_members(&self.members); + } + } } -impl, I: Instance> Module { +#[cfg(feature = "std")] +impl, I: 'static> GenesisConfig { + /// Direct implementation of `GenesisBuild::build_storage`. + /// + /// Kept in order not to break dependency. + pub fn build_storage(&self) -> Result { + >::build_storage(self) + } + + /// Direct implementation of `GenesisBuild::assimilate_storage`. + /// + /// Kept in order not to break dependency. + pub fn assimilate_storage( + &self, + storage: &mut sp_runtime::Storage + ) -> Result<(), String> { + >::assimilate_storage(self, storage) + } +} + +/// Origin for the collective module. +#[derive(PartialEq, Eq, Clone, RuntimeDebug, Encode, Decode)] +pub enum RawOrigin { + /// It has been condoned by a given number of members of the collective from a given total. + Members(MemberCount, MemberCount), + /// It has been condoned by a single member of the collective. + Member(AccountId), + /// Dummy to manage the fact we have instancing. + _Phantom(sp_std::marker::PhantomData), +} + +#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug)] +/// Info for keeping track of a motion being voted on. +pub struct Votes { + /// The proposal's unique index. + index: ProposalIndex, + /// The number of approval votes that are needed to pass the motion. + threshold: MemberCount, + /// The current set of voters that approved it. + ayes: Vec, + /// The current set of voters that rejected it. + nays: Vec, + /// The hard end time of this vote. + end: BlockNumber, +} + +impl, I: 'static> Pallet { /// Check whether `who` is a member of the collective. pub fn is_member(who: &T::AccountId) -> bool { // Note: The dispatchables *do not* use this to check membership so make sure @@ -726,40 +732,29 @@ impl, I: Instance> Module { // read the length of the proposal storage entry directly let proposal_len = storage::read(&key, &mut [0; 0], 0) .ok_or(Error::::ProposalMissing)?; + ensure!(proposal_len <= length_bound, Error::::WrongProposalLength); + let proposal = ProposalOf::::get(hash).ok_or(Error::::ProposalMissing)?; let proposal_weight = proposal.get_dispatch_info().weight; ensure!(proposal_weight <= weight_bound, Error::::WrongProposalWeight); Ok((proposal, proposal_len as usize)) } - /// Weight: - /// If `approved`: - /// - the weight of `proposal` preimage. - /// - two events deposited. - /// - two removals, one mutation. - /// - computation and i/o `O(P + L)` where: - /// - `P` is number of active proposals, - /// - `L` is the encoded length of `proposal` preimage. - /// - /// If not `approved`: - /// - one event deposited. - /// Two removals, one mutation. - /// Computation and i/o `O(P)` where: - /// - `P` is number of active proposals + // Approve proposal fn do_approve_proposal( seats: MemberCount, voting: Votes, proposal_hash: T::Hash, proposal: >::Proposal, ) -> (Weight, u32) { - Self::deposit_event(RawEvent::Approved(proposal_hash)); + Self::deposit_event(Event::Approved(proposal_hash)); let dispatch_weight = proposal.get_dispatch_info().weight; let origin = RawOrigin::Members(voting.threshold, seats).into(); let result = proposal.dispatch(origin); Self::deposit_event( - RawEvent::Executed(proposal_hash, result.map(|_| ()).map_err(|e| e.error)) + Event::Executed(proposal_hash, result.map(|_| ()).map_err(|e| e.error)) ); // default to the dispatch info weight for safety let proposal_weight = get_result_weight(result).unwrap_or(dispatch_weight); // P1 @@ -768,9 +763,10 @@ impl, I: Instance> Module { (proposal_weight, proposal_count) } + // disapprove proposal from the pallet fn do_disapprove_proposal(proposal_hash: T::Hash) -> u32 { // disapproved - Self::deposit_event(RawEvent::Disapproved(proposal_hash)); + Self::deposit_event(Event::Disapproved(proposal_hash)); Self::remove_proposal(proposal_hash) } @@ -787,24 +783,11 @@ impl, I: Instance> Module { } } -impl, I: Instance> ChangeMembers for Module { +impl, I: 'static> ChangeMembers for Pallet { /// Update the members of the collective. Votes are updated and the prime is reset. /// /// NOTE: Does not enforce the expected `MaxMembers` limit on the amount of members, but /// the weight estimations rely on it to estimate dispatchable weight. - /// - /// # - /// ## Weight - /// - `O(MP + N)` - /// - where `M` old-members-count (governance-bounded) - /// - where `N` new-members-count (governance-bounded) - /// - where `P` proposals-count - /// - DB: - /// - 1 storage read (codec `O(P)`) for reading the proposals - /// - `P` storage mutations for updating the votes (codec `O(M)`) - /// - 1 storage write (codec `O(N)`) for storing the new members - /// - 1 storage write (codec `O(1)`) for deleting the old prime - /// # fn change_members_sorted( _incoming: &[T::AccountId], outgoing: &[T::AccountId], @@ -846,7 +829,17 @@ impl, I: Instance> ChangeMembers for Module { } } -impl, I: Instance> InitializeMembers for Module { +/// Return the weight of a dispatch call result as an `Option`. +/// +/// Will return the weight regardless of what the state of the result is. +fn get_result_weight(result: DispatchResultWithPostInfo) -> Option { + match result { + Ok(post_info) => post_info.actual_weight, + Err(err) => err.post_info.actual_weight, + } +} + +impl, I: 'static> InitializeMembers for Pallet { fn initialize_members(members: &[T::AccountId]) { if !members.is_empty() { assert!(>::get().is_empty(), "Members are already initialized!"); @@ -868,7 +861,7 @@ where } } -pub struct EnsureMember(sp_std::marker::PhantomData<(AccountId, I)>); +pub struct EnsureMember(sp_std::marker::PhantomData<(AccountId, I)>); impl< O: Into, O>> + From>, AccountId: Default, @@ -888,7 +881,7 @@ impl< } } -pub struct EnsureMembers(sp_std::marker::PhantomData<(N, AccountId, I)>); +pub struct EnsureMembers(sp_std::marker::PhantomData<(N, AccountId, I)>); impl< O: Into, O>> + From>, N: U32, @@ -909,7 +902,7 @@ impl< } } -pub struct EnsureProportionMoreThan( +pub struct EnsureProportionMoreThan( sp_std::marker::PhantomData<(N, D, AccountId, I)> ); impl< @@ -933,7 +926,7 @@ impl< } } -pub struct EnsureProportionAtLeast( +pub struct EnsureProportionAtLeast( sp_std::marker::PhantomData<(N, D, AccountId, I)> ); impl< @@ -957,10 +950,14 @@ impl< } } + #[cfg(test)] mod tests { use super::*; - use frame_support::{Hashable, assert_ok, assert_noop, parameter_types}; + use frame_support::{ + Hashable, assert_ok, assert_noop, parameter_types, + weights::{Pays}, + }; use frame_system::{self as system, EventRecord, Phase}; use hex_literal::hex; use sp_core::H256; @@ -1043,9 +1040,9 @@ mod tests { UncheckedExtrinsic = UncheckedExtrinsic { System: system::{Module, Call, Event}, - Collective: collective::::{Module, Call, Event, Origin, Config}, - CollectiveMajority: collective::::{Module, Call, Event, Origin, Config}, - DefaultCollective: collective::{Module, Call, Event, Origin, Config}, + Collective: collective::::{Module, Call, Storage, Event, Origin, Config}, + CollectiveMajority: collective::::{Module, Call, Storage, Event, Origin, Config}, + DefaultCollective: collective::{Module, Call, Storage, Event, Origin, Config}, } ); From a2aa4ef6ad871ac9a665487c69b2f55e707acddf Mon Sep 17 00:00:00 2001 From: RK Date: Wed, 24 Feb 2021 19:49:31 +0530 Subject: [PATCH 02/13] Update frame/collective/src/lib.rs Co-authored-by: Guillaume Thiolliere --- frame/collective/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 435912d573e45..7bb56c73cd0bd 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -324,7 +324,7 @@ pub mod pallet { let active_proposals = >::try_mutate(|proposals| -> Result { proposals.push(proposal_hash); - ensure!(proposals.len() <= T::MaxProposals::get() as usize,>::TooManyProposals); + ensure!(proposals.len() <= T::MaxProposals::get() as usize, >::TooManyProposals); Ok(proposals.len()) })?; let index = Self::proposal_count(); From c464d0d4d14e32b6fac207d9e8b00121b00b6bd6 Mon Sep 17 00:00:00 2001 From: RK Date: Wed, 24 Feb 2021 19:49:43 +0530 Subject: [PATCH 03/13] Update frame/collective/src/lib.rs Co-authored-by: Guillaume Thiolliere --- frame/collective/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 7bb56c73cd0bd..4af20cd91be75 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -571,8 +571,7 @@ pub mod pallet { } /// Old name generated by `decl_event`. - // TODO :: Have to re-check is "Note message" required ? - // #[deprecated(note = "use `Event` instead")] + #[deprecated(note = "use `Event` instead")] pub type RawEvent = Event; #[pallet::error] From dece855835c63f87ab954d7357e5db9e46bbb11d Mon Sep 17 00:00:00 2001 From: shamb0 Date: Wed, 24 Feb 2021 22:14:34 +0530 Subject: [PATCH 04/13] wk2108 | port to frame v2 | weight review comments fix --- frame/collective/src/lib.rs | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 4af20cd91be75..c3826a22ef648 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -195,11 +195,13 @@ pub mod pallet { /// /// NOTE: Does not enforce the expected `MaxMembers` limit on the amount of members, but /// the weight estimations rely on it to estimate dispatchable weight. - #[pallet::weight( + #[pallet::weight(( T::WeightInfo::set_members( *old_count, // M new_members.len() as u32, // N T::MaxProposals::get() // P + ), + DispatchClass::Operational ))] pub fn set_members( origin: OriginFor, @@ -239,12 +241,13 @@ pub mod pallet { /// Dispatch a proposal from a member using the `Member` origin. /// /// Origin must be a member of the collective. - #[pallet::weight( + #[pallet::weight(( T::WeightInfo::execute( *length_bound, // B T::MaxMembers::get(), // M - ) - )] + ).saturating_add(proposal.get_dispatch_info().weight), // P + DispatchClass::Operational, + ))] pub fn execute( origin: OriginFor, proposal: Box<>::Proposal>, @@ -277,7 +280,7 @@ pub mod pallet { /// `threshold` determines whether `proposal` is executed directly (`threshold < 2`) /// or put up for voting. /// - #[pallet::weight( + #[pallet::weight(( if *threshold < 2 { T::WeightInfo::propose_execute( *length_bound, // B @@ -289,8 +292,9 @@ pub mod pallet { T::MaxMembers::get(), // M T::MaxProposals::get(), // P2 ) - } - )] + }, + DispatchClass::Operational + ))] pub fn propose( origin: OriginFor, #[pallet::compact] threshold: MemberCount, @@ -351,7 +355,10 @@ pub mod pallet { /// /// Transaction fees will be waived if the member is voting on any particular proposal /// for the first time and the call is successful. Subsequent vote changes will charge a fee. - #[pallet::weight(T::WeightInfo::vote(T::MaxMembers::get()))] + #[pallet::weight(( + T::WeightInfo::vote(T::MaxMembers::get()), + DispatchClass::Operational + ))] pub fn vote( origin: OriginFor, proposal: T::Hash, @@ -427,7 +434,7 @@ pub mod pallet { /// + `length_bound`: The upper bound for the length of the proposal in storage. Checked via /// `storage::read` so it is `size_of::() == 4` larger than the pure length. /// - #[pallet::weight( + #[pallet::weight(( { let b = *length_bound; let m = T::MaxMembers::get(); @@ -438,8 +445,9 @@ pub mod pallet { .max(T::WeightInfo::close_approved(b, m, p2)) .max(T::WeightInfo::close_disapproved(m, p2)) .saturating_add(p1) - } - )] + }, + DispatchClass::Operational + ))] pub fn close( origin: OriginFor, proposal_hash: T::Hash, From bcf3615320ea258aa8a07c7ddb89b07407fe5c6b Mon Sep 17 00:00:00 2001 From: shamb0 Date: Mon, 29 Mar 2021 19:45:11 +0530 Subject: [PATCH 05/13] pallet-collective frame v2 port rumtime migration updates --- frame/collective/src/lib.rs | 45 ++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 8bb5e0fc47c3b..28866b7ed93e6 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -49,12 +49,16 @@ use sp_runtime::{RuntimeDebug, traits::Hash}; use frame_support::{ codec::{Decode, Encode}, + storage::migration::{move_storage_from_pallet}, dispatch::{ DispatchError, DispatchResultWithPostInfo, Dispatchable, PostDispatchInfo, }, ensure, - traits::{ChangeMembers, EnsureOrigin, Get, InitializeMembers}, + traits::{ + ChangeMembers, EnsureOrigin, + Get, InitializeMembers, PalletInfo, + }, weights::{GetDispatchInfo, Weight}, }; #[cfg(feature = "std")] @@ -179,6 +183,20 @@ pub mod pallet { #[pallet::hooks] impl, I: 'static> Hooks> for Pallet { + fn on_runtime_upgrade() -> frame_support::weights::Weight { + if !UpgradedToTripleRefCount::::get() { + UpgradedToTripleRefCount::::put(true); + migrations::migrate_to_triple_ref_count::() + } else { + 0 + } + } + + fn integrity_test() { + T::BlockWeights::get() + .validate() + .expect("The weights are invalid."); + } } #[pallet::call] @@ -650,6 +668,12 @@ pub mod pallet { #[pallet::getter(fn prime)] pub type Prime, I: 'static = ()> = StorageValue<_, T::AccountId, OptionQuery>; + /// True if we have upgraded so that AccountInfo contains three types of `RefCount`. False + /// (default) if not. + #[pallet::storage] + pub(super) type UpgradedToTripleRefCount, I: 'static = ()> = + StorageValue<_, bool, ValueQuery>; + #[pallet::genesis_config] pub struct GenesisConfig, I: 'static = ()>{ pub members: Vec, @@ -669,11 +693,30 @@ pub mod pallet { #[pallet::genesis_build] impl, I: 'static> GenesisBuild for GenesisConfig { fn build(&self) { + >::put(false); Pallet::::initialize_members(&self.members); } } } +mod migrations { + use super::*; + + pub fn migrate_to_triple_ref_count, I: 'static>() -> frame_support::weights::Weight { + + let new_name = T::PalletInfo::name::>() + .expect("Fatal Error Invalid PalletInfo name"); + move_storage_from_pallet(b"Proposals", b"Collective", new_name.as_bytes()); + move_storage_from_pallet(b"ProposalOf", b"Collective", new_name.as_bytes()); + move_storage_from_pallet(b"Voting", b"Collective", new_name.as_bytes()); + move_storage_from_pallet(b"ProposalCount", b"Collective", new_name.as_bytes()); + move_storage_from_pallet(b"Members", b"Collective", new_name.as_bytes()); + move_storage_from_pallet(b"Prime", b"Collective", new_name.as_bytes()); + + T::BlockWeights::get().max_block + } +} + #[cfg(feature = "std")] impl, I: 'static> GenesisConfig { /// Direct implementation of `GenesisBuild::build_storage`. From e35a1f14a6897f0a17caf75c861ae30600b3e5b3 Mon Sep 17 00:00:00 2001 From: shamb0 Date: Tue, 30 Mar 2021 20:26:46 +0530 Subject: [PATCH 06/13] pallet-collective frame v2 port fix review remarks on runtime upgrade --- frame/collective/src/lib.rs | 32 +++++++++++++++++++------------- frame/system/src/lib.rs | 6 ------ 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 28866b7ed93e6..5b45c76c4f10e 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -184,9 +184,9 @@ pub mod pallet { #[pallet::hooks] impl, I: 'static> Hooks> for Pallet { fn on_runtime_upgrade() -> frame_support::weights::Weight { - if !UpgradedToTripleRefCount::::get() { - UpgradedToTripleRefCount::::put(true); - migrations::migrate_to_triple_ref_count::() + if !UpgradedPalletPrefix::::get() { + UpgradedPalletPrefix::::put(true); + migrations::migrate_to_new_pallet_prefix::() } else { 0 } @@ -671,7 +671,7 @@ pub mod pallet { /// True if we have upgraded so that AccountInfo contains three types of `RefCount`. False /// (default) if not. #[pallet::storage] - pub(super) type UpgradedToTripleRefCount, I: 'static = ()> = + pub(super) type UpgradedPalletPrefix, I: 'static = ()> = StorageValue<_, bool, ValueQuery>; #[pallet::genesis_config] @@ -693,7 +693,7 @@ pub mod pallet { #[pallet::genesis_build] impl, I: 'static> GenesisBuild for GenesisConfig { fn build(&self) { - >::put(false); + >::put(true); Pallet::::initialize_members(&self.members); } } @@ -702,16 +702,22 @@ pub mod pallet { mod migrations { use super::*; - pub fn migrate_to_triple_ref_count, I: 'static>() -> frame_support::weights::Weight { + pub fn migrate_to_new_pallet_prefix, I: 'static>() -> frame_support::weights::Weight { let new_name = T::PalletInfo::name::>() - .expect("Fatal Error Invalid PalletInfo name"); - move_storage_from_pallet(b"Proposals", b"Collective", new_name.as_bytes()); - move_storage_from_pallet(b"ProposalOf", b"Collective", new_name.as_bytes()); - move_storage_from_pallet(b"Voting", b"Collective", new_name.as_bytes()); - move_storage_from_pallet(b"ProposalCount", b"Collective", new_name.as_bytes()); - move_storage_from_pallet(b"Members", b"Collective", new_name.as_bytes()); - move_storage_from_pallet(b"Prime", b"Collective", new_name.as_bytes()); + .expect("Fatal Error Invalid PalletInfo name") + .as_bytes(); + + if new_name == b"Collective" { + return 0 + } + + move_storage_from_pallet(b"Proposals", b"Collective", new_name); + move_storage_from_pallet(b"ProposalOf", b"Collective", new_name); + move_storage_from_pallet(b"Voting", b"Collective", new_name); + move_storage_from_pallet(b"ProposalCount", b"Collective", new_name); + move_storage_from_pallet(b"Members", b"Collective", new_name); + move_storage_from_pallet(b"Prime", b"Collective", new_name); T::BlockWeights::get().max_block } diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 9d3ecd6f41f5d..9688494b20ad2 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -269,12 +269,6 @@ pub mod pallet { 0 } } - - fn integrity_test() { - T::BlockWeights::get() - .validate() - .expect("The weights are invalid."); - } } #[pallet::call] From 5e1b90b71d242c73c9c84d3814d65800d9a7ac69 Mon Sep 17 00:00:00 2001 From: shamb0 Date: Wed, 31 Mar 2021 13:42:18 +0530 Subject: [PATCH 07/13] pallet-collective frame v2 port fix review remarks on runtime upgrade --- frame/collective/src/lib.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 5b45c76c4f10e..9d9b0277479ee 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -191,12 +191,6 @@ pub mod pallet { 0 } } - - fn integrity_test() { - T::BlockWeights::get() - .validate() - .expect("The weights are invalid."); - } } #[pallet::call] From 0a334eb0054662a6230028b97e46085146d3c2b3 Mon Sep 17 00:00:00 2001 From: shamb0 Date: Thu, 1 Apr 2021 13:17:38 +0530 Subject: [PATCH 08/13] pallet-collective port frame v2 runtime migration unit test update --- frame/collective/src/lib.rs | 150 +++++++++++++++++++++++++++++++++++- 1 file changed, 147 insertions(+), 3 deletions(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 9d9b0277479ee..1de85b31a0188 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -184,7 +184,7 @@ pub mod pallet { #[pallet::hooks] impl, I: 'static> Hooks> for Pallet { fn on_runtime_upgrade() -> frame_support::weights::Weight { - if !UpgradedPalletPrefix::::get() { + if !Self::upgraded_pallet_prefix() { UpgradedPalletPrefix::::put(true); migrations::migrate_to_new_pallet_prefix::() } else { @@ -662,9 +662,9 @@ pub mod pallet { #[pallet::getter(fn prime)] pub type Prime, I: 'static = ()> = StorageValue<_, T::AccountId, OptionQuery>; - /// True if we have upgraded so that AccountInfo contains three types of `RefCount`. False /// (default) if not. #[pallet::storage] + #[pallet::getter(fn upgraded_pallet_prefix)] pub(super) type UpgradedPalletPrefix, I: 'static = ()> = StorageValue<_, bool, ValueQuery>; @@ -1009,13 +1009,17 @@ mod tests { use super::*; use frame_support::{ Hashable, assert_ok, assert_noop, parameter_types, + Identity, weights::{Pays}, + pallet_prelude::{StorageValue, StorageMap}, + traits::{OnRuntimeUpgrade}, }; use frame_system::{self as system, EventRecord, Phase}; use hex_literal::hex; use sp_core::H256; use sp_runtime::{ - traits::{BlakeTwo256, IdentityLookup}, testing::Header, + traits::{BlakeTwo256, IdentityLookup}, + testing::Header, BuildStorage, }; use crate as collective; @@ -1780,4 +1784,144 @@ mod tests { }) } + #[test] + fn on_runtime_upgrade_works() { + + struct OldProposalsPrefix; + impl frame_support::traits::StorageInstance for OldProposalsPrefix { + const STORAGE_PREFIX: &'static str = "Proposals"; + fn pallet_prefix() -> &'static str { + "Collective" + } + } + type OldProposals = StorageValue>; + + struct OldProposalOfPrefix; + impl frame_support::traits::StorageInstance for OldProposalOfPrefix { + const STORAGE_PREFIX: &'static str = "ProposalOf"; + fn pallet_prefix() -> &'static str { + "Collective" + } + } + type OldProposalOf = StorageMap; + + struct OldVotingPrefix; + impl frame_support::traits::StorageInstance for OldVotingPrefix { + const STORAGE_PREFIX: &'static str = "Voting"; + fn pallet_prefix() -> &'static str { + "Collective" + } + } + type OldVoting = StorageMap>; + + struct OldProposalCountPrefix; + impl frame_support::traits::StorageInstance for OldProposalCountPrefix { + const STORAGE_PREFIX: &'static str = "ProposalCount"; + fn pallet_prefix() -> &'static str { + "Collective" + } + } + type OldProposalCount = StorageValue; + + struct OldMembersPrefix; + impl frame_support::traits::StorageInstance for OldMembersPrefix { + const STORAGE_PREFIX: &'static str = "Members"; + fn pallet_prefix() -> &'static str { + "Collective" + } + } + type OldMembers = StorageValue>; + + struct OldPrimePrefix; + impl frame_support::traits::StorageInstance for OldPrimePrefix { + const STORAGE_PREFIX: &'static str = "Prime"; + fn pallet_prefix() -> &'static str { + "Collective" + } + } + type OldPrime = StorageValue; + + new_test_ext().execute_with(|| { + + let end = 4; + + collective::UpgradedPalletPrefix::::put(false); + + // Storage Item "Proposals" + let proposal = make_proposal(42); + let hash: H256 = proposal.blake2_256().into(); + OldProposals::put(vec![hash]); + + assert_eq!(OldProposals::get(), Some(vec![hash])); + assert_eq!(collective::Proposals::::get(), []); + + // Storage Item "ProposalOf" + OldProposalOf::insert(hash, proposal); + + assert_eq!(OldProposalOf::iter().collect::>().len(), 1); + assert_eq!(collective::ProposalOf::::iter().collect::>().len(), 0); + + // Storage Item "Votes" + OldVoting::insert(hash, + Votes { + index: 0, + threshold: 2, + ayes: vec![], + nays: vec![1], + end + }, + ); + + assert_eq!(OldVoting::iter().collect::>().len(), 1); + assert_eq!(collective::Voting::::iter().collect::>().len(), 0); + + // Storage Item "ProposalCount" + OldProposalCount::put(10); + + assert_eq!(OldProposalCount::get(), Some(10)); + assert_eq!(collective::ProposalCount::::get(), 0); + + // Storage Item "Members" + OldMembers::put(vec![2]); + + assert_eq!(OldMembers::get(), Some(vec![2])); + assert_eq!(collective::Members::::get().is_empty(), true); + + // Storage Item "Prime" + OldPrime::put(2); + + assert_eq!(OldPrime::get(), Some(2)); + assert_eq!(collective::Prime::::get(), None); + + // Trigger runtime upgraade + AllPallets::on_runtime_upgrade(); + + // Storage Item "Proposals" + assert_eq!(OldProposals::get(), None); + assert_eq!(collective::Proposals::::get(), vec![hash]); + + // Storage Item "ProposalOf" + assert_eq!(OldProposalOf::iter().collect::>().len(), 0); + assert_eq!(collective::ProposalOf::::iter().collect::>().len(), 1); + + // Storage Item "Voting" + assert_eq!(OldVoting::iter().collect::>().len(), 0); + assert_eq!(collective::Voting::::iter().collect::>().len(), 1); + + // Storage Item "ProposalCount" + assert_eq!(OldProposalCount::get(), None); + assert_eq!(collective::ProposalCount::::get(), 10); + + // Storage Item "Members" + assert_eq!(OldMembers::get(), None); + assert_eq!(collective::Members::::get(), [2]); + + // Storage Item "Prime" + assert_eq!(OldPrime::get(), None); + assert_eq!(collective::Prime::::get(), Some(2)); + + + }) + } + } From 3ffae36c5f8c68be0424ebd7b419aeee2f097a76 Mon Sep 17 00:00:00 2001 From: shamb0 Date: Sun, 4 Apr 2021 17:44:47 +0530 Subject: [PATCH 09/13] review comment closure --- frame/collective/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index c0de6d9e51f2d..30b179d099f57 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -191,6 +191,13 @@ pub mod pallet { 0 } } + + // It is an extra not really part of the PR, + fn integrity_test() { + T::BlockWeights::get() + .validate() + .expect("The weights are invalid."); + } } #[pallet::call] From 6f7991ccacbaa449be1a0a2ff76ab8d86a0f1dc3 Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Tue, 13 Apr 2021 15:02:01 +0200 Subject: [PATCH 10/13] Update frame/system/src/lib.rs --- frame/system/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index b860e1e38fd5f..d8a50f9f7a186 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -286,6 +286,12 @@ pub mod pallet { 0 } } + + fn integrity_test() { + T::BlockWeights::get() + .validate() + .expect("The weights are invalid."); + } } #[pallet::call] From b718f80f098542d9dc2fc8b4edbbe32c39674530 Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Thu, 15 Apr 2021 11:25:17 +0200 Subject: [PATCH 11/13] Update frame/collective/src/lib.rs --- frame/collective/src/lib.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index cee604cab2899..a16b5a0e0fcea 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -710,9 +710,6 @@ mod migrations { .expect("Fatal Error Invalid PalletInfo name") .as_bytes(); - if new_name == b"Collective" { - return 0 - } move_storage_from_pallet(b"Proposals", b"Collective", new_name); move_storage_from_pallet(b"ProposalOf", b"Collective", new_name); From 545fb1df57ffaf77043bcb65913aecaa8ae628a5 Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Thu, 15 Apr 2021 11:25:24 +0200 Subject: [PATCH 12/13] Update frame/collective/src/lib.rs --- frame/collective/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index a16b5a0e0fcea..3d15cc6a58c39 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -727,6 +727,7 @@ impl, I: 'static> GenesisConfig { /// Direct implementation of `GenesisBuild::build_storage`. /// /// Kept in order not to break dependency. + #[deprecated(note = "use [`GenesisBuild::build_storage`] instead")] pub fn build_storage(&self) -> Result { >::build_storage(self) } From 86d8a54f9f17a6694b651bbe42681b4bfae30fca Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Thu, 15 Apr 2021 11:25:30 +0200 Subject: [PATCH 13/13] Update frame/collective/src/lib.rs --- frame/collective/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 3d15cc6a58c39..fffb3c0f050d5 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -735,6 +735,7 @@ impl, I: 'static> GenesisConfig { /// Direct implementation of `GenesisBuild::assimilate_storage`. /// /// Kept in order not to break dependency. + #[deprecated(note = "use [`GenesisBuild::assimilate_storage`] instead")] pub fn assimilate_storage( &self, storage: &mut sp_runtime::Storage