From 3a275ea5394174f611c273d3e23f2ef51ef77633 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Tue, 26 Mar 2024 15:52:54 -0500 Subject: [PATCH] change(pallet-referenda): treat `Finalizing` state as a logical (intrinsic) period within `Ongoing` state. --- substrate/frame/referenda/src/lib.rs | 102 +++++++++-------- substrate/frame/referenda/src/migration.rs | 126 --------------------- substrate/frame/referenda/src/mock.rs | 4 - substrate/frame/referenda/src/types.rs | 39 +++---- 4 files changed, 71 insertions(+), 200 deletions(-) diff --git a/substrate/frame/referenda/src/lib.rs b/substrate/frame/referenda/src/lib.rs index ae3d2c16cd380..8c5c04b83c5b7 100644 --- a/substrate/frame/referenda/src/lib.rs +++ b/substrate/frame/referenda/src/lib.rs @@ -628,22 +628,10 @@ pub mod pallet { ensure_root(origin)?; let now = frame_system::Pallet::::block_number(); - let info = ReferendumInfoFor::::get(index); - - let (info, dirty, branch) = match info { - Some(ReferendumInfo::Ongoing(mut status)) => { - // This is our wake-up, so we can disregard the alarm. - status.alarm = None; - Self::service_referendum(now, index, status) - }, - Some(ReferendumInfo::Finalizing(mut status)) => { - status.alarm = None; - Self::service_auction(now, index, status) - }, - Some(info) => (info, false, ServiceBranch::Fail), - _ => Err(Error::::NotOngoing)?, - }; - + let mut status = Self::ensure_ongoing(index)?; + // This is our wake-up, so we can disregard the alarm. + status.alarm = None; + let (info, dirty, branch) = Self::service_referendum(now, index, status); if dirty { ReferendumInfoFor::::insert(index, info); } @@ -769,8 +757,17 @@ impl, I: 'static> Polling for Pallet { ) -> R { match ReferendumInfoFor::::get(index) { Some(ReferendumInfo::Ongoing(mut status)) => { - let result = f(PollStatus::Ongoing(&mut status.tally, status.track)); let now = frame_system::Pallet::::block_number(); + let is_finalizing = status + .clone() + .deciding + .map(|d| d.confirming.map(|x| now >= x).unwrap_or(false)) + .unwrap(); + if is_finalizing { + return f(PollStatus::None); + } + + let result = f(PollStatus::Ongoing(&mut status.tally, status.track)); Self::ensure_alarm_at(&mut status, index, now + One::one()); ReferendumInfoFor::::insert(index, ReferendumInfo::Ongoing(status)); result @@ -789,8 +786,17 @@ impl, I: 'static> Polling for Pallet { ) -> Result { match ReferendumInfoFor::::get(index) { Some(ReferendumInfo::Ongoing(mut status)) => { - let result = f(PollStatus::Ongoing(&mut status.tally, status.track))?; let now = frame_system::Pallet::::block_number(); + let is_finalizing = status + .clone() + .deciding + .map(|d| d.confirming.map(|x| now >= x).unwrap_or(false)) + .unwrap(); + if is_finalizing { + return f(PollStatus::None); + } + + let result = f(PollStatus::Ongoing(&mut status.tally, status.track))?; Self::ensure_alarm_at(&mut status, index, now + One::one()); ReferendumInfoFor::::insert(index, ReferendumInfo::Ongoing(status)); Ok(result) @@ -867,7 +873,16 @@ impl, I: 'static> Pallet { index: ReferendumIndex, ) -> Result, DispatchError> { match ReferendumInfoFor::::get(index) { - Some(ReferendumInfo::Ongoing(status)) => Ok(status), + Some(ReferendumInfo::Ongoing(status)) => { + match status.clone().deciding.map(|d| d.confirming) { + Some(Some(confirming)) + if frame_system::Pallet::::block_number() >= confirming => + { + Err(Error::::NotOngoing.into()) + }, + _ => Ok(status), + } + }, _ => Err(Error::::NotOngoing.into()), } } @@ -928,8 +943,8 @@ impl, I: 'static> Pallet { let alarm_interval = T::AlarmInterval::get().max(One::one()); // Alarm must go off no earlier than `when`. // This rounds `when` upwards to the next multiple of `alarm_interval`. - let when = (when.saturating_add(alarm_interval.saturating_sub(One::one())) / - alarm_interval) + let when = (when.saturating_add(alarm_interval.saturating_sub(One::one())) + / alarm_interval) .saturating_mul(alarm_interval); let result = T::Scheduler::schedule( DispatchTime::At(when), @@ -1042,7 +1057,7 @@ impl, I: 'static> Pallet { Ok(c) => c, Err(_) => { debug_assert!(false, "Unable to create a bounded call from `one_fewer_deciding`??",); - return + return; }, }; Self::set_alarm(call, next_block); @@ -1069,7 +1084,7 @@ impl, I: 'static> Pallet { false, "Unable to create a bounded call from `nudge_referendum`??", ); - return false + return false; }, }; status.alarm = Self::set_alarm(call, alarm); @@ -1173,7 +1188,7 @@ impl, I: 'static> Pallet { ), true, ServiceBranch::TimedOut, - ) + ); } }, Some(deciding) => { @@ -1185,6 +1200,7 @@ impl, I: 'static> Pallet { &track.min_approval, status.track, ); + branch = if is_passing { match deciding.confirming { Some(t) if now >= t => { @@ -1195,18 +1211,14 @@ impl, I: 'static> Pallet { // Sent to Auction only after confirm period finishes. Whether it will pass // or not depends on the status history when confirming, and a random point // in time within confirm period. + PassingStatusInConfirmPeriod::::insert( index, now.saturating_less_one(), is_passing, ); - Self::ensure_alarm_at(&mut status, index, now.saturating_plus_one()); - - return ( - ReferendumInfo::Finalizing(status), - true, - ServiceBranch::ContinueConfirming, - ) + log::debug!("going to auction at {now}"); + return Self::service_auction(now, index, status); }, Some(_) => { // We don't care if failing within confirm period. @@ -1244,13 +1256,8 @@ impl, I: 'static> Pallet { now.saturating_less_one(), is_passing, ); - Self::ensure_alarm_at(&mut status, index, now.saturating_plus_one()); - - return ( - ReferendumInfo::Finalizing(status), - true, - ServiceBranch::ContinueConfirming, - ) + log::debug!("going to auction at {now}"); + return Self::service_auction(now, index, status); }, Some(_) => { // We don't care if failing within confirm period. @@ -1308,7 +1315,7 @@ impl, I: 'static> Pallet { // Note: it'd be weird to come up to this point and yet not have a track let track = match Self::track(status.clone().track) { Some(x) => x, - None => return (ReferendumInfo::Finalizing(status), false, ServiceBranch::Fail), + None => return (ReferendumInfo::Ongoing(status), false, ServiceBranch::Fail), }; let confirming_until = status @@ -1324,8 +1331,11 @@ impl, I: 'static> Pallet { // Do not use random until made sure random seed is not known before confirm ends if known_since <= confirming_until { + log::debug!("auction: better luck next block"); Self::ensure_alarm_at(&mut status, index, now.saturating_plus_one()); - return (ReferendumInfo::Finalizing(status), false, ServiceBranch::ContinueConfirming); + return (ReferendumInfo::Ongoing(status), false, ServiceBranch::ContinueConfirming); + } else { + Self::ensure_no_alarm(&mut status); } let raw_offset_block_number = >::decode(&mut raw_offset.as_ref()) @@ -1454,8 +1464,8 @@ impl, I: 'static> Pallet { id: TrackIdOf, ) -> bool { let x = Perbill::from_rational(elapsed.min(period), period); - support_needed.passing(x, tally.support(id)) && - approval_needed.passing(x, tally.approval(id)) + support_needed.passing(x, tally.support(id)) + && approval_needed.passing(x, tally.approval(id)) } /// Clear metadata if exist for a given referendum index. @@ -1477,8 +1487,8 @@ impl, I: 'static> Pallet { #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> { ensure!( - ReferendumCount::::get() as usize == - ReferendumInfoFor::::iter_keys().count(), + ReferendumCount::::get() as usize + == ReferendumInfoFor::::iter_keys().count(), "Number of referenda in `ReferendumInfoFor` is different than `ReferendumCount`" ); @@ -1516,8 +1526,8 @@ impl, I: 'static> Pallet { if let Some(deciding) = status.deciding { ensure!( - deciding.since < - deciding.confirming.unwrap_or(BlockNumberFor::::max_value()), + deciding.since + < deciding.confirming.unwrap_or(BlockNumberFor::::max_value()), "Deciding status cannot begin before confirming stage." ) } diff --git a/substrate/frame/referenda/src/migration.rs b/substrate/frame/referenda/src/migration.rs index 31b05b118afff..631eb7340e567 100644 --- a/substrate/frame/referenda/src/migration.rs +++ b/substrate/frame/referenda/src/migration.rs @@ -90,57 +90,6 @@ pub mod v0 { pub mod v1 { use super::*; - pub type ReferendumInfoOf = ReferendumInfo< - TrackIdOf, - PalletsOriginOf, - frame_system::pallet_prelude::BlockNumberFor, - BoundedCallOf, - BalanceOf, - TallyOf, - ::AccountId, - ScheduleAddressOf, - >; - - #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)] - pub enum ReferendumInfo< - TrackId: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, - RuntimeOrigin: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, - Moment: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone + EncodeLike, - Call: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, - Balance: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, - Tally: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, - AccountId: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, - ScheduleAddress: Eq + PartialEq + Debug + Encode + Decode + TypeInfo + Clone, - > { - /// Referendum has been submitted and is being voted on. - Ongoing( - ReferendumStatus< - TrackId, - RuntimeOrigin, - Moment, - Call, - Balance, - Tally, - AccountId, - ScheduleAddress, - >, - ), - /// Referendum finished with approval. Submission deposit is held. - Approved(Moment, Option>, Option>), - /// Referendum finished with rejection. Submission deposit is held. - Rejected(Moment, Option>, Option>), - /// Referendum finished with cancellation. Submission deposit is held. - Cancelled(Moment, Option>, Option>), - /// Referendum finished and was never decided. Submission deposit is held. - TimedOut(Moment, Option>, Option>), - /// Referendum finished with a kill. - Killed(Moment), - } - - #[storage_alias] - pub type ReferendumInfoFor, I: 'static> = - StorageMap, Blake2_128Concat, ReferendumIndex, ReferendumInfoOf>; - /// The log target. const TARGET: &'static str = "runtime::referenda::migration::v1"; @@ -212,81 +161,6 @@ pub mod v1 { } } -pub mod v2 { - use super::*; - - /// The log target. - const TARGET: &'static str = "runtime::referenda::migration::v2"; - - /// Transforms a submission deposit of ReferendumInfo(Approved|Rejected|Cancelled|TimedOut) to - /// optional value, making it refundable. - pub struct MigrateV1ToV2(PhantomData<(T, I)>); - impl, I: 'static> OnRuntimeUpgrade for MigrateV1ToV2 { - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - let referendum_count = v1::ReferendumInfoFor::::iter().count(); - log::info!( - target: TARGET, - "pre-upgrade state contains '{}' referendums.", - referendum_count - ); - Ok((referendum_count as u32).encode()) - } - - fn on_runtime_upgrade() -> Weight { - let in_code_version = Pallet::::in_code_storage_version(); - let on_chain_version = Pallet::::on_chain_storage_version(); - let mut weight = T::DbWeight::get().reads(1); - log::info!( - target: TARGET, - "running migration with in-code storage version {:?} / onchain {:?}.", - in_code_version, - on_chain_version - ); - if on_chain_version != 1 { - log::warn!(target: TARGET, "skipping migration from v1 to v2."); - return weight; - } - v1::ReferendumInfoFor::::iter().for_each(|(key, value)| { - let maybe_new_value = match value { - v1::ReferendumInfo::Ongoing(s) => Some(ReferendumInfo::Ongoing(s)), - v1::ReferendumInfo::Killed(e) => Some(ReferendumInfo::Killed(e)), - v1::ReferendumInfo::Approved(e, s, d) => - Some(ReferendumInfo::Approved(e, s, d)), - v1::ReferendumInfo::Rejected(e, s, d) => - Some(ReferendumInfo::Rejected(e, s, d)), - v1::ReferendumInfo::Cancelled(e, s, d) => - Some(ReferendumInfo::Cancelled(e, s, d)), - v1::ReferendumInfo::TimedOut(e, s, d) => - Some(ReferendumInfo::TimedOut(e, s, d)), - }; - if let Some(new_value) = maybe_new_value { - weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); - log::info!(target: TARGET, "migrating referendum #{:?}", &key); - ReferendumInfoFor::::insert(key, new_value); - } else { - weight.saturating_accrue(T::DbWeight::get().reads(1)); - } - }); - StorageVersion::new(2).put::>(); - weight.saturating_accrue(T::DbWeight::get().writes(1)); - weight - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(state: Vec) -> Result<(), TryRuntimeError> { - let on_chain_version = Pallet::::on_chain_storage_version(); - ensure!(on_chain_version == 2, "must upgrade from version 1 to 2."); - let pre_referendum_count: u32 = Decode::decode(&mut &state[..]) - .expect("failed to decode the state from pre-upgrade."); - let post_referendum_count = ReferendumInfoFor::::iter().count() as u32; - ensure!(post_referendum_count == pre_referendum_count, "must migrate all referendums."); - log::info!(target: TARGET, "migrated all referendums."); - Ok(()) - } - } -} - #[cfg(test)] pub mod test { use super::*; diff --git a/substrate/frame/referenda/src/mock.rs b/substrate/frame/referenda/src/mock.rs index ee58dce4f15ed..35068fb61a2b7 100644 --- a/substrate/frame/referenda/src/mock.rs +++ b/substrate/frame/referenda/src/mock.rs @@ -420,10 +420,6 @@ pub fn deciding_and_failing_since(i: ReferendumIndex) -> u64 { deciding: Some(DecidingStatus { since, confirming: None, .. }), .. }) => since, - ReferendumInfo::Finalizing(ReferendumStatus { - deciding: Some(DecidingStatus { since, confirming: None, .. }), - .. - }) => since, _ => panic!("Not deciding"), } } diff --git a/substrate/frame/referenda/src/types.rs b/substrate/frame/referenda/src/types.rs index dec16cfcea57b..9420d4c1b08b0 100644 --- a/substrate/frame/referenda/src/types.rs +++ b/substrate/frame/referenda/src/types.rs @@ -237,19 +237,6 @@ pub enum ReferendumInfo< ScheduleAddress, >, ), - /// Referendum is waiting to be finished via candle auction resolution. - Finalizing( - ReferendumStatus< - TrackId, - RuntimeOrigin, - Moment, - Call, - Balance, - Tally, - AccountId, - ScheduleAddress, - >, - ), /// Referendum finished with approval. Submission deposit is held. Approved(Moment, Option>, Option>), /// Referendum finished with rejection. Submission deposit is held. @@ -278,9 +265,9 @@ impl< pub fn take_decision_deposit(&mut self) -> Result>, ()> { use ReferendumInfo::*; match self { - Ongoing(x) | Finalizing(x) if x.decision_deposit.is_none() => Ok(None), + Ongoing(x) if x.decision_deposit.is_none() => Ok(None), // Cannot refund deposit if Ongoing as this breaks assumptions. - Ongoing(_) | Finalizing(_) => Err(()), + Ongoing(_) => Err(()), Approved(_, _, d) | Rejected(_, _, d) | TimedOut(_, _, d) | Cancelled(_, _, d) => { Ok(d.take()) }, @@ -297,7 +284,7 @@ impl< // Can only refund deposit if it's appoved or cancelled. Approved(_, s, _) | Cancelled(_, s, _) => Ok(s.take()), // Cannot refund deposit if Ongoing as this breaks assumptions. - Ongoing(..) | Rejected(..) | TimedOut(..) | Finalizing(_) | Killed(..) => Err(()), + Ongoing(..) | Rejected(..) | TimedOut(..) | Killed(..) => Err(()), } } } @@ -455,10 +442,12 @@ impl Curve { /// Determine the `y` value for the given `x` value. pub fn threshold(&self, x: Perbill) -> Perbill { match self { - Self::LinearDecreasing { length, floor, ceil } => - *ceil - (x.min(*length).saturating_div(*length, Down) * (*ceil - *floor)), - Self::SteppedDecreasing { begin, end, step, period } => - (*begin - (step.int_mul(x.int_div(*period))).min(*begin)).max(*end), + Self::LinearDecreasing { length, floor, ceil } => { + *ceil - (x.min(*length).saturating_div(*length, Down) * (*ceil - *floor)) + }, + Self::SteppedDecreasing { begin, end, step, period } => { + (*begin - (step.int_mul(x.int_div(*period))).min(*begin)).max(*end) + }, Self::Reciprocal { factor, x_offset, y_offset } => factor .checked_rounding_div(FixedI64::from(x) + *x_offset, Low) .map(|yp| (yp + *y_offset).into_clamped_perthing()) @@ -497,20 +486,22 @@ impl Curve { /// ``` pub fn delay(&self, y: Perbill) -> Perbill { match self { - Self::LinearDecreasing { length, floor, ceil } => + Self::LinearDecreasing { length, floor, ceil } => { if y < *floor { Perbill::one() } else if y > *ceil { Perbill::zero() } else { (*ceil - y).saturating_div(*ceil - *floor, Up).saturating_mul(*length) - }, - Self::SteppedDecreasing { begin, end, step, period } => + } + }, + Self::SteppedDecreasing { begin, end, step, period } => { if y < *end { Perbill::one() } else { period.int_mul((*begin - y.min(*begin) + step.less_epsilon()).int_div(*step)) - }, + } + }, Self::Reciprocal { factor, x_offset, y_offset } => { let y = FixedI64::from(y); let maybe_term = factor.checked_rounding_div(y - *y_offset, High);