From eef035e3182306a397df306730b534e819408afd Mon Sep 17 00:00:00 2001 From: dharjeezy Date: Sun, 7 Dec 2025 18:11:11 +0100 Subject: [PATCH 1/3] introduce preimage in pallet collective --- substrate/frame/collective/Cargo.toml | 1 + substrate/frame/collective/src/lib.rs | 19 ++++---- substrate/frame/collective/src/tests.rs | 64 +++++++++++++++++++++++-- 3 files changed, 71 insertions(+), 13 deletions(-) diff --git a/substrate/frame/collective/Cargo.toml b/substrate/frame/collective/Cargo.toml index 8e53000352aea..748cfcc417e11 100644 --- a/substrate/frame/collective/Cargo.toml +++ b/substrate/frame/collective/Cargo.toml @@ -29,6 +29,7 @@ sp-runtime = { workspace = true } [dev-dependencies] pallet-balances = { workspace = true, default-features = false } +pallet-preimage = { workspace = true, default-features = false} [features] default = ["std"] diff --git a/substrate/frame/collective/src/lib.rs b/substrate/frame/collective/src/lib.rs index 54144836f1708..719808d107c34 100644 --- a/substrate/frame/collective/src/lib.rs +++ b/substrate/frame/collective/src/lib.rs @@ -60,7 +60,8 @@ use frame_support::{ ensure, traits::{ Backing, ChangeMembers, Consideration, EnsureOrigin, EnsureOriginWithArg, Get, GetBacking, - InitializeMembers, MaybeConsideration, OriginTrait, StorageVersion, + InitializeMembers, MaybeConsideration, OriginTrait, QueryPreimage, StorageVersion, + StorePreimage }, weights::Weight, }; @@ -398,6 +399,8 @@ pub mod pallet { /// consider using a constant cost (e.g., [`crate::deposit::Constant`]) equal to the minimum /// balance under the `runtime-benchmarks` feature. type Consideration: MaybeConsideration; + + type Preimages: QueryPreimage + StorePreimage; } #[pallet::genesis_config] @@ -439,7 +442,7 @@ pub mod pallet { /// Actual proposal for a given hash, if it's current. #[pallet::storage] pub type ProposalOf, I: 'static = ()> = - StorageMap<_, Identity, T::Hash, >::Proposal, OptionQuery>; + StorageMap<_, Identity, T::Hash, (), OptionQuery>; /// Consideration cost created for publishing and storing a proposal. /// @@ -976,7 +979,8 @@ impl, I: 'static> Pallet { let index = ProposalCount::::get(); >::mutate(|i| *i += 1); - >::insert(proposal_hash, proposal); + T::Preimages::note(proposal.encode().into())?; + >::insert(proposal_hash, ()); let votes = { let end = frame_system::Pallet::::block_number() + T::MotionDuration::get(); Votes { index, threshold, ayes: vec![], nays: vec![], end } @@ -1135,12 +1139,11 @@ impl, I: 'static> Pallet { length_bound: u32, weight_bound: Weight, ) -> Result<(>::Proposal, usize), DispatchError> { - let key = ProposalOf::::hashed_key_for(hash); - // read the length of the proposal storage entry directly - let proposal_len = - storage::read(&key, &mut [0; 0], 0).ok_or(Error::::ProposalMissing)?; + let proposal_len = T::Preimages::len(hash).ok_or(Error::::ProposalMissing)?; ensure!(proposal_len <= length_bound, Error::::WrongProposalLength); - let proposal = ProposalOf::::get(hash).ok_or(Error::::ProposalMissing)?; + + let bytes = T::Preimages::fetch(hash, Some(proposal_len)).map_err(|_| Error::::ProposalMissing)?; + let proposal = >::Proposal::decode(&mut &bytes[..]).map_err(|_| Error::::ProposalMissing)?; let proposal_weight = proposal.get_dispatch_info().call_weight; ensure!(proposal_weight.all_lte(weight_bound), Error::::WrongProposalWeight); Ok((proposal, proposal_len as usize)) diff --git a/substrate/frame/collective/src/tests.rs b/substrate/frame/collective/src/tests.rs index baac228254f55..ffbc7972208c9 100644 --- a/substrate/frame/collective/src/tests.rs +++ b/substrate/frame/collective/src/tests.rs @@ -25,7 +25,6 @@ use frame_support::{ fungible::{HoldConsideration, Inspect, Mutate}, ConstU32, ConstU64, StorageVersion, }, - Hashable, }; use frame_system::{EnsureRoot, EventRecord, Phase}; use sp_core::{ConstU128, H256}; @@ -34,6 +33,9 @@ use sp_runtime::{ traits::{BlakeTwo256, Convert, Zero}, BuildStorage, FixedU128, }; +use std::collections::HashMap; +use std::cell::RefCell; +use std::borrow::Cow; pub type Block = sp_runtime::generic::Block; pub type UncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic; @@ -47,6 +49,7 @@ frame_support::construct_runtime!( CollectiveMajority: pallet_collective::, DefaultCollective: pallet_collective, Democracy: mock_democracy, + Preimage: pallet_preimage, } ); @@ -85,6 +88,14 @@ mod mock_democracy { } } +impl pallet_preimage::Config for Test { + type RuntimeEvent = RuntimeEvent; + type WeightInfo = (); + type Currency = Balances; + type ManagerOrigin = EnsureRoot; + type Consideration = (); +} + pub type MaxMembers = ConstU32<100>; type AccountId = u64; @@ -134,6 +145,7 @@ impl Config for Test { type KillOrigin = EnsureRoot; type Consideration = HoldConsideration; + type Preimages = Preimage; } type CollectiveMajorityDeposit = deposit::Linear, ProposalDepositBase>; @@ -154,6 +166,7 @@ impl Config for Test { type KillOrigin = EnsureRoot; type Consideration = HoldConsideration; + type Preimages = Preimage; } impl mock_democracy::Config for Test { type RuntimeEvent = RuntimeEvent; @@ -182,6 +195,7 @@ impl Config for Test { type KillOrigin = EnsureRoot; type Consideration = HoldConsideration; + type Preimages = Preimage; } pub struct ExtBuilder { @@ -227,6 +241,7 @@ impl ExtBuilder { pub fn build_and_execute(self, test: impl FnOnce() -> ()) { self.build().execute_with(|| { + PREIMAGES.with(|m| m.borrow_mut().clear()); test(); Collective::do_try_state().unwrap(); }) @@ -357,6 +372,9 @@ fn close_works() { who: 1, amount: 0, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -504,6 +522,9 @@ fn close_with_prime_works() { who: 1, amount: 0, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -579,6 +600,9 @@ fn close_with_voting_prime_works() { who: 1, amount: 0, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -672,6 +696,9 @@ fn close_with_no_prime_but_majority_works() { who: 5, amount: 2, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::CollectiveMajority(CollectiveEvent::Proposed { account: 5, proposal_index: 0, @@ -847,7 +874,7 @@ fn propose_works() { proposal_len )); assert_eq!(*Proposals::::get(), vec![hash]); - assert_eq!(ProposalOf::::get(&hash), Some(proposal)); + assert_eq!(ProposalOf::::get(&hash), Some(())); assert_eq!( Voting::::get(&hash), Some(Votes { index: 0, threshold: 3, ayes: vec![], nays: vec![], end }) @@ -863,6 +890,9 @@ fn propose_works() { who: 1, amount: 0, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -1058,6 +1088,9 @@ fn motions_vote_after_works() { who: 1, amount: 0, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -1210,6 +1243,9 @@ fn motions_approval_with_enough_votes_and_lower_voting_threshold_works() { who: 1, amount: 0, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, // 0 is the proposal that failed to execute? @@ -1272,6 +1308,9 @@ fn motions_approval_with_enough_votes_and_lower_voting_threshold_works() { who: 1, amount: 0, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 1, @@ -1350,6 +1389,9 @@ fn motions_disapproval_works() { who: 1, amount: 0, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -1416,6 +1458,9 @@ fn motions_approval_works() { who: 1, amount: 0, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -1497,15 +1542,21 @@ fn motion_with_no_votes_closes_with_disapproval() { // Events show that the close ended in a disapproval. assert_eq!( System::events()[1], + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), + ); + assert_eq!( + System::events()[2], record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, proposal_hash: hash, threshold: 3 - })) + })), ); assert_eq!( - System::events()[2], + System::events()[3], record(RuntimeEvent::Collective(CollectiveEvent::Closed { proposal_hash: hash, yes: 0, @@ -1513,7 +1564,7 @@ fn motion_with_no_votes_closes_with_disapproval() { })) ); assert_eq!( - System::events()[3], + System::events()[4], record(RuntimeEvent::Collective(CollectiveEvent::Disapproved { proposal_hash: hash })) ); }) @@ -1581,6 +1632,9 @@ fn disapprove_proposal_works() { who: 1, amount: 0, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { + hash, + })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, From f304a89187906ad2072add9f2fd99073b0b31301 Mon Sep 17 00:00:00 2001 From: dharjeezy Date: Sun, 7 Dec 2025 18:15:40 +0100 Subject: [PATCH 2/3] fmt --- substrate/frame/collective/src/lib.rs | 8 ++-- substrate/frame/collective/src/tests.rs | 52 +++++++------------------ 2 files changed, 18 insertions(+), 42 deletions(-) diff --git a/substrate/frame/collective/src/lib.rs b/substrate/frame/collective/src/lib.rs index 719808d107c34..cdbba5b240185 100644 --- a/substrate/frame/collective/src/lib.rs +++ b/substrate/frame/collective/src/lib.rs @@ -61,7 +61,7 @@ use frame_support::{ traits::{ Backing, ChangeMembers, Consideration, EnsureOrigin, EnsureOriginWithArg, Get, GetBacking, InitializeMembers, MaybeConsideration, OriginTrait, QueryPreimage, StorageVersion, - StorePreimage + StorePreimage, }, weights::Weight, }; @@ -1142,8 +1142,10 @@ impl, I: 'static> Pallet { let proposal_len = T::Preimages::len(hash).ok_or(Error::::ProposalMissing)?; ensure!(proposal_len <= length_bound, Error::::WrongProposalLength); - let bytes = T::Preimages::fetch(hash, Some(proposal_len)).map_err(|_| Error::::ProposalMissing)?; - let proposal = >::Proposal::decode(&mut &bytes[..]).map_err(|_| Error::::ProposalMissing)?; + let bytes = T::Preimages::fetch(hash, Some(proposal_len)) + .map_err(|_| Error::::ProposalMissing)?; + let proposal = >::Proposal::decode(&mut &bytes[..]) + .map_err(|_| Error::::ProposalMissing)?; let proposal_weight = proposal.get_dispatch_info().call_weight; ensure!(proposal_weight.all_lte(weight_bound), Error::::WrongProposalWeight); Ok((proposal, proposal_len as usize)) diff --git a/substrate/frame/collective/src/tests.rs b/substrate/frame/collective/src/tests.rs index ffbc7972208c9..f5ec9ef8a65dc 100644 --- a/substrate/frame/collective/src/tests.rs +++ b/substrate/frame/collective/src/tests.rs @@ -33,9 +33,7 @@ use sp_runtime::{ traits::{BlakeTwo256, Convert, Zero}, BuildStorage, FixedU128, }; -use std::collections::HashMap; -use std::cell::RefCell; -use std::borrow::Cow; +use std::{borrow::Cow, cell::RefCell, collections::HashMap}; pub type Block = sp_runtime::generic::Block; pub type UncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic; @@ -372,9 +370,7 @@ fn close_works() { who: 1, amount: 0, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -522,9 +518,7 @@ fn close_with_prime_works() { who: 1, amount: 0, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -600,9 +594,7 @@ fn close_with_voting_prime_works() { who: 1, amount: 0, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -696,9 +688,7 @@ fn close_with_no_prime_but_majority_works() { who: 5, amount: 2, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::CollectiveMajority(CollectiveEvent::Proposed { account: 5, proposal_index: 0, @@ -890,9 +880,7 @@ fn propose_works() { who: 1, amount: 0, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -1088,9 +1076,7 @@ fn motions_vote_after_works() { who: 1, amount: 0, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -1243,9 +1229,7 @@ fn motions_approval_with_enough_votes_and_lower_voting_threshold_works() { who: 1, amount: 0, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, // 0 is the proposal that failed to execute? @@ -1308,9 +1292,7 @@ fn motions_approval_with_enough_votes_and_lower_voting_threshold_works() { who: 1, amount: 0, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 1, @@ -1389,9 +1371,7 @@ fn motions_disapproval_works() { who: 1, amount: 0, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -1458,9 +1438,7 @@ fn motions_approval_works() { who: 1, amount: 0, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, @@ -1542,9 +1520,7 @@ fn motion_with_no_votes_closes_with_disapproval() { // Events show that the close ended in a disapproval. assert_eq!( System::events()[1], - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), ); assert_eq!( System::events()[2], @@ -1632,9 +1608,7 @@ fn disapprove_proposal_works() { who: 1, amount: 0, })), - record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { - hash, - })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Noted { hash })), record(RuntimeEvent::Collective(CollectiveEvent::Proposed { account: 1, proposal_index: 0, From 3de175a0c4b35a916c8862147c4d2360413be6c7 Mon Sep 17 00:00:00 2001 From: dharjeezy Date: Wed, 17 Dec 2025 07:14:09 +0100 Subject: [PATCH 3/3] multi block migrations --- Cargo.lock | 2 + substrate/frame/collective/Cargo.toml | 2 + substrate/frame/collective/src/lib.rs | 33 ++++---- .../frame/collective/src/migrations/mod.rs | 1 + .../frame/collective/src/migrations/v5.rs | 77 +++++++++++++++++++ substrate/frame/collective/src/tests.rs | 76 ++++++++++++++++-- 6 files changed, 168 insertions(+), 23 deletions(-) create mode 100644 substrate/frame/collective/src/migrations/v5.rs diff --git a/Cargo.lock b/Cargo.lock index d129f4eea452c..a349220c940e6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -12034,6 +12034,8 @@ dependencies = [ "frame-system", "log", "pallet-balances", + "pallet-migrations", + "pallet-preimage", "parity-scale-codec", "scale-info", "sp-core 28.0.0", diff --git a/substrate/frame/collective/Cargo.toml b/substrate/frame/collective/Cargo.toml index 748cfcc417e11..877d8927a2cf2 100644 --- a/substrate/frame/collective/Cargo.toml +++ b/substrate/frame/collective/Cargo.toml @@ -26,6 +26,7 @@ scale-info = { features = ["derive"], workspace = true } sp-core = { workspace = true } sp-io = { workspace = true } sp-runtime = { workspace = true } +pallet-migrations = { workspace = true } [dev-dependencies] pallet-balances = { workspace = true, default-features = false } @@ -44,6 +45,7 @@ std = [ "sp-core/std", "sp-io/std", "sp-runtime/std", + "pallet-migrations/std" ] runtime-benchmarks = [ "frame-benchmarking/runtime-benchmarks", diff --git a/substrate/frame/collective/src/lib.rs b/substrate/frame/collective/src/lib.rs index cdbba5b240185..5b067597a47ea 100644 --- a/substrate/frame/collective/src/lib.rs +++ b/substrate/frame/collective/src/lib.rs @@ -439,11 +439,6 @@ pub mod pallet { pub type Proposals, I: 'static = ()> = StorageValue<_, BoundedVec, ValueQuery>; - /// Actual proposal for a given hash, if it's current. - #[pallet::storage] - pub type ProposalOf, I: 'static = ()> = - StorageMap<_, Identity, T::Hash, (), OptionQuery>; - /// Consideration cost created for publishing and storing a proposal. /// /// Determined by [Config::Consideration] and may be not present for certain proposals (e.g. if @@ -857,7 +852,7 @@ pub mod pallet { pub fn kill(origin: OriginFor, proposal_hash: T::Hash) -> DispatchResultWithPostInfo { T::KillOrigin::ensure_origin(origin)?; ensure!( - ProposalOf::::get(&proposal_hash).is_some(), + Voting::::contains_key(&proposal_hash), Error::::ProposalMissing ); let burned = if let Some((who, cost)) = >::take(proposal_hash) { @@ -891,7 +886,7 @@ pub mod pallet { ) -> DispatchResult { ensure_signed_or_root(origin)?; ensure!( - ProposalOf::::get(&proposal_hash).is_none(), + !Voting::::contains_key(&proposal_hash), Error::::ProposalActive ); if let Some((who, cost)) = >::take(proposal_hash) { @@ -936,7 +931,7 @@ impl, I: 'static> Pallet { ); let proposal_hash = T::Hashing::hash_of(&proposal); - ensure!(!>::contains_key(proposal_hash), Error::::DuplicateProposal); + ensure!(!Voting::::contains_key(proposal_hash), Error::::DuplicateProposal); let seats = Members::::get().len() as MemberCount; let result = proposal.dispatch(RawOrigin::Members(1, seats).into()); @@ -963,7 +958,7 @@ impl, I: 'static> Pallet { ); let proposal_hash = T::Hashing::hash_of(&proposal); - ensure!(!>::contains_key(proposal_hash), Error::::DuplicateProposal); + ensure!(!Voting::::contains_key(proposal_hash), Error::::DuplicateProposal); let active_proposals = >::try_mutate(|proposals| -> Result { @@ -980,7 +975,11 @@ impl, I: 'static> Pallet { >::mutate(|i| *i += 1); T::Preimages::note(proposal.encode().into())?; - >::insert(proposal_hash, ()); + let votes = { + let end = frame_system::Pallet::::block_number() + T::MotionDuration::get(); + Votes { index, threshold, ayes: vec![], nays: vec![], end } + }; + >::insert(proposal_hash, votes); let votes = { let end = frame_system::Pallet::::block_number() + T::MotionDuration::get(); Votes { index, threshold, ayes: vec![], nays: vec![], end } @@ -1197,7 +1196,7 @@ impl, I: 'static> Pallet { // Removes a proposal from the pallet, cleaning up votes and the vector of proposals. fn remove_proposal(proposal_hash: T::Hash) -> u32 { // remove proposal and vote - ProposalOf::::remove(&proposal_hash); + T::Preimages::unnote(&proposal_hash); Voting::::remove(&proposal_hash); let num_proposals = Proposals::::mutate(|proposals| { proposals.retain(|h| h != &proposal_hash); @@ -1215,12 +1214,12 @@ impl, I: 'static> Pallet { /// 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. + /// call mapped to it inside the `Voting` 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` + /// * Count of `Voting` should match the count of `Proposals` /// /// Looking at votes: /// * The sum of aye and nay votes for a proposal can never exceed @@ -1239,8 +1238,8 @@ impl, I: 'static> Pallet { Proposals::::get().into_iter().try_for_each( |proposal| -> Result<(), TryRuntimeError> { ensure!( - ProposalOf::::get(proposal).is_some(), - "Proposal hash from `Proposals` is not found inside the `ProposalOf` mapping." + Voting::::contains_key(proposal), + "Proposal hash from `Proposals` is not found inside the `Voting` mapping." ); Ok(()) }, @@ -1251,8 +1250,8 @@ impl, I: 'static> Pallet { "The actual number of proposals is greater than `ProposalCount`" ); ensure!( - Proposals::::get().into_iter().count() == >::iter_keys().count(), - "Proposal count inside `Proposals` is not equal to the proposal count in `ProposalOf`" + Proposals::::get().into_iter().count() == >::iter_keys().count(), + "Proposal count inside `Proposals` is not equal to the proposal count in `Voting`" ); Proposals::::get().into_iter().try_for_each( diff --git a/substrate/frame/collective/src/migrations/mod.rs b/substrate/frame/collective/src/migrations/mod.rs index 2487ed1d5da52..42e417fc385e6 100644 --- a/substrate/frame/collective/src/migrations/mod.rs +++ b/substrate/frame/collective/src/migrations/mod.rs @@ -17,3 +17,4 @@ /// Version 4. pub mod v4; +pub mod v5; diff --git a/substrate/frame/collective/src/migrations/v5.rs b/substrate/frame/collective/src/migrations/v5.rs new file mode 100644 index 0000000000000..9b386af47960e --- /dev/null +++ b/substrate/frame/collective/src/migrations/v5.rs @@ -0,0 +1,77 @@ +use super::super::*; +use codec::{Decode, Encode, MaxEncodedLen}; +use frame_support::{ + migrations::{SteppedMigration, SteppedMigrationError}, + pallet_prelude::*, + traits::{StorePreimage}, + weights::WeightMeter, +}; +use scale_info::TypeInfo; + +#[frame_support::storage_alias] +pub type OldProposalOf, I: 'static> = +StorageMap, Identity, ::Hash, >::Proposal>; + +#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, Clone, Copy, PartialEq, Eq, RuntimeDebug)] +pub enum Cursor { + MigrateProposals(u32), + ClearStorage, +} + +pub struct MigrateToV5(PhantomData<(T, I)>); + +impl, I: 'static> SteppedMigration for MigrateToV5 { + type Cursor = Cursor; + type Identifier = [u8; 32]; + + fn id() -> Self::Identifier { + *b"CollectiveMigrationV5___________" + } + + fn step( + cursor: Option, + meter: &mut WeightMeter, + ) -> Result, SteppedMigrationError> { + let cursor = cursor.unwrap_or(Cursor::MigrateProposals(0)); + + match cursor { + Cursor::MigrateProposals(index) => { + let proposals = Proposals::::get(); + let count = proposals.len() as u32; + + let mut current_index = index; + + let weight_per_item = T::DbWeight::get().reads_writes(2, 2); + + while current_index < count { + if meter.try_consume(weight_per_item).is_err() { + return Ok(Some(Cursor::MigrateProposals(current_index))); + } + + let hash = proposals[current_index as usize]; + if let Some(proposal) = OldProposalOf::::get(hash) { + let _ = T::Preimages::note(proposal.encode().into()); + OldProposalOf::::remove(hash); + } + + current_index += 1; + } + + Ok(Some(Cursor::ClearStorage)) + }, + Cursor::ClearStorage => { + let limit = 100u32; + let result = OldProposalOf::::clear(limit, None); + + let weight = T::DbWeight::get().reads_writes(1, 1).saturating_mul(result.loops as u64); + meter.consume(weight); + + if result.maybe_cursor.is_some() { + Ok(Some(Cursor::ClearStorage)) + } else { + Ok(None) + } + } + } + } +} \ No newline at end of file diff --git a/substrate/frame/collective/src/tests.rs b/substrate/frame/collective/src/tests.rs index f5ec9ef8a65dc..2313c0acf2f85 100644 --- a/substrate/frame/collective/src/tests.rs +++ b/substrate/frame/collective/src/tests.rs @@ -19,12 +19,14 @@ use super::{Event as CollectiveEvent, *}; use crate as pallet_collective; use frame_support::{ assert_noop, assert_ok, derive_impl, - dispatch::Pays, + BoundedVec, dispatch::Pays, + migrations::SteppedMigration, parameter_types, traits::{ fungible::{HoldConsideration, Inspect, Mutate}, ConstU32, ConstU64, StorageVersion, }, + weights::WeightMeter, }; use frame_system::{EnsureRoot, EventRecord, Phase}; use sp_core::{ConstU128, H256}; @@ -34,6 +36,8 @@ use sp_runtime::{ BuildStorage, FixedU128, }; use std::{borrow::Cow, cell::RefCell, collections::HashMap}; +use frame_support::Hashable; + pub type Block = sp_runtime::generic::Block; pub type UncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic; @@ -239,7 +243,6 @@ impl ExtBuilder { pub fn build_and_execute(self, test: impl FnOnce() -> ()) { self.build().execute_with(|| { - PREIMAGES.with(|m| m.borrow_mut().clear()); test(); Collective::do_try_state().unwrap(); }) @@ -398,7 +401,8 @@ fn close_works() { })), record(RuntimeEvent::Collective(CollectiveEvent::Disapproved { proposal_hash: hash - })) + })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Cleared { hash })), ] ); }); @@ -546,7 +550,8 @@ fn close_with_prime_works() { })), record(RuntimeEvent::Collective(CollectiveEvent::Disapproved { proposal_hash: hash - })) + })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Cleared { hash })), ] ); }); @@ -624,7 +629,8 @@ fn close_with_voting_prime_works() { record(RuntimeEvent::Collective(CollectiveEvent::Executed { proposal_hash: hash, result: Err(DispatchError::BadOrigin) - })) + })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Cleared { hash })), ] ); }); @@ -728,6 +734,7 @@ fn close_with_no_prime_but_majority_works() { proposal_hash: hash, result: Err(DispatchError::BadOrigin) })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Cleared { hash })), record(RuntimeEvent::Balances(pallet_balances::Event::Released { reason: ::RuntimeHoldReason::Collective( HoldReason::ProposalSubmission, @@ -864,7 +871,6 @@ fn propose_works() { proposal_len )); assert_eq!(*Proposals::::get(), vec![hash]); - assert_eq!(ProposalOf::::get(&hash), Some(())); assert_eq!( Voting::::get(&hash), Some(Votes { index: 0, threshold: 3, ayes: vec![], nays: vec![], end }) @@ -1260,6 +1266,7 @@ fn motions_approval_with_enough_votes_and_lower_voting_threshold_works() { proposal_hash: hash, result: Err(DispatchError::BadOrigin) })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Cleared { hash })), ] ); @@ -1333,6 +1340,7 @@ fn motions_approval_with_enough_votes_and_lower_voting_threshold_works() { proposal_hash: hash, result: Ok(()) })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Cleared { hash })), ] ); }); @@ -1400,6 +1408,7 @@ fn motions_disapproval_works() { record(RuntimeEvent::Collective(CollectiveEvent::Disapproved { proposal_hash: hash })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Cleared { hash })), ] ); }); @@ -1469,6 +1478,7 @@ fn motions_approval_works() { proposal_hash: hash, result: Err(DispatchError::BadOrigin) })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Cleared { hash })), ] ); }); @@ -1632,6 +1642,7 @@ fn disapprove_proposal_works() { record(RuntimeEvent::Collective(CollectiveEvent::Disapproved { proposal_hash: hash })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Cleared { hash })), ] ); }) @@ -1756,6 +1767,7 @@ fn kill_proposal_with_deposit() { proposal_hash: last_hash.unwrap(), who: 1, })), + record(RuntimeEvent::Preimage(pallet_preimage::Event::Cleared { hash: last_hash.unwrap() })), record(RuntimeEvent::Collective(CollectiveEvent::Killed { proposal_hash: last_hash.unwrap(), })), @@ -1890,3 +1902,55 @@ fn constant_deposit_work() { assert_eq!(>::convert(1), 12); assert_eq!(>::convert(2), 12); } + +#[test] +fn test_proposal_migration_works() { + + ExtBuilder::default().build_and_execute(|| { + let proposal = make_proposal(100); + let proposal_hash = BlakeTwo256::hash_of(&proposal); + + crate::migrations::v5::OldProposalOf::::insert(proposal_hash, proposal.clone()); + + let proposals: BoundedVec<_, MaxProposals> = vec![proposal_hash].try_into().unwrap(); + Proposals::::put(proposals); + ProposalCount::::put(1u32); + + let votes = Votes { + index: 0, + threshold: 1, + ayes: vec![], + nays: vec![], + end: 100, + }; + Voting::::insert(proposal_hash, votes); + + StorageVersion::new(0).put::(); + + let mut cursor = None; + let mut meter = WeightMeter::with_limit(Weight::MAX); + + loop { + let result = crate::migrations::v5::MigrateToV5::::step( + cursor, + &mut meter, + ); + + match result { + Ok(maybe_next) => { + if maybe_next.is_none() { + break; + } + cursor = maybe_next; + }, + Err(e) => { + panic!("Migration failed: {:?}", e); + } + } + } + + assert!(pallet_preimage::RequestStatusFor::::contains_key(proposal_hash)); + + assert!(crate::migrations::v5::OldProposalOf::::get(proposal_hash).is_none()); + }); +}