From 32212bcb18089e6435ab02932d57e0629d53c919 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Tue, 18 Apr 2023 20:42:07 +0200 Subject: [PATCH 01/25] Try-state for Referenda pallet --- frame/referenda/src/lib.rs | 49 ++++++++++++++++++++++++++++++++ frame/referenda/src/migration.rs | 16 ++++++++++- frame/referenda/src/mock.rs | 37 +++++++++++++++--------- frame/referenda/src/tests.rs | 48 +++++++++++++++---------------- 4 files changed, 111 insertions(+), 39 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 68837376c5b33..4cdfa223be6c7 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -66,6 +66,7 @@ use codec::{Codec, Encode}; use frame_support::{ + dispatch::DispatchResult, ensure, traits::{ schedule::{ @@ -411,6 +412,15 @@ pub mod pallet { PreimageNotExist, } + #[pallet::hooks] + impl, I: 'static> Hooks for Pallet { + #[cfg(feature = "try-runtime")] + fn try_state(_n: T::BlockNumber) -> Result<(), &'static str> { + Self::do_try_state()?; + Ok(()) + } + } + #[pallet::call] impl, I: 'static> Pallet { /// Propose a referendum on a privileged action. @@ -1270,4 +1280,43 @@ impl, I: 'static> Pallet { Self::deposit_event(Event::::MetadataCleared { index, hash }); } } + + /// Ensure the correctness of the state of this pallet. + /// + /// The following assertions must always apply. + /// + /// Looking at referenda: + /// + /// * `ReferendumCount` must always be equal to the number of referenda in `ReferendumInfoFor`. + /// + /// Looking at referenda info: + /// + /// * The submission deposit cannot be less than `T::SubmissionDeposit`. + #[cfg(any(feature = "try-runtime", test))] + fn do_try_state() -> DispatchResult { + ensure!( + ReferendumCount::::get() as usize == + ReferendumInfoFor::::iter_keys().count(), + DispatchError::Other( + "Number of referenda in `ReferendumInfoFor` is greater than `ReferendumCount`" + ) + ); + + ReferendumInfoFor::::iter().try_for_each( + |(_, referendum)| -> Result<(), DispatchError> { + match referendum { + ReferendumInfo::Ongoing(status) => { + ensure!( + status.submission_deposit.amount >= T::SubmissionDeposit::get(), + DispatchError::Other("Incorrect submission deposit.") + ); + }, + _ => {}, + } + Ok(()) + }, + )?; + + Ok(()) + } } diff --git a/frame/referenda/src/migration.rs b/frame/referenda/src/migration.rs index c27ab452ac637..9cfa5b3a65f78 100644 --- a/frame/referenda/src/migration.rs +++ b/frame/referenda/src/migration.rs @@ -82,6 +82,10 @@ pub mod v0 { #[storage_alias] pub type ReferendumInfoFor, I: 'static> = StorageMap, Blake2_128Concat, ReferendumIndex, ReferendumInfoOf>; + + #[storage_alias] + pub type ReferendumCount, I: 'static> = + StorageValue, ReferendumIndex, ValueQuery>; } pub mod v1 { @@ -188,6 +192,14 @@ pub mod test { } } + fn increment_referendum_index() { + ReferendumCount::::mutate(|x| { + let r = *x; + *x += 1; + r + }); + } + #[test] pub fn referendum_status_v0() { // make sure the bytes of the encoded referendum v0 is decodable. @@ -199,10 +211,11 @@ pub mod test { #[test] fn migration_v0_to_v1_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { // create and insert into the storage an ongoing referendum v0. let status_v0 = create_status_v0(); let ongoing_v0 = v0::ReferendumInfoOf::::Ongoing(status_v0.clone()); + increment_referendum_index(); v0::ReferendumInfoFor::::insert(2, ongoing_v0); // create and insert into the storage an approved referendum v0. let approved_v0 = v0::ReferendumInfoOf::::Approved( @@ -210,6 +223,7 @@ pub mod test { Deposit { who: 1, amount: 10 }, Some(Deposit { who: 2, amount: 20 }), ); + increment_referendum_index(); v0::ReferendumInfoFor::::insert(5, approved_v0); // run migration from v0 to v1. v1::MigrateV0ToV1::::on_runtime_upgrade(); diff --git a/frame/referenda/src/mock.rs b/frame/referenda/src/mock.rs index cdedb79556f35..7e2f3d357f21d 100644 --- a/frame/referenda/src/mock.rs +++ b/frame/referenda/src/mock.rs @@ -231,23 +231,32 @@ impl Config for Test { type Tracks = TestTracksInfo; type Preimages = Preimage; } +pub struct ExtBuilder {} -pub fn new_test_ext() -> sp_io::TestExternalities { - let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); - let balances = vec![(1, 100), (2, 100), (3, 100), (4, 100), (5, 100), (6, 100)]; - pallet_balances::GenesisConfig:: { balances } - .assimilate_storage(&mut t) - .unwrap(); - let mut ext = sp_io::TestExternalities::new(t); - ext.execute_with(|| System::set_block_number(1)); - ext +impl Default for ExtBuilder { + fn default() -> Self { + Self {} + } } -/// Execute the function two times, with `true` and with `false`. -#[allow(dead_code)] -pub fn new_test_ext_execute_with_cond(execute: impl FnOnce(bool) -> () + Clone) { - new_test_ext().execute_with(|| (execute.clone())(false)); - new_test_ext().execute_with(|| execute(true)); +impl ExtBuilder { + pub fn build(self) -> sp_io::TestExternalities { + let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); + let balances = vec![(1, 100), (2, 100), (3, 100), (4, 100), (5, 100), (6, 100)]; + pallet_balances::GenesisConfig:: { balances } + .assimilate_storage(&mut t) + .unwrap(); + let mut ext = sp_io::TestExternalities::new(t); + ext.execute_with(|| System::set_block_number(1)); + ext + } + + pub fn build_and_execute(self, test: impl FnOnce() -> ()) { + self.build().execute_with(|| { + test(); + Referenda::do_try_state().unwrap(); + }) + } } #[derive(Encode, Debug, Decode, TypeInfo, Eq, PartialEq, Clone, MaxEncodedLen)] diff --git a/frame/referenda/src/tests.rs b/frame/referenda/src/tests.rs index 0a1561d001a7d..83e11809714e5 100644 --- a/frame/referenda/src/tests.rs +++ b/frame/referenda/src/tests.rs @@ -30,7 +30,7 @@ use pallet_balances::Error as BalancesError; #[test] fn params_should_work() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_eq!(ReferendumCount::::get(), 0); assert_eq!(Balances::free_balance(42), 0); assert_eq!(Balances::total_issuance(), 600); @@ -39,7 +39,7 @@ fn params_should_work() { #[test] fn basic_happy_path_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { // #1: submit assert_ok!(Referenda::submit( RuntimeOrigin::signed(1), @@ -75,7 +75,7 @@ fn basic_happy_path_works() { #[test] fn insta_confirm_then_kill_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let r = Confirming { immediate: true }.create(); run_to(6); assert_ok!(Referenda::kill(RuntimeOrigin::root(), r)); @@ -85,7 +85,7 @@ fn insta_confirm_then_kill_works() { #[test] fn confirm_then_reconfirm_with_elapsed_trigger_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let r = Confirming { immediate: false }.create(); assert_eq!(confirming_until(r), 8); run_to(7); @@ -99,7 +99,7 @@ fn confirm_then_reconfirm_with_elapsed_trigger_works() { #[test] fn instaconfirm_then_reconfirm_with_elapsed_trigger_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let r = Confirming { immediate: true }.create(); run_to(6); assert_eq!(confirming_until(r), 7); @@ -113,7 +113,7 @@ fn instaconfirm_then_reconfirm_with_elapsed_trigger_works() { #[test] fn instaconfirm_then_reconfirm_with_voting_trigger_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let r = Confirming { immediate: true }.create(); run_to(6); assert_eq!(confirming_until(r), 7); @@ -131,7 +131,7 @@ fn instaconfirm_then_reconfirm_with_voting_trigger_works() { #[test] fn voting_should_extend_for_late_confirmation() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let r = Passing.create(); run_to(10); assert_eq!(confirming_until(r), 11); @@ -142,7 +142,7 @@ fn voting_should_extend_for_late_confirmation() { #[test] fn should_instafail_during_extension_confirmation() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let r = Passing.create(); run_to(10); assert_eq!(confirming_until(r), 11); @@ -155,7 +155,7 @@ fn should_instafail_during_extension_confirmation() { #[test] fn confirming_then_fail_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let r = Failing.create(); // Normally ends at 5 + 4 (voting period) = 9. assert_eq!(deciding_and_failing_since(r), 5); @@ -170,7 +170,7 @@ fn confirming_then_fail_works() { #[test] fn queueing_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { // Submit a proposal into a track with a queue len of 1. assert_ok!(Referenda::submit( RuntimeOrigin::signed(5), @@ -267,7 +267,7 @@ fn queueing_works() { #[test] fn alarm_interval_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let call = ::Preimages::bound(CallOf::::from(Call::nudge_referendum { index: 0, @@ -288,7 +288,7 @@ fn alarm_interval_works() { #[test] fn decision_time_is_correct() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let decision_time = |since: u64| { Pallet::::decision_time( &DecidingStatus { since: since.into(), confirming: None }, @@ -306,7 +306,7 @@ fn decision_time_is_correct() { #[test] fn auto_timeout_should_happen_with_nothing_but_submit() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { // #1: submit assert_ok!(Referenda::submit( RuntimeOrigin::signed(1), @@ -327,7 +327,7 @@ fn auto_timeout_should_happen_with_nothing_but_submit() { #[test] fn tracks_are_distinguished() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { assert_ok!(Referenda::submit( RuntimeOrigin::signed(1), Box::new(RawOrigin::Root.into()), @@ -388,7 +388,7 @@ fn tracks_are_distinguished() { #[test] fn submit_errors_work() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let h = set_balance_proposal_bounded(1); // No track for Signed origins. assert_noop!( @@ -416,7 +416,7 @@ fn submit_errors_work() { #[test] fn decision_deposit_errors_work() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let e = Error::::NotOngoing; assert_noop!(Referenda::place_decision_deposit(RuntimeOrigin::signed(2), 0), e); @@ -438,7 +438,7 @@ fn decision_deposit_errors_work() { #[test] fn refund_deposit_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let e = Error::::BadReferendum; assert_noop!(Referenda::refund_decision_deposit(RuntimeOrigin::signed(1), 0), e); @@ -463,7 +463,7 @@ fn refund_deposit_works() { #[test] fn refund_submission_deposit_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { // refund of non existing referendum fails. let e = Error::::BadReferendum; assert_noop!(Referenda::refund_submission_deposit(RuntimeOrigin::signed(1), 0), e); @@ -501,7 +501,7 @@ fn refund_submission_deposit_works() { #[test] fn cancel_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let h = set_balance_proposal_bounded(1); assert_ok!(Referenda::submit( RuntimeOrigin::signed(1), @@ -520,7 +520,7 @@ fn cancel_works() { #[test] fn cancel_errors_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let h = set_balance_proposal_bounded(1); assert_ok!(Referenda::submit( RuntimeOrigin::signed(1), @@ -538,7 +538,7 @@ fn cancel_errors_works() { #[test] fn kill_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let h = set_balance_proposal_bounded(1); assert_ok!(Referenda::submit( RuntimeOrigin::signed(1), @@ -558,7 +558,7 @@ fn kill_works() { #[test] fn kill_errors_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let h = set_balance_proposal_bounded(1); assert_ok!(Referenda::submit( RuntimeOrigin::signed(1), @@ -599,7 +599,7 @@ fn curve_handles_all_inputs() { #[test] fn set_metadata_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { use frame_support::traits::Hash as PreimageHash; // invalid preimage hash. let invalid_hash: PreimageHash = [1u8; 32].into(); @@ -647,7 +647,7 @@ fn set_metadata_works() { #[test] fn clear_metadata_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build_and_execute(|| { let hash = note_preimage(1); assert_ok!(Referenda::submit( RuntimeOrigin::signed(1), From 1ccc5e23101bf096ba08ebb648aefdfeb555db95 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Tue, 18 Apr 2023 22:53:52 +0200 Subject: [PATCH 02/25] fix & more tests --- frame/referenda/src/benchmarking.rs | 2 +- frame/referenda/src/lib.rs | 30 ++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/frame/referenda/src/benchmarking.rs b/frame/referenda/src/benchmarking.rs index 288c65feae567..ec67273370678 100644 --- a/frame/referenda/src/benchmarking.rs +++ b/frame/referenda/src/benchmarking.rs @@ -657,7 +657,7 @@ benchmarks_instance_pallet! { impl_benchmark_test_suite!( Referenda, - crate::mock::new_test_ext(), + crate::mock::ExtBuilder::default().build(), crate::mock::Test ); } diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 4cdfa223be6c7..8595e65955ea7 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1292,8 +1292,13 @@ impl, I: 'static> Pallet { /// Looking at referenda info: /// /// * The submission deposit cannot be less than `T::SubmissionDeposit`. + /// * There must exist track info for the track of the referendum. + /// * The decision deposit if provided cannot be less than the decision deposit of the track. + /// * The deciding stage has to begin before confirmation period. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> DispatchResult { + use sp_runtime::traits::Bounded; + ensure!( ReferendumCount::::get() as usize == ReferendumInfoFor::::iter_keys().count(), @@ -1308,8 +1313,31 @@ impl, I: 'static> Pallet { ReferendumInfo::Ongoing(status) => { ensure!( status.submission_deposit.amount >= T::SubmissionDeposit::get(), - DispatchError::Other("Incorrect submission deposit.") + DispatchError::Other("Submission deposit too small.") ); + + ensure!( + Self::track(status.track).is_some(), + DispatchError::Other("No track info for the track of the referendum.") + ); + + if let Some(decision_deposit) = status.decision_deposit { + ensure!( + decision_deposit.amount >= + Self::track(status.track).unwrap().decision_deposit, + DispatchError::Other("Decision deposit too small.") + ) + } + + if let Some(deciding) = status.deciding { + ensure!( + deciding.since < + deciding.confirming.unwrap_or(T::BlockNumber::max_value()), + DispatchError::Other( + "Deciding status cannot begin before confirming stage." + ) + ) + } }, _ => {}, } From af16dd4c3adc83ddaf7c9322b37c02ac46139949 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Sat, 22 Apr 2023 14:45:43 +0200 Subject: [PATCH 03/25] checking more stuff --- frame/referenda/src/lib.rs | 111 ++++++++++++++++++++++++++----------- 1 file changed, 78 insertions(+), 33 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 8595e65955ea7..9b3d2b0a54dc1 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1036,7 +1036,7 @@ impl, I: 'static> Pallet { /// - If it's ready to be decided, start deciding; /// - If it's not ready to be decided and non-deciding timeout has passed, fail; /// - If it's ongoing and passing, ensure confirming; if at end of confirmation period, pass. - /// - If it's ongoing and not passing, stop confirning; if it has reached end time, fail. + /// - If it's ongoing and not passing, stop confirming; if it has reached end time, fail. /// /// Weight will be a bit different depending on what it does, but it's designed so as not to /// differ dramatically, especially if `MaxQueue` is kept small. In particular _there are no @@ -1095,6 +1095,7 @@ impl, I: 'static> Pallet { branch = if status.decision_deposit.is_some() { let prepare_end = status.submitted.saturating_add(track.prepare_period); if now >= prepare_end { + println!("CAOCAO 👋"); let (maybe_alarm, branch) = Self::ready_for_deciding(now, track, index, &mut status); if let Some(set_alarm) = maybe_alarm { @@ -1291,10 +1292,21 @@ impl, I: 'static> Pallet { /// /// Looking at referenda info: /// + /// - Data regarding ongoing phase: + /// /// * The submission deposit cannot be less than `T::SubmissionDeposit`. /// * There must exist track info for the track of the referendum. /// * The decision deposit if provided cannot be less than the decision deposit of the track. /// * The deciding stage has to begin before confirmation period. + /// * The proposal must get in queue before the `T::UndecidingTimeout` passes. + /// + /// - Data in `ReferendumInfo::Approved`, `ReferendumInfo::Rejected`, + /// `ReferendumInfo::Cancelled` and `ReferendumInfo::TimedOut`: + /// + /// * The submission deposit cannot be less than `T::SubmissionDeposit`. + /// + /// Looking at tracks: + /// * The TrackQueue should be empty if `DecidingCount` is less than `TrackInfo::max_deciding`. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> DispatchResult { use sp_runtime::traits::Bounded; @@ -1307,43 +1319,76 @@ impl, I: 'static> Pallet { ) ); - ReferendumInfoFor::::iter().try_for_each( - |(_, referendum)| -> Result<(), DispatchError> { - match referendum { - ReferendumInfo::Ongoing(status) => { - ensure!( - status.submission_deposit.amount >= T::SubmissionDeposit::get(), - DispatchError::Other("Submission deposit too small.") - ); + ReferendumInfoFor::::iter().try_for_each(|(_, referendum)| -> DispatchResult { + match referendum { + ReferendumInfo::Ongoing(status) => { + ensure!( + status.submission_deposit.amount >= T::SubmissionDeposit::get(), + DispatchError::Other("Submission deposit too small.") + ); + + ensure!( + Self::track(status.track).is_some(), + DispatchError::Other("No track info for the track of the referendum.") + ); + if let Some(decision_deposit) = status.decision_deposit { ensure!( - Self::track(status.track).is_some(), - DispatchError::Other("No track info for the track of the referendum.") - ); + decision_deposit.amount >= + Self::track(status.track).unwrap().decision_deposit, + DispatchError::Other("Decision deposit too small.") + ) + } - if let Some(decision_deposit) = status.decision_deposit { - ensure!( - decision_deposit.amount >= - Self::track(status.track).unwrap().decision_deposit, - DispatchError::Other("Decision deposit too small.") + if let Some(deciding) = status.deciding { + ensure!( + deciding.since < + deciding.confirming.unwrap_or(T::BlockNumber::max_value()), + DispatchError::Other( + "Deciding status cannot begin before confirming stage." ) - } + ) + } + /* + let now = frame_system::Pallet::::block_number(); + let timeout = status.clone().submitted.saturating_add(T::UndecidingTimeout::get()); - if let Some(deciding) = status.deciding { - ensure!( - deciding.since < - deciding.confirming.unwrap_or(T::BlockNumber::max_value()), - DispatchError::Other( - "Deciding status cannot begin before confirming stage." - ) - ) - } - }, - _ => {}, - } - Ok(()) - }, - )?; + if status.deciding.is_none() && timeout > now { + ensure!(status.in_queue == true, DispatchError::Other("The submission must be in queue by now.")); + } + */ + }, + ReferendumInfo::Approved(_, maybe_submission_deposit, _) | + ReferendumInfo::Rejected(_, maybe_submission_deposit, _) | + ReferendumInfo::Cancelled(_, maybe_submission_deposit, _) | + ReferendumInfo::TimedOut(_, maybe_submission_deposit, _) => { + if let Some(submission_deposit) = maybe_submission_deposit { + ensure!( + submission_deposit.amount >= T::SubmissionDeposit::get(), + DispatchError::Other("Submission deposit too small.") + ); + } + }, + _ => {}, + } + Ok(()) + })?; + + T::Tracks::tracks().iter().try_for_each(|track| -> DispatchResult { + if DecidingCount::::get(track.0) < track.1.max_deciding { + let timed_out = TrackQueue::::get(track.0).iter().filter(|(i, _)| match ReferendumInfoFor::::get(i).unwrap() { + ReferendumInfo::TimedOut(..) => true, + _ => false + }).count(); + + // Timed out proposals cannot be in the deciding phase. + ensure!( + (TrackQueue::::get(track.0).iter().count()).saturating_sub(timed_out) == 0, + DispatchError::Other("`TrackQueue` should be empty when `DecidingCount` is less than `TrackInfo::max_decidin`.") + ); + } + Ok(()) + })?; Ok(()) } From 6eb9ab6e6914c0d0683b44388779d05d8de3423d Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Sat, 22 Apr 2023 15:21:09 +0200 Subject: [PATCH 04/25] remove log --- frame/referenda/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 9b3d2b0a54dc1..9a2cc71f8fe29 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1095,7 +1095,6 @@ impl, I: 'static> Pallet { branch = if status.decision_deposit.is_some() { let prepare_end = status.submitted.saturating_add(track.prepare_period); if now >= prepare_end { - println!("CAOCAO 👋"); let (maybe_alarm, branch) = Self::ready_for_deciding(now, track, index, &mut status); if let Some(set_alarm) = maybe_alarm { From a267bd50421056f3494f1cf21cbf5c3ebb4bad49 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Sat, 22 Apr 2023 20:23:58 +0200 Subject: [PATCH 05/25] two more tests --- frame/referenda/src/lib.rs | 47 +++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 9a2cc71f8fe29..d1ddcdaadd11d 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1285,9 +1285,11 @@ impl, I: 'static> Pallet { /// /// The following assertions must always apply. /// - /// Looking at referenda: + /// General assertions: /// /// * `ReferendumCount` must always be equal to the number of referenda in `ReferendumInfoFor`. + /// * Referendum indices must be sorted in the `ReferendumInfoOf` storage map. + /// * Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoOf`. /// /// Looking at referenda info: /// @@ -1297,7 +1299,6 @@ impl, I: 'static> Pallet { /// * There must exist track info for the track of the referendum. /// * The decision deposit if provided cannot be less than the decision deposit of the track. /// * The deciding stage has to begin before confirmation period. - /// * The proposal must get in queue before the `T::UndecidingTimeout` passes. /// /// - Data in `ReferendumInfo::Approved`, `ReferendumInfo::Rejected`, /// `ReferendumInfo::Cancelled` and `ReferendumInfo::TimedOut`: @@ -1305,7 +1306,10 @@ impl, I: 'static> Pallet { /// * The submission deposit cannot be less than `T::SubmissionDeposit`. /// /// Looking at tracks: - /// * The TrackQueue should be empty if `DecidingCount` is less than `TrackInfo::max_deciding`. + /// * The `TrackQueue` should be empty if `DecidingCount` is less than + /// `TrackInfo::max_deciding`. + /// * The referendum indices stored in TrackQueue` must exist as keys in the `ReferendumInfoFor` + /// storage map. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> DispatchResult { use sp_runtime::traits::Bounded; @@ -1318,6 +1322,27 @@ impl, I: 'static> Pallet { ) ); + // The order of indices when running `queueing_works` is: 0, 4, 2, 1, 3 🤔 + /* + ensure!( + ReferendumInfoFor::::iter_keys() + .collect::>() + .windows(2) + .all(|w| w[0] < w[1]), + DispatchError::Other( + "Referenda indices must be sorted in `ReferendumInfoFor` storage map" + ) + ); + */ + + MetadataOf::::iter_keys().try_for_each(|referendum_index| -> DispatchResult { + ensure!( + ReferendumInfoFor::::contains_key(referendum_index), + DispatchError::Other("Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoOf`") + ); + Ok(()) + })?; + ReferendumInfoFor::::iter().try_for_each(|(_, referendum)| -> DispatchResult { match referendum { ReferendumInfo::Ongoing(status) => { @@ -1348,14 +1373,6 @@ impl, I: 'static> Pallet { ) ) } - /* - let now = frame_system::Pallet::::block_number(); - let timeout = status.clone().submitted.saturating_add(T::UndecidingTimeout::get()); - - if status.deciding.is_none() && timeout > now { - ensure!(status.in_queue == true, DispatchError::Other("The submission must be in queue by now.")); - } - */ }, ReferendumInfo::Approved(_, maybe_submission_deposit, _) | ReferendumInfo::Rejected(_, maybe_submission_deposit, _) | @@ -1386,6 +1403,14 @@ impl, I: 'static> Pallet { DispatchError::Other("`TrackQueue` should be empty when `DecidingCount` is less than `TrackInfo::max_decidin`.") ); } + + TrackQueue::::get(track.0).iter().try_for_each(|(referendum_index, _)| -> DispatchResult { + ensure!( + ReferendumInfoFor::::contains_key(referendum_index), + DispatchError::Other("`ReferendumIndex` inside the `TrackQueue` should be a key in `ReferendumInfoFor`") + ); + Ok(()) + })?; Ok(()) })?; From 203030a5bb72488d91ac1a21845e8f7bbebf6e23 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Tue, 25 Apr 2023 15:59:37 +0200 Subject: [PATCH 06/25] Update frame/referenda/src/lib.rs Co-authored-by: Muharem Ismailov --- frame/referenda/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index d1ddcdaadd11d..6e7e963095a33 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1308,7 +1308,7 @@ impl, I: 'static> Pallet { /// Looking at tracks: /// * The `TrackQueue` should be empty if `DecidingCount` is less than /// `TrackInfo::max_deciding`. - /// * The referendum indices stored in TrackQueue` must exist as keys in the `ReferendumInfoFor` + /// * The referendum indices stored in `TrackQueue` must exist as keys in the `ReferendumInfoFor` /// storage map. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> DispatchResult { From 678c9fa49077dbbf29ee00d615c0cbcbf9cc670b Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Tue, 25 Apr 2023 16:05:50 +0200 Subject: [PATCH 07/25] fixes --- frame/referenda/src/lib.rs | 53 ++++---------------------------- frame/referenda/src/migration.rs | 4 --- 2 files changed, 6 insertions(+), 51 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 6e7e963095a33..2dcb1c5d36c42 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1295,20 +1295,14 @@ impl, I: 'static> Pallet { /// /// - Data regarding ongoing phase: /// - /// * The submission deposit cannot be less than `T::SubmissionDeposit`. /// * There must exist track info for the track of the referendum. - /// * The decision deposit if provided cannot be less than the decision deposit of the track. /// * The deciding stage has to begin before confirmation period. /// - /// - Data in `ReferendumInfo::Approved`, `ReferendumInfo::Rejected`, - /// `ReferendumInfo::Cancelled` and `ReferendumInfo::TimedOut`: - /// - /// * The submission deposit cannot be less than `T::SubmissionDeposit`. - /// /// Looking at tracks: /// * The `TrackQueue` should be empty if `DecidingCount` is less than /// `TrackInfo::max_deciding`. - /// * The referendum indices stored in `TrackQueue` must exist as keys in the `ReferendumInfoFor` + /// * The referendum indices stored in `TrackQueue` must exist as keys in the + /// `ReferendumInfoFor` /// storage map. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> DispatchResult { @@ -1322,23 +1316,12 @@ impl, I: 'static> Pallet { ) ); - // The order of indices when running `queueing_works` is: 0, 4, 2, 1, 3 🤔 - /* - ensure!( - ReferendumInfoFor::::iter_keys() - .collect::>() - .windows(2) - .all(|w| w[0] < w[1]), - DispatchError::Other( - "Referenda indices must be sorted in `ReferendumInfoFor` storage map" - ) - ); - */ - MetadataOf::::iter_keys().try_for_each(|referendum_index| -> DispatchResult { ensure!( - ReferendumInfoFor::::contains_key(referendum_index), - DispatchError::Other("Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoOf`") + ReferendumInfoFor::::contains_key(referendum_index), + DispatchError::Other( + "Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoOf`" + ) ); Ok(()) })?; @@ -1346,24 +1329,11 @@ impl, I: 'static> Pallet { ReferendumInfoFor::::iter().try_for_each(|(_, referendum)| -> DispatchResult { match referendum { ReferendumInfo::Ongoing(status) => { - ensure!( - status.submission_deposit.amount >= T::SubmissionDeposit::get(), - DispatchError::Other("Submission deposit too small.") - ); - ensure!( Self::track(status.track).is_some(), DispatchError::Other("No track info for the track of the referendum.") ); - if let Some(decision_deposit) = status.decision_deposit { - ensure!( - decision_deposit.amount >= - Self::track(status.track).unwrap().decision_deposit, - DispatchError::Other("Decision deposit too small.") - ) - } - if let Some(deciding) = status.deciding { ensure!( deciding.since < @@ -1374,17 +1344,6 @@ impl, I: 'static> Pallet { ) } }, - ReferendumInfo::Approved(_, maybe_submission_deposit, _) | - ReferendumInfo::Rejected(_, maybe_submission_deposit, _) | - ReferendumInfo::Cancelled(_, maybe_submission_deposit, _) | - ReferendumInfo::TimedOut(_, maybe_submission_deposit, _) => { - if let Some(submission_deposit) = maybe_submission_deposit { - ensure!( - submission_deposit.amount >= T::SubmissionDeposit::get(), - DispatchError::Other("Submission deposit too small.") - ); - } - }, _ => {}, } Ok(()) diff --git a/frame/referenda/src/migration.rs b/frame/referenda/src/migration.rs index 9cfa5b3a65f78..cd48b17500ba2 100644 --- a/frame/referenda/src/migration.rs +++ b/frame/referenda/src/migration.rs @@ -82,10 +82,6 @@ pub mod v0 { #[storage_alias] pub type ReferendumInfoFor, I: 'static> = StorageMap, Blake2_128Concat, ReferendumIndex, ReferendumInfoOf>; - - #[storage_alias] - pub type ReferendumCount, I: 'static> = - StorageValue, ReferendumIndex, ValueQuery>; } pub mod v1 { From e045e12914ae96fa2cf9314b28525f4386fa3d32 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Sun, 30 Apr 2023 09:18:01 +0200 Subject: [PATCH 08/25] new check --- frame/referenda/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 2dcb1c5d36c42..09d09a8fcd0aa 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1297,6 +1297,8 @@ impl, I: 'static> Pallet { /// /// * There must exist track info for the track of the referendum. /// * The deciding stage has to begin before confirmation period. + /// * If alarm is set the nudge call has to be at most `UndecidingTimeout` blocks away + /// from the submission block. /// /// Looking at tracks: /// * The `TrackQueue` should be empty if `DecidingCount` is less than @@ -1343,6 +1345,10 @@ impl, I: 'static> Pallet { ) ) } + + if let Some(alarm) = status.alarm { + ensure!(alarm.0 <= status.submitted.saturating_add(T::UndecidingTimeout::get()), "The alarm cannot be set more than `UndecidingTimeout` away from the submission block.") + } }, _ => {}, } From beae91fe87300b75616572188085670981709b6d Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Thu, 22 Jun 2023 09:32:22 +0200 Subject: [PATCH 09/25] merge fixes --- frame/referenda/src/lib.rs | 46 ++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 8bfcdbe1718c0..302ad2b5c0b06 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -85,6 +85,9 @@ use sp_runtime::{ }; use sp_std::{fmt::Debug, prelude::*}; +#[cfg(any(test, feature = "try-runtime"))] +use sp_runtime::TryRuntimeError; + mod branch; pub mod migration; mod types; @@ -419,7 +422,7 @@ pub mod pallet { #[pallet::hooks] impl, I: 'static> Hooks for Pallet { #[cfg(feature = "try-runtime")] - fn try_state(_n: T::BlockNumber) -> Result<(), &'static str> { + fn try_state(_n: T::BlockNumber) -> Result<(), TryRuntimeError> { Self::do_try_state()?; Ok(()) } @@ -1320,23 +1323,19 @@ impl, I: 'static> Pallet { /// `ReferendumInfoFor` /// storage map. #[cfg(any(feature = "try-runtime", test))] - fn do_try_state() -> DispatchResult { + fn do_try_state() -> Result<(), TryRuntimeError> { use sp_runtime::traits::Bounded; ensure!( ReferendumCount::::get() as usize == ReferendumInfoFor::::iter_keys().count(), - DispatchError::Other( - "Number of referenda in `ReferendumInfoFor` is greater than `ReferendumCount`" - ) + "Number of referenda in `ReferendumInfoFor` is greater than `ReferendumCount`" ); MetadataOf::::iter_keys().try_for_each(|referendum_index| -> DispatchResult { ensure!( ReferendumInfoFor::::contains_key(referendum_index), - DispatchError::Other( - "Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoOf`" - ) + "Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoOf`" ); Ok(()) })?; @@ -1346,16 +1345,14 @@ impl, I: 'static> Pallet { ReferendumInfo::Ongoing(status) => { ensure!( Self::track(status.track).is_some(), - DispatchError::Other("No track info for the track of the referendum.") + "No track info for the track of the referendum." ); if let Some(deciding) = status.deciding { ensure!( deciding.since < deciding.confirming.unwrap_or(T::BlockNumber::max_value()), - DispatchError::Other( "Deciding status cannot begin before confirming stage." - ) ) } @@ -1370,25 +1367,30 @@ impl, I: 'static> Pallet { T::Tracks::tracks().iter().try_for_each(|track| -> DispatchResult { if DecidingCount::::get(track.0) < track.1.max_deciding { - let timed_out = TrackQueue::::get(track.0).iter().filter(|(i, _)| match ReferendumInfoFor::::get(i).unwrap() { - ReferendumInfo::TimedOut(..) => true, - _ => false - }).count(); + let timed_out = TrackQueue::::get(track.0) + .iter() + .filter(|(i, _)| match ReferendumInfoFor::::get(i).unwrap() { + ReferendumInfo::TimedOut(..) => true, + _ => false, + }) + .count(); // Timed out proposals cannot be in the deciding phase. ensure!( (TrackQueue::::get(track.0).iter().count()).saturating_sub(timed_out) == 0, - DispatchError::Other("`TrackQueue` should be empty when `DecidingCount` is less than `TrackInfo::max_decidin`.") + "`TrackQueue` should be empty when `DecidingCount` is less than `TrackInfo::max_decidin`." ); } - TrackQueue::::get(track.0).iter().try_for_each(|(referendum_index, _)| -> DispatchResult { - ensure!( - ReferendumInfoFor::::contains_key(referendum_index), - DispatchError::Other("`ReferendumIndex` inside the `TrackQueue` should be a key in `ReferendumInfoFor`") + TrackQueue::::get(track.0).iter().try_for_each( + |(referendum_index, _)| -> DispatchResult { + ensure!( + ReferendumInfoFor::::contains_key(referendum_index), + "`ReferendumIndex` inside the `TrackQueue` should be a key in `ReferendumInfoFor`" ); - Ok(()) - })?; + Ok(()) + }, + )?; Ok(()) })?; From df933c347a07fd561aa8b99628c76fdb60880e14 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Thu, 22 Jun 2023 09:51:13 +0200 Subject: [PATCH 10/25] fix warning --- frame/referenda/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 302ad2b5c0b06..c72df6f53327e 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1324,8 +1324,6 @@ impl, I: 'static> Pallet { /// storage map. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), TryRuntimeError> { - use sp_runtime::traits::Bounded; - ensure!( ReferendumCount::::get() as usize == ReferendumInfoFor::::iter_keys().count(), From 40cd0975f711043276fea16c3a72c9aabe589e1e Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Tue, 27 Jun 2023 15:22:40 +0200 Subject: [PATCH 11/25] remove check --- frame/referenda/src/lib.rs | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index c72df6f53327e..be60b0a5df29f 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1317,8 +1317,6 @@ impl, I: 'static> Pallet { /// from the submission block. /// /// Looking at tracks: - /// * The `TrackQueue` should be empty if `DecidingCount` is less than - /// `TrackInfo::max_deciding`. /// * The referendum indices stored in `TrackQueue` must exist as keys in the /// `ReferendumInfoFor` /// storage map. @@ -1364,22 +1362,6 @@ impl, I: 'static> Pallet { })?; T::Tracks::tracks().iter().try_for_each(|track| -> DispatchResult { - if DecidingCount::::get(track.0) < track.1.max_deciding { - let timed_out = TrackQueue::::get(track.0) - .iter() - .filter(|(i, _)| match ReferendumInfoFor::::get(i).unwrap() { - ReferendumInfo::TimedOut(..) => true, - _ => false, - }) - .count(); - - // Timed out proposals cannot be in the deciding phase. - ensure!( - (TrackQueue::::get(track.0).iter().count()).saturating_sub(timed_out) == 0, - "`TrackQueue` should be empty when `DecidingCount` is less than `TrackInfo::max_decidin`." - ); - } - TrackQueue::::get(track.0).iter().try_for_each( |(referendum_index, _)| -> DispatchResult { ensure!( From 358c8a81e034198f07313d55a2c9869c75d07bc2 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Tue, 27 Jun 2023 19:28:03 +0200 Subject: [PATCH 12/25] Update frame/referenda/src/lib.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Gonçalo Pestana --- frame/referenda/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index be60b0a5df29f..d1b74e51e260a 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1325,7 +1325,7 @@ impl, I: 'static> Pallet { ensure!( ReferendumCount::::get() as usize == ReferendumInfoFor::::iter_keys().count(), - "Number of referenda in `ReferendumInfoFor` is greater than `ReferendumCount`" + "Number of referenda in `ReferendumInfoFor` is different than `ReferendumCount`" ); MetadataOf::::iter_keys().try_for_each(|referendum_index| -> DispatchResult { From dec6227c013a43c213145d8ed22315330aea3d6a Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Tue, 27 Jun 2023 19:41:41 +0200 Subject: [PATCH 13/25] separate into multiple functions --- frame/referenda/src/lib.rs | 51 ++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index d1b74e51e260a..f4b07a98cd5f9 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1304,22 +1304,8 @@ impl, I: 'static> Pallet { /// General assertions: /// /// * `ReferendumCount` must always be equal to the number of referenda in `ReferendumInfoFor`. - /// * Referendum indices must be sorted in the `ReferendumInfoOf` storage map. - /// * Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoOf`. - /// - /// Looking at referenda info: - /// - /// - Data regarding ongoing phase: - /// - /// * There must exist track info for the track of the referendum. - /// * The deciding stage has to begin before confirmation period. - /// * If alarm is set the nudge call has to be at most `UndecidingTimeout` blocks away - /// from the submission block. - /// - /// Looking at tracks: - /// * The referendum indices stored in `TrackQueue` must exist as keys in the - /// `ReferendumInfoFor` - /// storage map. + /// * Referendum indices must be sorted in the `ReferendumInfoFor` storage map. + /// * Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoFor`. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), TryRuntimeError> { ensure!( @@ -1328,6 +1314,14 @@ impl, I: 'static> Pallet { "Number of referenda in `ReferendumInfoFor` is different than `ReferendumCount`" ); + ensure!( + ReferendumInfoFor::::iter_keys() + .collect::>() + .windows(2) + .all(|v| v[0] <= v[1]), + "Referendum indices must be sorted in the `ReferendumInfoFor` storage map" + ); + MetadataOf::::iter_keys().try_for_each(|referendum_index| -> DispatchResult { ensure!( ReferendumInfoFor::::contains_key(referendum_index), @@ -1336,6 +1330,22 @@ impl, I: 'static> Pallet { Ok(()) })?; + Self::try_state_referenda_info()?; + Self::try_state_tracks()?; + + Ok(()) + } + + /// Looking at referenda info: + /// + /// - Data regarding ongoing phase: + /// + /// * There must exist track info for the track of the referendum. + /// * The deciding stage has to begin before confirmation period. + /// * If alarm is set the nudge call has to be at most `UndecidingTimeout` blocks away + /// from the submission block. + #[cfg(any(feature = "try-runtime", test))] + fn try_state_referenda_info() -> Result<(), TryRuntimeError> { ReferendumInfoFor::::iter().try_for_each(|(_, referendum)| -> DispatchResult { match referendum { ReferendumInfo::Ongoing(status) => { @@ -1361,6 +1371,15 @@ impl, I: 'static> Pallet { Ok(()) })?; + Ok(()) + } + + /// Looking at tracks: + /// + /// * The referendum indices stored in `TrackQueue` must exist as keys in the + /// `ReferendumInfoFor` storage map. + #[cfg(any(feature = "try-runtime", test))] + fn try_state_tracks() -> Result<(), TryRuntimeError> { T::Tracks::tracks().iter().try_for_each(|track| -> DispatchResult { TrackQueue::::get(track.0).iter().try_for_each( |(referendum_index, _)| -> DispatchResult { From b7762589096e7aeb4e0cc6a9e06dde2e27e58ea6 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Tue, 27 Jun 2023 19:56:04 +0200 Subject: [PATCH 14/25] clean up --- frame/referenda/src/lib.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index f4b07a98cd5f9..c64f192477235 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1369,9 +1369,7 @@ impl, I: 'static> Pallet { _ => {}, } Ok(()) - })?; - - Ok(()) + }) } /// Looking at tracks: @@ -1380,9 +1378,9 @@ impl, I: 'static> Pallet { /// `ReferendumInfoFor` storage map. #[cfg(any(feature = "try-runtime", test))] fn try_state_tracks() -> Result<(), TryRuntimeError> { - T::Tracks::tracks().iter().try_for_each(|track| -> DispatchResult { + T::Tracks::tracks().iter().try_for_each(|track| { TrackQueue::::get(track.0).iter().try_for_each( - |(referendum_index, _)| -> DispatchResult { + |(referendum_index, _)| -> Result<(), TryRuntimeError> { ensure!( ReferendumInfoFor::::contains_key(referendum_index), "`ReferendumIndex` inside the `TrackQueue` should be a key in `ReferendumInfoFor`" @@ -1391,8 +1389,6 @@ impl, I: 'static> Pallet { }, )?; Ok(()) - })?; - - Ok(()) + }) } } From c4e1a3885c59a0c57057c9cea8b933bdee350175 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Tue, 27 Jun 2023 19:57:57 +0200 Subject: [PATCH 15/25] unnecessary return value specified --- frame/referenda/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index c64f192477235..003d6339db68b 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1346,7 +1346,7 @@ impl, I: 'static> Pallet { /// from the submission block. #[cfg(any(feature = "try-runtime", test))] fn try_state_referenda_info() -> Result<(), TryRuntimeError> { - ReferendumInfoFor::::iter().try_for_each(|(_, referendum)| -> DispatchResult { + ReferendumInfoFor::::iter().try_for_each(|(_, referendum)| { match referendum { ReferendumInfo::Ongoing(status) => { ensure!( From c65a0393fe251d274a1a6a977576d4c0b95492c6 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Tue, 27 Jun 2023 21:10:17 +0200 Subject: [PATCH 16/25] fix --- frame/referenda/src/lib.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 003d6339db68b..dcefc36ae64be 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1304,7 +1304,6 @@ impl, I: 'static> Pallet { /// General assertions: /// /// * `ReferendumCount` must always be equal to the number of referenda in `ReferendumInfoFor`. - /// * Referendum indices must be sorted in the `ReferendumInfoFor` storage map. /// * Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoFor`. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), TryRuntimeError> { @@ -1314,14 +1313,6 @@ impl, I: 'static> Pallet { "Number of referenda in `ReferendumInfoFor` is different than `ReferendumCount`" ); - ensure!( - ReferendumInfoFor::::iter_keys() - .collect::>() - .windows(2) - .all(|v| v[0] <= v[1]), - "Referendum indices must be sorted in the `ReferendumInfoFor` storage map" - ); - MetadataOf::::iter_keys().try_for_each(|referendum_index| -> DispatchResult { ensure!( ReferendumInfoFor::::contains_key(referendum_index), From cb3c11c862a3c035e5a42b387417a466627ad409 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Fri, 30 Jun 2023 15:36:52 +0200 Subject: [PATCH 17/25] Update frame/referenda/src/lib.rs --- frame/referenda/src/lib.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index dcefc36ae64be..13a55230ef880 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1352,10 +1352,6 @@ impl, I: 'static> Pallet { "Deciding status cannot begin before confirming stage." ) } - - if let Some(alarm) = status.alarm { - ensure!(alarm.0 <= status.submitted.saturating_add(T::UndecidingTimeout::get()), "The alarm cannot be set more than `UndecidingTimeout` away from the submission block.") - } }, _ => {}, } From 3b6655001799ac6db6a122dfd9762e1ddbad88dd Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Fri, 30 Jun 2023 16:16:40 +0200 Subject: [PATCH 18/25] fmt --- frame/referenda/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 13a55230ef880..b69208445f586 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1349,7 +1349,7 @@ impl, I: 'static> Pallet { ensure!( deciding.since < deciding.confirming.unwrap_or(T::BlockNumber::max_value()), - "Deciding status cannot begin before confirming stage." + "Deciding status cannot begin before confirming stage." ) } }, From 011d5b34cfad0b74e0957ab1f2e66dcf264e029c Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Fri, 28 Jul 2023 09:01:29 -0700 Subject: [PATCH 19/25] remove import --- frame/referenda/src/lib.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index b69208445f586..dc80fbb5bcfe8 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -85,9 +85,6 @@ use sp_runtime::{ }; use sp_std::{fmt::Debug, prelude::*}; -#[cfg(any(test, feature = "try-runtime"))] -use sp_runtime::TryRuntimeError; - mod branch; pub mod migration; mod types; @@ -422,7 +419,7 @@ pub mod pallet { #[pallet::hooks] impl, I: 'static> Hooks for Pallet { #[cfg(feature = "try-runtime")] - fn try_state(_n: T::BlockNumber) -> Result<(), TryRuntimeError> { + fn try_state(_n: T::BlockNumber) -> Result<(), sp_runtime::TryRuntimeError> { Self::do_try_state()?; Ok(()) } @@ -1306,7 +1303,7 @@ impl, I: 'static> Pallet { /// * `ReferendumCount` must always be equal to the number of referenda in `ReferendumInfoFor`. /// * Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoFor`. #[cfg(any(feature = "try-runtime", test))] - fn do_try_state() -> Result<(), TryRuntimeError> { + fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> { ensure!( ReferendumCount::::get() as usize == ReferendumInfoFor::::iter_keys().count(), @@ -1336,7 +1333,7 @@ impl, I: 'static> Pallet { /// * If alarm is set the nudge call has to be at most `UndecidingTimeout` blocks away /// from the submission block. #[cfg(any(feature = "try-runtime", test))] - fn try_state_referenda_info() -> Result<(), TryRuntimeError> { + fn try_state_referenda_info() -> Result<(), sp_runtime::TryRuntimeError> { ReferendumInfoFor::::iter().try_for_each(|(_, referendum)| { match referendum { ReferendumInfo::Ongoing(status) => { @@ -1364,10 +1361,10 @@ impl, I: 'static> Pallet { /// * The referendum indices stored in `TrackQueue` must exist as keys in the /// `ReferendumInfoFor` storage map. #[cfg(any(feature = "try-runtime", test))] - fn try_state_tracks() -> Result<(), TryRuntimeError> { + fn try_state_tracks() -> Result<(), sp_runtime::TryRuntimeError> { T::Tracks::tracks().iter().try_for_each(|track| { TrackQueue::::get(track.0).iter().try_for_each( - |(referendum_index, _)| -> Result<(), TryRuntimeError> { + |(referendum_index, _)| -> Result<(), sp_runtime::TryRuntimeError> { ensure!( ReferendumInfoFor::::contains_key(referendum_index), "`ReferendumIndex` inside the `TrackQueue` should be a key in `ReferendumInfoFor`" From 14797f87bdbdfbc2d75d6458e8599379a03cc6d1 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Fri, 28 Jul 2023 09:16:28 -0700 Subject: [PATCH 20/25] fmt --- frame/referenda/src/mock.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/frame/referenda/src/mock.rs b/frame/referenda/src/mock.rs index 1a14370318059..e44167ed561c5 100644 --- a/frame/referenda/src/mock.rs +++ b/frame/referenda/src/mock.rs @@ -236,13 +236,13 @@ impl Default for ExtBuilder { impl ExtBuilder { pub fn build(self) -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::::default().build_storage().unwrap(); - let balances = vec![(1, 100), (2, 100), (3, 100), (4, 100), (5, 100), (6, 100)]; - pallet_balances::GenesisConfig:: { balances } - .assimilate_storage(&mut t) - .unwrap(); - let mut ext = sp_io::TestExternalities::new(t); - ext.execute_with(|| System::set_block_number(1)); - ext + let balances = vec![(1, 100), (2, 100), (3, 100), (4, 100), (5, 100), (6, 100)]; + pallet_balances::GenesisConfig:: { balances } + .assimilate_storage(&mut t) + .unwrap(); + let mut ext = sp_io::TestExternalities::new(t); + ext.execute_with(|| System::set_block_number(1)); + ext } pub fn build_and_execute(self, test: impl FnOnce() -> ()) { From 87cfc687dc7586091c15bb326c31ead9e946557e Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Fri, 28 Jul 2023 09:19:01 -0700 Subject: [PATCH 21/25] fix CI --- frame/referenda/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 1310125e91b29..567941a964028 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -418,9 +418,9 @@ pub mod pallet { } #[pallet::hooks] - impl, I: 'static> Hooks for Pallet { + impl, I: 'static> Hooks> for Pallet { #[cfg(feature = "try-runtime")] - fn try_state(_n: T::BlockNumber) -> Result<(), sp_runtime::TryRuntimeError> { + fn try_state(_n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { Self::do_try_state()?; Ok(()) } @@ -1346,7 +1346,7 @@ impl, I: 'static> Pallet { if let Some(deciding) = status.deciding { ensure!( deciding.since < - deciding.confirming.unwrap_or(T::BlockNumber::max_value()), + deciding.confirming.unwrap_or(BlockNumberFor::::max_value()), "Deciding status cannot begin before confirming stage." ) } From b153a12787ab588406c6ed6272269fa6d43ff4e8 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Fri, 28 Jul 2023 09:26:27 -0700 Subject: [PATCH 22/25] Update frame/referenda/src/lib.rs Co-authored-by: Liam Aharon --- frame/referenda/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 567941a964028..a6bea1a221359 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1302,7 +1302,7 @@ impl, I: 'static> Pallet { /// General assertions: /// /// * `ReferendumCount` must always be equal to the number of referenda in `ReferendumInfoFor`. - /// * Referendum indices in `MetadataOf` must also be stored in `ReferendumInfoFor`. + /// * Referendum indices in [`MetadataOf`] must also be stored in [`ReferendumInfoFor`]. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> { ensure!( From 2ac039d043341e2169f481e10b6a1aef9890d33d Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Fri, 28 Jul 2023 09:30:10 -0700 Subject: [PATCH 23/25] last fixes --- frame/referenda/src/lib.rs | 6 +++--- frame/referenda/src/migration.rs | 12 ++---------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index a6bea1a221359..867ba521533bd 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1301,7 +1301,7 @@ impl, I: 'static> Pallet { /// /// General assertions: /// - /// * `ReferendumCount` must always be equal to the number of referenda in `ReferendumInfoFor`. + /// * `ReferendumCount` must always be equal to the number of referenda in [`ReferendumInfoFor`]. /// * Referendum indices in [`MetadataOf`] must also be stored in [`ReferendumInfoFor`]. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> { @@ -1331,7 +1331,7 @@ impl, I: 'static> Pallet { /// /// * There must exist track info for the track of the referendum. /// * The deciding stage has to begin before confirmation period. - /// * If alarm is set the nudge call has to be at most `UndecidingTimeout` blocks away + /// * If alarm is set the nudge call has to be at most [`UndecidingTimeout`] blocks away /// from the submission block. #[cfg(any(feature = "try-runtime", test))] fn try_state_referenda_info() -> Result<(), sp_runtime::TryRuntimeError> { @@ -1359,7 +1359,7 @@ impl, I: 'static> Pallet { /// Looking at tracks: /// - /// * The referendum indices stored in `TrackQueue` must exist as keys in the + /// * The referendum indices stored in [`TrackQueue`] must exist as keys in the /// `ReferendumInfoFor` storage map. #[cfg(any(feature = "try-runtime", test))] fn try_state_tracks() -> Result<(), sp_runtime::TryRuntimeError> { diff --git a/frame/referenda/src/migration.rs b/frame/referenda/src/migration.rs index 234b36454f215..281da83d6569e 100644 --- a/frame/referenda/src/migration.rs +++ b/frame/referenda/src/migration.rs @@ -188,14 +188,6 @@ pub mod test { } } - fn increment_referendum_index() { - ReferendumCount::::mutate(|x| { - let r = *x; - *x += 1; - r - }); - } - #[test] pub fn referendum_status_v0() { // make sure the bytes of the encoded referendum v0 is decodable. @@ -211,7 +203,7 @@ pub mod test { // create and insert into the storage an ongoing referendum v0. let status_v0 = create_status_v0(); let ongoing_v0 = v0::ReferendumInfoOf::::Ongoing(status_v0.clone()); - increment_referendum_index(); + ReferendumCount::::mutate(|x| x.saturating_inc()); v0::ReferendumInfoFor::::insert(2, ongoing_v0); // create and insert into the storage an approved referendum v0. let approved_v0 = v0::ReferendumInfoOf::::Approved( @@ -219,7 +211,7 @@ pub mod test { Deposit { who: 1, amount: 10 }, Some(Deposit { who: 2, amount: 20 }), ); - increment_referendum_index(); + ReferendumCount::::mutate(|x| x.saturating_inc()); v0::ReferendumInfoFor::::insert(5, approved_v0); // run migration from v0 to v1. v1::MigrateV0ToV1::::on_runtime_upgrade(); From 59ec6a331bc30b6a3bbb2e8c287620bd70484651 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Fri, 28 Jul 2023 09:31:34 -0700 Subject: [PATCH 24/25] :P --- frame/referenda/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 867ba521533bd..551f4649f6acd 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1301,7 +1301,7 @@ impl, I: 'static> Pallet { /// /// General assertions: /// - /// * `ReferendumCount` must always be equal to the number of referenda in [`ReferendumInfoFor`]. + /// * [`ReferendumCount`] must always be equal to the number of referenda in [`ReferendumInfoFor`]. /// * Referendum indices in [`MetadataOf`] must also be stored in [`ReferendumInfoFor`]. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> { @@ -1360,7 +1360,7 @@ impl, I: 'static> Pallet { /// Looking at tracks: /// /// * The referendum indices stored in [`TrackQueue`] must exist as keys in the - /// `ReferendumInfoFor` storage map. + /// [`ReferendumInfoFor`] storage map. #[cfg(any(feature = "try-runtime", test))] fn try_state_tracks() -> Result<(), sp_runtime::TryRuntimeError> { T::Tracks::tracks().iter().try_for_each(|track| { From 7874eb1b6fb12637534ff336d686bb454868cda9 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Fri, 28 Jul 2023 09:34:40 -0700 Subject: [PATCH 25/25] fmt --- frame/referenda/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/referenda/src/lib.rs b/frame/referenda/src/lib.rs index 551f4649f6acd..d4dbbf8a3c998 100644 --- a/frame/referenda/src/lib.rs +++ b/frame/referenda/src/lib.rs @@ -1301,7 +1301,8 @@ impl, I: 'static> Pallet { /// /// General assertions: /// - /// * [`ReferendumCount`] must always be equal to the number of referenda in [`ReferendumInfoFor`]. + /// * [`ReferendumCount`] must always be equal to the number of referenda in + /// [`ReferendumInfoFor`]. /// * Referendum indices in [`MetadataOf`] must also be stored in [`ReferendumInfoFor`]. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> {