diff --git a/frame/collective/src/benchmarking.rs b/frame/collective/src/benchmarking.rs index d4b5af1bc1937..bcd203c3894a3 100644 --- a/frame/collective/src/benchmarking.rs +++ b/frame/collective/src/benchmarking.rs @@ -646,5 +646,5 @@ benchmarks_instance_pallet! { assert_last_event::(Event::Disapproved { proposal_hash: last_hash }.into()); } - impl_benchmark_test_suite!(Collective, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(Collective, crate::tests::ExtBuilder::default().build(), crate::tests::Test); } diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index bde08f08c13b4..0cbf7e97261df 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -50,8 +50,8 @@ use sp_std::{marker::PhantomData, prelude::*, result}; use frame_support::{ codec::{Decode, Encode, MaxEncodedLen}, dispatch::{ - DispatchError, DispatchResultWithPostInfo, Dispatchable, GetDispatchInfo, Pays, - PostDispatchInfo, + DispatchError, DispatchResult, DispatchResultWithPostInfo, Dispatchable, GetDispatchInfo, + Pays, PostDispatchInfo, }, ensure, traits::{ @@ -341,6 +341,15 @@ pub mod pallet { WrongProposalLength, } + #[pallet::hooks] + impl, I: 'static> Hooks> for Pallet { + #[cfg(feature = "try-runtime")] + fn try_state(_n: BlockNumberFor) -> Result<(), &'static str> { + Self::do_try_state()?; + Ok(()) + } + } + // Note that councillor operations are assigned to the operational class. #[pallet::call] impl, I: 'static> Pallet { @@ -916,6 +925,111 @@ impl, I: 'static> Pallet { }); num_proposals as u32 } + + /// Ensure the correctness of the state of this pallet. + /// + /// The following expectation must always apply. + /// + /// ## Expectations: + /// + /// Looking at proposals: + /// + /// * Each hash of a proposal that is stored inside `Proposals` must have a + /// call mapped to it inside the `ProposalOf` storage map. + /// * `ProposalCount` must always be more or equal to the number of + /// proposals inside the `Proposals` storage value. The reason why + /// `ProposalCount` can be more is because when a proposal is removed the + /// count is not deducted. + /// * Count of `ProposalOf` should match the count of `Proposals` + /// + /// Looking at votes: + /// * The sum of aye and nay votes for a proposal can never exceed + /// `MaxMembers`. + /// * The proposal index inside the `Voting` storage map must be unique. + /// * All proposal hashes inside `Voting` must exist in `Proposals`. + /// + /// Looking at members: + /// * The members count must never exceed `MaxMembers`. + /// * All the members must be sorted by value. + /// + /// Looking at prime account: + /// * The prime account must be a member of the collective. + #[cfg(any(feature = "try-runtime", test))] + fn do_try_state() -> DispatchResult { + Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult { + ensure!( + Self::proposal_of(proposal).is_some(), + DispatchError::Other( + "Proposal hash from `Proposals` is not found inside the `ProposalOf` mapping." + ) + ); + Ok(()) + })?; + + ensure!( + Self::proposals().into_iter().count() <= Self::proposal_count() as usize, + DispatchError::Other("The actual number of proposals is greater than `ProposalCount`") + ); + ensure!( + Self::proposals().into_iter().count() == >::iter_keys().count(), + DispatchError::Other("Proposal count inside `Proposals` is not equal to the proposal count in `ProposalOf`") + ); + + Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult { + if let Some(votes) = Self::voting(proposal) { + let ayes = votes.ayes.len(); + let nays = votes.nays.len(); + + ensure!( + ayes.saturating_add(nays) <= T::MaxMembers::get() as usize, + DispatchError::Other("The sum of ayes and nays is greater than `MaxMembers`") + ); + } + Ok(()) + })?; + + let mut proposal_indices = vec![]; + Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult { + if let Some(votes) = Self::voting(proposal) { + let proposal_index = votes.index; + ensure!( + !proposal_indices.contains(&proposal_index), + DispatchError::Other("The proposal index is not unique.") + ); + proposal_indices.push(proposal_index); + } + Ok(()) + })?; + + >::iter_keys().try_for_each(|proposal_hash| -> DispatchResult { + ensure!( + Self::proposals().contains(&proposal_hash), + DispatchError::Other( + "`Proposals` doesn't contain the proposal hash from the `Voting` storage map." + ) + ); + Ok(()) + })?; + + ensure!( + Self::members().len() <= T::MaxMembers::get() as usize, + DispatchError::Other("The member count is greater than `MaxMembers`.") + ); + + ensure!( + Self::members().windows(2).all(|members| members[0] <= members[1]), + DispatchError::Other("The members are not sorted by value.") + ); + + if let Some(prime) = Self::prime() { + ensure!( + Self::members().contains(&prime), + DispatchError::Other("Prime account is not a member.") + ); + } + + Ok(()) + } } impl, I: 'static> ChangeMembers for Pallet { diff --git a/frame/collective/src/tests.rs b/frame/collective/src/tests.rs index 2550ab3ed2017..79dc3becfffc6 100644 --- a/frame/collective/src/tests.rs +++ b/frame/collective/src/tests.rs @@ -155,23 +155,40 @@ impl Config for Test { type SetMembersOrigin = EnsureRoot; } -pub fn new_test_ext() -> sp_io::TestExternalities { - let mut ext: sp_io::TestExternalities = GenesisConfig { - collective: pallet_collective::GenesisConfig { - members: vec![1, 2, 3], - phantom: Default::default(), - }, - collective_majority: pallet_collective::GenesisConfig { - members: vec![1, 2, 3, 4, 5], - phantom: Default::default(), - }, - default_collective: Default::default(), +pub struct ExtBuilder {} + +impl Default for ExtBuilder { + fn default() -> Self { + Self {} + } +} + +impl ExtBuilder { + pub fn build(self) -> sp_io::TestExternalities { + let mut ext: sp_io::TestExternalities = GenesisConfig { + collective: pallet_collective::GenesisConfig { + members: vec![1, 2, 3], + phantom: Default::default(), + }, + collective_majority: pallet_collective::GenesisConfig { + members: vec![1, 2, 3, 4, 5], + phantom: Default::default(), + }, + default_collective: Default::default(), + } + .build_storage() + .unwrap() + .into(); + ext.execute_with(|| System::set_block_number(1)); + ext + } + + pub fn build_and_execute(self, test: impl FnOnce() -> ()) { + self.build().execute_with(|| { + test(); + Collective::do_try_state().unwrap(); + }) } - .build_storage() - .unwrap() - .into(); - ext.execute_with(|| System::set_block_number(1)); - ext } fn make_proposal(value: u64) -> RuntimeCall { @@ -186,7 +203,7 @@ fn record(event: RuntimeEvent) -> EventRecord { #[test] fn motions_basic_environment_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_eq!(Collective::members(), vec![1, 2, 3]); assert_eq!(*Collective::proposals(), Vec::::new()); }); @@ -194,7 +211,7 @@ fn motions_basic_environment_works() { #[test] fn close_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let proposal_weight = proposal.get_dispatch_info().weight; @@ -262,7 +279,7 @@ fn close_works() { #[test] fn proposal_weight_limit_works_on_approve() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = RuntimeCall::Collective(crate::Call::set_members { new_members: vec![1, 2, 3], prime: None, @@ -304,7 +321,7 @@ fn proposal_weight_limit_works_on_approve() { #[test] fn proposal_weight_limit_ignored_on_disapprove() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = RuntimeCall::Collective(crate::Call::set_members { new_members: vec![1, 2, 3], prime: None, @@ -334,7 +351,7 @@ fn proposal_weight_limit_ignored_on_disapprove() { #[test] fn close_with_prime_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let proposal_weight = proposal.get_dispatch_info().weight; @@ -402,7 +419,7 @@ fn close_with_prime_works() { #[test] fn close_with_voting_prime_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let proposal_weight = proposal.get_dispatch_info().weight; @@ -472,7 +489,7 @@ fn close_with_voting_prime_works() { #[test] fn close_with_no_prime_but_majority_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let proposal_weight = proposal.get_dispatch_info().weight; @@ -552,7 +569,7 @@ fn close_with_no_prime_but_majority_works() { #[test] fn removal_of_old_voters_votes_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let hash = BlakeTwo256::hash_of(&proposal); @@ -600,7 +617,7 @@ fn removal_of_old_voters_votes_works() { #[test] fn removal_of_old_voters_votes_works_with_set_members() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let hash = BlakeTwo256::hash_of(&proposal); @@ -658,7 +675,7 @@ fn removal_of_old_voters_votes_works_with_set_members() { #[test] fn propose_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let hash = proposal.blake2_256().into(); @@ -690,7 +707,7 @@ fn propose_works() { #[test] fn limit_active_proposals() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { for i in 0..MaxProposals::get() { let proposal = make_proposal(i as u64); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); @@ -717,7 +734,7 @@ fn limit_active_proposals() { #[test] fn correct_validate_and_get_proposal() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = RuntimeCall::Collective(crate::Call::set_members { new_members: vec![1, 2, 3], prime: None, @@ -763,7 +780,7 @@ fn correct_validate_and_get_proposal() { #[test] fn motions_ignoring_non_collective_proposals_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); assert_noop!( @@ -780,7 +797,7 @@ fn motions_ignoring_non_collective_proposals_works() { #[test] fn motions_ignoring_non_collective_votes_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let hash: H256 = proposal.blake2_256().into(); @@ -799,7 +816,7 @@ fn motions_ignoring_non_collective_votes_works() { #[test] fn motions_ignoring_bad_index_collective_vote_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { System::set_block_number(3); let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); @@ -819,7 +836,7 @@ fn motions_ignoring_bad_index_collective_vote_works() { #[test] fn motions_vote_after_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let hash: H256 = proposal.blake2_256().into(); @@ -888,7 +905,7 @@ fn motions_vote_after_works() { #[test] fn motions_all_first_vote_free_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let hash: H256 = proposal.blake2_256().into(); @@ -946,7 +963,7 @@ fn motions_all_first_vote_free_works() { #[test] fn motions_reproposing_disapproved_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let proposal_weight = proposal.get_dispatch_info().weight; @@ -978,7 +995,7 @@ fn motions_reproposing_disapproved_works() { #[test] fn motions_approval_with_enough_votes_and_lower_voting_threshold_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = RuntimeCall::Democracy(mock_democracy::Call::external_propose_majority {}); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let proposal_weight = proposal.get_dispatch_info().weight; @@ -1108,7 +1125,7 @@ fn motions_approval_with_enough_votes_and_lower_voting_threshold_works() { #[test] fn motions_disapproval_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let proposal_weight = proposal.get_dispatch_info().weight; @@ -1167,7 +1184,7 @@ fn motions_disapproval_works() { #[test] fn motions_approval_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let proposal_weight = proposal.get_dispatch_info().weight; @@ -1228,7 +1245,7 @@ fn motions_approval_works() { #[test] fn motion_with_no_votes_closes_with_disapproval() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let proposal_weight = proposal.get_dispatch_info().weight; @@ -1289,7 +1306,7 @@ fn close_disapprove_does_not_care_about_weight_or_len() { // This test confirms that if you close a proposal that would be disapproved, // we do not care about the proposal length or proposal weight since it will // not be read from storage or executed. - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let hash: H256 = proposal.blake2_256().into(); @@ -1321,7 +1338,7 @@ fn close_disapprove_does_not_care_about_weight_or_len() { #[test] fn disapprove_proposal_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let proposal = make_proposal(42); let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32); let hash: H256 = proposal.blake2_256().into(); @@ -1380,7 +1397,7 @@ fn genesis_build_panics_with_duplicate_members() { #[test] fn migration_v4() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { use frame_support::traits::PalletInfoAccess; let old_pallet = "OldCollective";