From f6cd78c64d3a691263b416db44526ae0c14aafa3 Mon Sep 17 00:00:00 2001 From: Matteo Muraca <56828990+muraca@users.noreply.github.com> Date: Tue, 5 Mar 2024 12:04:11 +0100 Subject: [PATCH] Removed `pallet::getter` usage from `pallet-collective` (#3456) Part of #3326 This one is easier as all the storage items are public. @ggwpez @kianenigma @shawntabrizi --------- Signed-off-by: Matteo Muraca Co-authored-by: Liam Aharon Co-authored-by: command-bot <> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- substrate/bin/node/runtime/src/impls.rs | 8 +-- substrate/frame/alliance/src/mock.rs | 2 +- substrate/frame/alliance/src/tests.rs | 4 +- .../frame/collective/src/benchmarking.rs | 32 ++++----- substrate/frame/collective/src/lib.rs | 72 +++++++++---------- substrate/frame/collective/src/tests.rs | 44 ++++++------ 6 files changed, 78 insertions(+), 84 deletions(-) diff --git a/substrate/bin/node/runtime/src/impls.rs b/substrate/bin/node/runtime/src/impls.rs index 7ff52a758b3dd..34f043b33a4ed 100644 --- a/substrate/bin/node/runtime/src/impls.rs +++ b/substrate/bin/node/runtime/src/impls.rs @@ -30,8 +30,8 @@ use pallet_identity::legacy::IdentityField; use sp_std::prelude::*; use crate::{ - AccountId, AllianceMotion, Assets, Authorship, Balances, Hash, NegativeImbalance, Runtime, - RuntimeCall, + AccountId, AllianceCollective, AllianceMotion, Assets, Authorship, Balances, Hash, + NegativeImbalance, Runtime, RuntimeCall, }; pub struct Author; @@ -107,7 +107,7 @@ impl ProposalProvider for AllianceProposalProvider } fn proposal_of(proposal_hash: Hash) -> Option { - AllianceMotion::proposal_of(proposal_hash) + pallet_collective::ProposalOf::::get(proposal_hash) } } @@ -276,7 +276,7 @@ mod multiplier_tests { let next = runtime_multiplier_update(fm); fm = next; if fm == min_multiplier() { - break + break; } iterations += 1; } diff --git a/substrate/frame/alliance/src/mock.rs b/substrate/frame/alliance/src/mock.rs index 4a65485ed8f66..fd44d33ef93ce 100644 --- a/substrate/frame/alliance/src/mock.rs +++ b/substrate/frame/alliance/src/mock.rs @@ -208,7 +208,7 @@ impl ProposalProvider for AllianceProposalProvider } fn proposal_of(proposal_hash: H256) -> Option { - AllianceMotion::proposal_of(proposal_hash) + pallet_collective::ProposalOf::::get(proposal_hash) } } diff --git a/substrate/frame/alliance/src/tests.rs b/substrate/frame/alliance/src/tests.rs index 8011627b237af..710de5a54bc95 100644 --- a/substrate/frame/alliance/src/tests.rs +++ b/substrate/frame/alliance/src/tests.rs @@ -187,8 +187,8 @@ fn propose_works() { Box::new(proposal.clone()), proposal_len )); - assert_eq!(*AllianceMotion::proposals(), vec![hash]); - assert_eq!(AllianceMotion::proposal_of(&hash), Some(proposal)); + assert_eq!(*pallet_collective::Proposals::::get(), vec![hash]); + assert_eq!(pallet_collective::ProposalOf::::get(&hash), Some(proposal)); assert_eq!( System::events(), vec![EventRecord { diff --git a/substrate/frame/collective/src/benchmarking.rs b/substrate/frame/collective/src/benchmarking.rs index af10eae5b673d..7b5df17b60a69 100644 --- a/substrate/frame/collective/src/benchmarking.rs +++ b/substrate/frame/collective/src/benchmarking.rs @@ -105,7 +105,7 @@ benchmarks_instance_pallet! { }: _(SystemOrigin::Root, new_members.clone(), new_members.last().cloned(), T::MaxMembers::get()) verify { new_members.sort(); - assert_eq!(Collective::::members(), new_members); + assert_eq!(Members::::get(), new_members); } execute { @@ -199,14 +199,14 @@ benchmarks_instance_pallet! { )?; } - assert_eq!(Collective::::proposals().len(), (p - 1) as usize); + assert_eq!(Proposals::::get().len(), (p - 1) as usize); let proposal: T::Proposal = SystemCall::::remark { remark: id_to_remark_data(p, b as usize) }.into(); }: propose(SystemOrigin::Signed(caller.clone()), threshold, Box::new(proposal.clone()), bytes_in_storage) verify { // New proposal is recorded - assert_eq!(Collective::::proposals().len(), p as usize); + assert_eq!(Proposals::::get().len(), p as usize); let proposal_hash = T::Hashing::hash_of(&proposal); assert_last_event::(Event::Proposed { account: caller, proposal_index: p - 1, proposal_hash, threshold }.into()); } @@ -269,7 +269,7 @@ benchmarks_instance_pallet! { approve, )?; - assert_eq!(Collective::::proposals().len(), p as usize); + assert_eq!(Proposals::::get().len(), p as usize); // Voter switches vote to nay, but does not kill the vote, just updates + inserts let approve = false; @@ -280,8 +280,8 @@ benchmarks_instance_pallet! { }: _(SystemOrigin::Signed(voter), last_hash, index, approve) verify { // All proposals exist and the last proposal has just been updated. - assert_eq!(Collective::::proposals().len(), p as usize); - let voting = Collective::::voting(&last_hash).ok_or("Proposal Missing")?; + assert_eq!(Proposals::::get().len(), p as usize); + let voting = Voting::::get(&last_hash).ok_or("Proposal Missing")?; assert_eq!(voting.ayes.len(), (m - 3) as usize); assert_eq!(voting.nays.len(), 1); } @@ -344,7 +344,7 @@ benchmarks_instance_pallet! { approve, )?; - assert_eq!(Collective::::proposals().len(), p as usize); + assert_eq!(Proposals::::get().len(), p as usize); // Voter switches vote to nay, which kills the vote let approve = false; @@ -361,7 +361,7 @@ benchmarks_instance_pallet! { }: close(SystemOrigin::Signed(voter), last_hash, index, Weight::MAX, bytes_in_storage) verify { // The last proposal is removed. - assert_eq!(Collective::::proposals().len(), (p - 1) as usize); + assert_eq!(Proposals::::get().len(), (p - 1) as usize); assert_last_event::(Event::Disapproved { proposal_hash: last_hash }.into()); } @@ -428,7 +428,7 @@ benchmarks_instance_pallet! { true, )?; - assert_eq!(Collective::::proposals().len(), p as usize); + assert_eq!(Proposals::::get().len(), p as usize); // Caller switches vote to aye, which passes the vote let index = p - 1; @@ -442,7 +442,7 @@ benchmarks_instance_pallet! { }: close(SystemOrigin::Signed(caller), last_hash, index, Weight::MAX, bytes_in_storage) verify { // The last proposal is removed. - assert_eq!(Collective::::proposals().len(), (p - 1) as usize); + assert_eq!(Proposals::::get().len(), (p - 1) as usize); assert_last_event::(Event::Executed { proposal_hash: last_hash, result: Ok(()) }.into()); } @@ -519,12 +519,12 @@ benchmarks_instance_pallet! { )?; System::::set_block_number(BlockNumberFor::::max_value()); - assert_eq!(Collective::::proposals().len(), p as usize); + assert_eq!(Proposals::::get().len(), p as usize); // Prime nay will close it as disapproved }: close(SystemOrigin::Signed(caller), last_hash, index, Weight::MAX, bytes_in_storage) verify { - assert_eq!(Collective::::proposals().len(), (p - 1) as usize); + assert_eq!(Proposals::::get().len(), (p - 1) as usize); assert_last_event::(Event::Disapproved { proposal_hash: last_hash }.into()); } @@ -591,12 +591,12 @@ benchmarks_instance_pallet! { // caller is prime, prime already votes aye by creating the proposal System::::set_block_number(BlockNumberFor::::max_value()); - assert_eq!(Collective::::proposals().len(), p as usize); + assert_eq!(Proposals::::get().len(), p as usize); // Prime aye will close it as approved }: close(SystemOrigin::Signed(caller), last_hash, p - 1, Weight::MAX, bytes_in_storage) verify { - assert_eq!(Collective::::proposals().len(), (p - 1) as usize); + assert_eq!(Proposals::::get().len(), (p - 1) as usize); assert_last_event::(Event::Executed { proposal_hash: last_hash, result: Ok(()) }.into()); } @@ -640,11 +640,11 @@ benchmarks_instance_pallet! { } System::::set_block_number(BlockNumberFor::::max_value()); - assert_eq!(Collective::::proposals().len(), p as usize); + assert_eq!(Proposals::::get().len(), p as usize); }: _(SystemOrigin::Root, last_hash) verify { - assert_eq!(Collective::::proposals().len(), (p - 1) as usize); + assert_eq!(Proposals::::get().len(), (p - 1) as usize); assert_last_event::(Event::Disapproved { proposal_hash: last_hash }.into()); } diff --git a/substrate/frame/collective/src/lib.rs b/substrate/frame/collective/src/lib.rs index dd481b5368146..882e99a6d0051 100644 --- a/substrate/frame/collective/src/lib.rs +++ b/substrate/frame/collective/src/lib.rs @@ -261,36 +261,30 @@ pub mod pallet { /// The hashes of the active proposals. #[pallet::storage] - #[pallet::getter(fn proposals)] pub type Proposals, I: 'static = ()> = StorageValue<_, BoundedVec, 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, >::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::event] @@ -459,7 +453,7 @@ pub mod pallet { #[pallet::compact] length_bound: u32, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - let members = Self::members(); + let members = Members::::get(); ensure!(members.contains(&who), Error::::NotMember); let proposal_len = proposal.encoded_size(); ensure!(proposal_len <= length_bound as usize, Error::::WrongProposalLength); @@ -519,7 +513,7 @@ pub mod pallet { #[pallet::compact] length_bound: u32, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - let members = Self::members(); + let members = Members::::get(); ensure!(members.contains(&who), Error::::NotMember); if threshold < 2 { @@ -565,7 +559,7 @@ pub mod pallet { approve: bool, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - let members = Self::members(); + let members = Members::::get(); ensure!(members.contains(&who), Error::::NotMember); // Detects first vote of the member in the motion @@ -669,7 +663,7 @@ impl, I: 'static> Pallet { pub fn is_member(who: &T::AccountId) -> bool { // Note: The dispatchables *do not* use this to check membership so make sure // to update those if this is changed. - Self::members().contains(who) + Members::::get().contains(who) } /// Execute immediately when adding a new proposal. @@ -688,7 +682,7 @@ impl, I: 'static> Pallet { let proposal_hash = T::Hashing::hash_of(&proposal); ensure!(!>::contains_key(proposal_hash), Error::::DuplicateProposal); - let seats = Self::members().len() as MemberCount; + let seats = Members::::get().len() as MemberCount; let result = proposal.dispatch(RawOrigin::Members(1, seats).into()); Self::deposit_event(Event::Executed { proposal_hash, @@ -721,7 +715,7 @@ impl, I: 'static> Pallet { Ok(proposals.len()) })?; - let index = Self::proposal_count(); + let index = ProposalCount::::get(); >::mutate(|i| *i += 1); >::insert(proposal_hash, proposal); let votes = { @@ -747,7 +741,7 @@ impl, I: 'static> Pallet { index: ProposalIndex, approve: bool, ) -> Result { - let mut voting = Self::voting(&proposal).ok_or(Error::::ProposalMissing)?; + let mut voting = Voting::::get(&proposal).ok_or(Error::::ProposalMissing)?; ensure!(voting.index == index, Error::::WrongIndex); let position_yes = voting.ayes.iter().position(|a| a == &who); @@ -798,12 +792,12 @@ impl, I: 'static> Pallet { proposal_weight_bound: Weight, length_bound: u32, ) -> DispatchResultWithPostInfo { - let voting = Self::voting(&proposal_hash).ok_or(Error::::ProposalMissing)?; + let voting = Voting::::get(&proposal_hash).ok_or(Error::::ProposalMissing)?; ensure!(voting.index == index, Error::::WrongIndex); let mut no_votes = voting.nays.len() as MemberCount; let mut yes_votes = voting.ayes.len() as MemberCount; - let seats = Self::members().len() as MemberCount; + let seats = Members::::get().len() as MemberCount; let approved = yes_votes >= voting.threshold; let disapproved = seats.saturating_sub(no_votes) < voting.threshold; // Allow (dis-)approving the proposal as soon as there are enough votes. @@ -837,7 +831,7 @@ impl, I: 'static> Pallet { // Only allow actual closing of the proposal after the voting period has ended. ensure!(frame_system::Pallet::::block_number() >= voting.end, Error::::TooEarly); - let prime_vote = Self::prime().map(|who| voting.ayes.iter().any(|a| a == &who)); + let prime_vote = Prime::::get().map(|who| voting.ayes.iter().any(|a| a == &who)); // default voting strategy. let default = T::DefaultVote::default_vote(prime_vote, yes_votes, no_votes, seats); @@ -978,29 +972,28 @@ impl, I: 'static> Pallet { /// * The prime account must be a member of the collective. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), TryRuntimeError> { - Self::proposals() - .into_iter() - .try_for_each(|proposal| -> Result<(), TryRuntimeError> { + Proposals::::get().into_iter().try_for_each( + |proposal| -> Result<(), TryRuntimeError> { ensure!( - Self::proposal_of(proposal).is_some(), + ProposalOf::::get(proposal).is_some(), "Proposal hash from `Proposals` is not found inside the `ProposalOf` mapping." ); Ok(()) - })?; + }, + )?; ensure!( - Self::proposals().into_iter().count() <= Self::proposal_count() as usize, + Proposals::::get().into_iter().count() <= ProposalCount::::get() as usize, "The actual number of proposals is greater than `ProposalCount`" ); ensure!( - Self::proposals().into_iter().count() == >::iter_keys().count(), + Proposals::::get().into_iter().count() == >::iter_keys().count(), "Proposal count inside `Proposals` is not equal to the proposal count in `ProposalOf`" ); - Self::proposals() - .into_iter() - .try_for_each(|proposal| -> Result<(), TryRuntimeError> { - if let Some(votes) = Self::voting(proposal) { + Proposals::::get().into_iter().try_for_each( + |proposal| -> Result<(), TryRuntimeError> { + if let Some(votes) = Voting::::get(proposal) { let ayes = votes.ayes.len(); let nays = votes.nays.len(); @@ -1010,13 +1003,13 @@ impl, I: 'static> Pallet { ); } Ok(()) - })?; + }, + )?; let mut proposal_indices = vec![]; - Self::proposals() - .into_iter() - .try_for_each(|proposal| -> Result<(), TryRuntimeError> { - if let Some(votes) = Self::voting(proposal) { + Proposals::::get().into_iter().try_for_each( + |proposal| -> Result<(), TryRuntimeError> { + if let Some(votes) = Voting::::get(proposal) { let proposal_index = votes.index; ensure!( !proposal_indices.contains(&proposal_index), @@ -1025,12 +1018,13 @@ impl, I: 'static> Pallet { proposal_indices.push(proposal_index); } Ok(()) - })?; + }, + )?; >::iter_keys().try_for_each( |proposal_hash| -> Result<(), TryRuntimeError> { ensure!( - Self::proposals().contains(&proposal_hash), + Proposals::::get().contains(&proposal_hash), "`Proposals` doesn't contain the proposal hash from the `Voting` storage map." ); Ok(()) @@ -1038,17 +1032,17 @@ impl, I: 'static> Pallet { )?; ensure!( - Self::members().len() <= T::MaxMembers::get() as usize, + Members::::get().len() <= T::MaxMembers::get() as usize, "The member count is greater than `MaxMembers`." ); ensure!( - Self::members().windows(2).all(|members| members[0] <= members[1]), + Members::::get().windows(2).all(|members| members[0] <= members[1]), "The members are not sorted by value." ); - if let Some(prime) = Self::prime() { - ensure!(Self::members().contains(&prime), "Prime account is not a member."); + if let Some(prime) = Prime::::get() { + ensure!(Members::::get().contains(&prime), "Prime account is not a member."); } Ok(()) @@ -1082,7 +1076,7 @@ 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() { + for h in Proposals::::get().into_iter() { >::mutate(h, |v| { if let Some(mut votes) = v.take() { votes.ayes = votes diff --git a/substrate/frame/collective/src/tests.rs b/substrate/frame/collective/src/tests.rs index ac5797c638cb1..92418794c5fb7 100644 --- a/substrate/frame/collective/src/tests.rs +++ b/substrate/frame/collective/src/tests.rs @@ -193,8 +193,8 @@ fn default_max_proposal_weight() -> Weight { #[test] fn motions_basic_environment_works() { ExtBuilder::default().build_and_execute(|| { - assert_eq!(Collective::members(), vec![1, 2, 3]); - assert_eq!(*Collective::proposals(), Vec::::new()); + assert_eq!(Members::::get(), vec![1, 2, 3]); + assert_eq!(*Proposals::::get(), Vec::::new()); }); } @@ -205,7 +205,7 @@ fn initialize_members_sorts_members() { ExtBuilder::default() .set_collective_members(unsorted_members) .build_and_execute(|| { - assert_eq!(Collective::members(), expected_members); + assert_eq!(Members::::get(), expected_members); }); } @@ -219,8 +219,8 @@ fn set_members_with_prime_works() { Some(3), MaxMembers::get() )); - assert_eq!(Collective::members(), members.clone()); - assert_eq!(Collective::prime(), Some(3)); + assert_eq!(Members::::get(), members.clone()); + assert_eq!(Prime::::get(), Some(3)); assert_noop!( Collective::set_members(RuntimeOrigin::root(), members, Some(4), MaxMembers::get()), Error::::PrimeAccountNotMember @@ -632,12 +632,12 @@ fn removal_of_old_voters_votes_works() { assert_ok!(Collective::vote(RuntimeOrigin::signed(1), hash, 0, true)); assert_ok!(Collective::vote(RuntimeOrigin::signed(2), hash, 0, true)); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 0, threshold: 3, ayes: vec![1, 2], nays: vec![], end }) ); Collective::change_members_sorted(&[4], &[1], &[2, 3, 4]); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 0, threshold: 3, ayes: vec![2], nays: vec![], end }) ); @@ -653,12 +653,12 @@ fn removal_of_old_voters_votes_works() { assert_ok!(Collective::vote(RuntimeOrigin::signed(2), hash, 1, true)); assert_ok!(Collective::vote(RuntimeOrigin::signed(3), hash, 1, false)); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 1, threshold: 2, ayes: vec![2], nays: vec![3], end }) ); Collective::change_members_sorted(&[], &[3], &[2, 4]); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 1, threshold: 2, ayes: vec![2], nays: vec![], end }) ); }); @@ -680,7 +680,7 @@ fn removal_of_old_voters_votes_works_with_set_members() { assert_ok!(Collective::vote(RuntimeOrigin::signed(1), hash, 0, true)); assert_ok!(Collective::vote(RuntimeOrigin::signed(2), hash, 0, true)); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 0, threshold: 3, ayes: vec![1, 2], nays: vec![], end }) ); assert_ok!(Collective::set_members( @@ -690,7 +690,7 @@ fn removal_of_old_voters_votes_works_with_set_members() { MaxMembers::get() )); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 0, threshold: 3, ayes: vec![2], nays: vec![], end }) ); @@ -706,7 +706,7 @@ fn removal_of_old_voters_votes_works_with_set_members() { assert_ok!(Collective::vote(RuntimeOrigin::signed(2), hash, 1, true)); assert_ok!(Collective::vote(RuntimeOrigin::signed(3), hash, 1, false)); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 1, threshold: 2, ayes: vec![2], nays: vec![3], end }) ); assert_ok!(Collective::set_members( @@ -716,7 +716,7 @@ fn removal_of_old_voters_votes_works_with_set_members() { MaxMembers::get() )); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 1, threshold: 2, ayes: vec![2], nays: vec![], end }) ); }); @@ -735,10 +735,10 @@ fn propose_works() { Box::new(proposal.clone()), proposal_len )); - assert_eq!(*Collective::proposals(), vec![hash]); - assert_eq!(Collective::proposal_of(&hash), Some(proposal)); + assert_eq!(*Proposals::::get(), vec![hash]); + assert_eq!(ProposalOf::::get(&hash), Some(proposal)); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 0, threshold: 3, ayes: vec![], nays: vec![], end }) ); @@ -898,13 +898,13 @@ fn motions_vote_after_works() { )); // Initially there a no votes when the motion is proposed. assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 0, threshold: 2, ayes: vec![], nays: vec![], end }) ); // Cast first aye vote. assert_ok!(Collective::vote(RuntimeOrigin::signed(1), hash, 0, true)); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 0, threshold: 2, ayes: vec![1], nays: vec![], end }) ); // Try to cast a duplicate aye vote. @@ -915,7 +915,7 @@ fn motions_vote_after_works() { // Cast a nay vote. assert_ok!(Collective::vote(RuntimeOrigin::signed(1), hash, 0, false)); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 0, threshold: 2, ayes: vec![], nays: vec![1], end }) ); // Try to cast a duplicate nay vote. @@ -966,7 +966,7 @@ fn motions_all_first_vote_free_works() { proposal_len, )); assert_eq!( - Collective::voting(&hash), + Voting::::get(&hash), Some(Votes { index: 0, threshold: 2, ayes: vec![], nays: vec![], end }) ); @@ -1031,14 +1031,14 @@ fn motions_reproposing_disapproved_works() { proposal_weight, proposal_len )); - assert_eq!(*Collective::proposals(), vec![]); + assert_eq!(*Proposals::::get(), vec![]); assert_ok!(Collective::propose( RuntimeOrigin::signed(1), 2, Box::new(proposal.clone()), proposal_len )); - assert_eq!(*Collective::proposals(), vec![hash]); + assert_eq!(*Proposals::::get(), vec![hash]); }); }