From 53dad99186fd5f3d4d26cd97b5e743a412a5a7c4 Mon Sep 17 00:00:00 2001 From: muraca Date: Sat, 8 Jul 2023 12:11:52 +0200 Subject: [PATCH] addressed some review comments Signed-off-by: muraca --- frame/alliance/Cargo.toml | 1 - frame/alliance/src/mock.rs | 2 +- frame/collective/Cargo.toml | 1 - frame/collective/src/lib.rs | 106 ++++++++++++++++------------------ frame/collective/src/tests.rs | 11 +++- frame/utility/src/tests.rs | 2 +- 6 files changed, 60 insertions(+), 63 deletions(-) diff --git a/frame/alliance/Cargo.toml b/frame/alliance/Cargo.toml index 0d8414e7ac435..d37edec2874c8 100644 --- a/frame/alliance/Cargo.toml +++ b/frame/alliance/Cargo.toml @@ -55,7 +55,6 @@ std = [ "frame-support/std", "frame-system/std", "pallet-identity/std", - "pallet-preimage/std", ] runtime-benchmarks = [ "array-bytes", diff --git a/frame/alliance/src/mock.rs b/frame/alliance/src/mock.rs index 6b04ae5e7dd6d..c1ec895038665 100644 --- a/frame/alliance/src/mock.rs +++ b/frame/alliance/src/mock.rs @@ -27,7 +27,7 @@ use sp_std::convert::{TryFrom, TryInto}; pub use frame_support::{ assert_noop, assert_ok, ord_parameter_types, parameter_types, - traits::{EitherOfDiverse, GenesisBuild, QueryPreimage, SortedMembers, StorePreimage}, + traits::{EitherOfDiverse, QueryPreimage, SortedMembers, StorePreimage}, BoundedVec, }; use frame_system::{EnsureRoot, EnsureSignedBy}; diff --git a/frame/collective/Cargo.toml b/frame/collective/Cargo.toml index 70ac480b15ed3..cfc7c6a426a70 100644 --- a/frame/collective/Cargo.toml +++ b/frame/collective/Cargo.toml @@ -40,7 +40,6 @@ std = [ "sp-io/std", "sp-runtime/std", "sp-std/std", - "pallet-preimage/std", ] runtime-benchmarks = [ "frame-benchmarking/runtime-benchmarks", diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index a7d551d8590ce..fbd2f2808bfd4 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -58,7 +58,7 @@ use frame_support::{ InitializeMembers, QueryPreimage, StorageVersion, StorePreimage, }, weights::Weight, - BoundedVec, CloneNoBound, EqNoBound, Parameter, PartialEqNoBound, + BoundedVec, CloneNoBound, EqNoBound, PalletError, Parameter, PartialEqNoBound, }; #[cfg(any(feature = "try-runtime", test))] @@ -351,6 +351,17 @@ pub mod pallet { WrongProposalLength, /// Prime account is not a member PrimeAccountNotMember, + /// The bound for votes was exceeded. + TooManyVotes, + /// Some error occurred that should never happen. This should be reported to the + /// maintainers. + Defensive(DefensiveError), + } + + #[derive(Encode, Decode, PartialEq, TypeInfo, PalletError, RuntimeDebug)] + pub enum DefensiveError { + /// Preimage length cannot be deduced from type. + PreimageLen, } #[pallet::hooks] @@ -660,6 +671,7 @@ fn get_result_weight(result: DispatchResultWithPostInfo) -> Option { impl, I: 'static> Pallet { /// The active proposals. + #[cfg(any(feature = "std", feature = "runtime-benchmarks"))] pub fn proposals() -> BoundedVec>::Proposal>, T::MaxProposals> { Voting::::iter() .map(|(k, _v)| k) @@ -735,13 +747,7 @@ impl, I: 'static> Pallet { >::mutate(|i| *i += 1); let votes = { let end = frame_system::Pallet::::block_number() + T::MotionDuration::get(); - Votes { - index, - threshold, - ayes: vec![].try_into().expect("empty vec is always bounded; qed"), - nays: vec![].try_into().expect("empty vec is always bounded; qed"), - end, - } + Votes { index, threshold, ayes: Default::default(), nays: Default::default(), end } }; >::insert(&proposal_bounded, votes); @@ -773,10 +779,7 @@ impl, I: 'static> Pallet { if approve { if position_yes.is_none() { - voting - .ayes - .try_push(who.clone()) - .expect("Proposal voting ayes can't overflow; qed"); + voting.ayes.try_push(who.clone()).map_err(|_| Error::::TooManyVotes)?; } else { return Err(Error::::DuplicateVote.into()) } @@ -785,10 +788,7 @@ impl, I: 'static> Pallet { } } else { if position_no.is_none() { - voting - .nays - .try_push(who.clone()) - .expect("Proposal voting nays can't overflow; qed"); + voting.nays.try_push(who.clone()).map_err(|_| Error::::TooManyVotes)?; } else { return Err(Error::::DuplicateVote.into()) } @@ -923,30 +923,19 @@ impl, I: 'static> Pallet { // read the length of the proposal by using peek let (proposal, proposal_len) = T::Preimages::peek(&proposal_bounded).map_err(|_| Error::::ProposalMissing)?; - let proposal_len = proposal_len.unwrap_or_else(|| match proposal_bounded { - Bounded::Inline(data) => data.len() as u32, - _ => panic!("preimage should be inline, or len already given by peek; qed"), - }); + let proposal_len = match proposal_len { + Some(len) => Ok(len), + None => match proposal_bounded { + Bounded::Inline(data) => Ok(data.len() as u32), + _ => Err(Error::::Defensive(DefensiveError::PreimageLen)), + }, + }?; ensure!(proposal_len <= length_bound, Error::::WrongProposalLength); let proposal_weight = proposal.get_dispatch_info().weight; ensure!(proposal_weight.all_lte(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 fn do_approve_proposal( seats: MemberCount, yes_votes: MemberCount, @@ -1082,28 +1071,33 @@ impl, I: 'static> ChangeMembers for Pallet { // remove accounts from all current voting in motions. let mut outgoing = outgoing.to_vec(); outgoing.sort(); - for h in Self::proposals().into_iter() { - >::mutate(h, |v| { - if let Some(mut votes) = v.take() { - votes.ayes = votes - .ayes - .into_iter() - .filter(|i| outgoing.binary_search(i).is_err()) - .collect::>() - .try_into() - .expect("The filtered elements should be at most equal to the original length; qed"); - votes.nays = votes - .nays - .into_iter() - .filter(|i| outgoing.binary_search(i).is_err()) - .collect::>() - .try_into() - .expect("The filtered elements should be at most equal to the original length; qed"); - - *v = Some(votes); - } - }); - } + >::translate_values( + |mut votes: Votes< + T::AccountId, + frame_system::pallet_prelude::BlockNumberFor, + T::MaxMembers, + >| { + votes.ayes = votes + .ayes + .into_iter() + .filter(|i| outgoing.binary_search(i).is_err()) + .collect::>() + .try_into() + .expect( + "The filtered elements should be at most equal to the original length; qed", + ); + votes.nays = votes + .nays + .into_iter() + .filter(|i| outgoing.binary_search(i).is_err()) + .collect::>() + .try_into() + .expect( + "The filtered elements should be at most equal to the original length; qed", + ); + Some(votes) + }, + ); Members::::put( BoundedVec::try_from(new.to_vec()).expect("Members bound was already checked; qed"), ); diff --git a/frame/collective/src/tests.rs b/frame/collective/src/tests.rs index f304fe55bb807..e990aaab7d843 100644 --- a/frame/collective/src/tests.rs +++ b/frame/collective/src/tests.rs @@ -21,7 +21,7 @@ use frame_support::{ assert_noop, assert_ok, dispatch::Pays, parameter_types, - traits::{ConstU32, ConstU64, GenesisBuild, StorageVersion}, + traits::{ConstU32, ConstU64, StorageVersion}, }; use frame_system::{EnsureRoot, EventRecord, Phase}; use sp_core::{bounded_vec, H256}; @@ -255,14 +255,19 @@ fn set_members_with_prime_works() { let members = vec![1, 2, 3]; assert_ok!(Collective::set_members( RuntimeOrigin::root(), - members.clone(), + members.clone().try_into().unwrap(), Some(3), MaxMembers::get() )); assert_eq!(Collective::members(), members.clone()); assert_eq!(Collective::prime(), Some(3)); assert_noop!( - Collective::set_members(RuntimeOrigin::root(), members, Some(4), MaxMembers::get()), + Collective::set_members( + RuntimeOrigin::root(), + members.try_into().unwrap(), + Some(4), + MaxMembers::get() + ), Error::::PrimeAccountNotMember ); }); diff --git a/frame/utility/src/tests.rs b/frame/utility/src/tests.rs index ed1e21cd7add0..5d06084e501c6 100644 --- a/frame/utility/src/tests.rs +++ b/frame/utility/src/tests.rs @@ -27,7 +27,7 @@ use frame_support::{ dispatch::{DispatchError, DispatchErrorWithPostInfo, Dispatchable, Pays}, error::BadOrigin, parameter_types, storage, - traits::{ConstU32, ConstU64, Contains, GenesisBuild, StorePreimage}, + traits::{ConstU32, ConstU64, Contains, StorePreimage}, weights::Weight, }; use pallet_collective::{EnsureProportionAtLeast, Instance1};