From 8c339c9d74bba917fbe49a755facbf0aeb5c402c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Tue, 2 Apr 2024 10:05:21 -0500 Subject: [PATCH] change(pallet-referenda): apply requested changes --- substrate/frame/referenda/src/lib.rs | 92 +++++++++++++--------------- 1 file changed, 42 insertions(+), 50 deletions(-) diff --git a/substrate/frame/referenda/src/lib.rs b/substrate/frame/referenda/src/lib.rs index 054d3775a668..ad2dcb349e69 100644 --- a/substrate/frame/referenda/src/lib.rs +++ b/substrate/frame/referenda/src/lib.rs @@ -627,11 +627,7 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { ensure_root(origin)?; let now = frame_system::Pallet::::block_number(); - - let mut status = match ReferendumInfoFor::::get(index) { - Some(ReferendumInfo::Ongoing(status)) => Ok(status), - _ => 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); @@ -744,6 +740,19 @@ pub mod pallet { } } +#[inline] +fn is_finalizing, I: 'static>(status: &ReferendumStatusOf) -> bool { + status + .clone() + .deciding + .map(|d| { + d.confirming + .map(|x| frame_system::Pallet::::block_number() >= x) + .unwrap_or(false) + }) + .unwrap_or(false) +} + impl, I: 'static> Polling for Pallet { type Index = ReferendumIndex; type Votes = VotesOf; @@ -760,17 +769,11 @@ impl, I: 'static> Polling for Pallet { ) -> R { match ReferendumInfoFor::::get(index) { Some(ReferendumInfo::Ongoing(mut status)) => { - 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_or(false); - if is_finalizing { + if is_finalizing(&status) { return f(PollStatus::None); } - let result = f(PollStatus::Ongoing(&mut status.tally, status.track)); + let now = frame_system::Pallet::::block_number(); Self::ensure_alarm_at(&mut status, index, now + One::one()); ReferendumInfoFor::::insert(index, ReferendumInfo::Ongoing(status)); result @@ -789,17 +792,11 @@ impl, I: 'static> Polling for Pallet { ) -> Result { match ReferendumInfoFor::::get(index) { Some(ReferendumInfo::Ongoing(mut status)) => { - 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_or(false); - if is_finalizing { + if is_finalizing(&status) { return f(PollStatus::None); } - let result = f(PollStatus::Ongoing(&mut status.tally, status.track))?; + let now = frame_system::Pallet::::block_number(); Self::ensure_alarm_at(&mut status, index, now + One::one()); ReferendumInfoFor::::insert(index, ReferendumInfo::Ongoing(status)); Ok(result) @@ -876,16 +873,7 @@ impl, I: 'static> Pallet { index: ReferendumIndex, ) -> Result, DispatchError> { match ReferendumInfoFor::::get(index) { - 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), - } - }, + Some(ReferendumInfo::Ongoing(status)) => Ok(status), _ => Err(Error::::NotOngoing.into()), } } @@ -946,8 +934,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), @@ -1060,7 +1048,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); @@ -1087,7 +1075,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); @@ -1201,7 +1189,7 @@ impl, I: 'static> Pallet { ), true, ServiceBranch::TimedOut, - ); + ) } }, Some(deciding) => { @@ -1230,7 +1218,7 @@ impl, I: 'static> Pallet { now.saturating_less_one(), is_passing, ); - return Self::service_auction(now, index, status); + return Self::confirm_by_candle(now, index, status); }, Some(_) => { // We don't care if failing within confirm period. @@ -1272,7 +1260,7 @@ impl, I: 'static> Pallet { now.saturating_less_one(), is_passing, ); - return Self::service_auction(now, index, status); + return Self::confirm_by_candle(now, index, status); }, Some(_) => { // We don't care if failing within confirm period. @@ -1321,12 +1309,13 @@ impl, I: 'static> Pallet { (ReferendumInfo::Ongoing(status), dirty_alarm || dirty, branch) } - /// "Candle" auction to decide the winning status of confirm period. + /// Find "candle" moment within the confirm period to decide whether the referendum is + /// confirmed or not. /// /// The "candle": passing or failing of a referendum is ultimately decided as a candle auction /// where, given a random point in time (defined as `t`), the definitive status of the the /// referendum is decided by the last status registered before `t`. - fn service_auction( + fn confirm_by_candle( now: BlockNumberFor, index: ReferendumIndex, mut status: ReferendumStatusOf, @@ -1340,9 +1329,9 @@ impl, I: 'static> Pallet { let confirming_until = status .clone() .deciding - .expect("having passed ongoing, we should have times for decision; qed") + .expect("called after having deciding, we have deciding.since time; qed") .confirming - .expect("having finished confirming, we should have a confirming_until time; qed"); + .expect("called after finished confirming, we have a confirming_until time; qed"); let confirming_since = confirming_until.saturating_sub(track.confirm_period); @@ -1370,9 +1359,12 @@ impl, I: 'static> Pallet { let statuses = PassingStatusInConfirmPeriod::::get(index); let statuses = statuses.range((Included(&confirming_since), Included(&candle_block_number))); - let (_, winning_status) = statuses.last().unwrap_or((&now, &true)); + let (_, is_confirmed) = statuses.last().unwrap_or((&now, &true)); + + // Cleanup passing status + PassingStatusInConfirmPeriod::::remove(index); - if *winning_status { + if *is_confirmed { // Passed! Self::ensure_no_alarm(&mut status); Self::note_one_fewer_deciding(status.track); @@ -1478,8 +1470,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. @@ -1501,8 +1493,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`" ); @@ -1540,8 +1532,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." ) }