Feedback from @XLC for Referenda Pallet (#10991)

* feedback from @xlc

* english

* fmt

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

substrate/frame/referenda/src/benchmarking.rs

@@ -48,7 +48,7 @@ fn create_referendum<T: Config>() -> (T::AccountId, ReferendumIndex) {
 		RawOrigin::Signed(caller.clone()).into(),
 		RawOrigin::Root.into(),
 		T::Hashing::hash_of(&0),
-		AtOrAfter::After(0u32.into())
+		DispatchTime::After(0u32.into())
 	));
 	let index = ReferendumCount::<T>::get() - 1;
 	(caller, index)
@@ -182,7 +182,7 @@ benchmarks! {
 		RawOrigin::Signed(caller),
 		RawOrigin::Root.into(),
 		T::Hashing::hash_of(&0),
-		AtOrAfter::After(0u32.into())
+		DispatchTime::After(0u32.into())
 	) verify {
 		let index = ReferendumCount::<T>::get().checked_sub(1).unwrap();
 		assert_matches!(ReferendumInfoFor::<T>::get(index), Some(ReferendumInfo::Ongoing(_)));

substrate/frame/referenda/src/lib.rs

@@ -68,8 +68,8 @@ use frame_support::{
 			v2::{Anon as ScheduleAnon, Named as ScheduleNamed},
 			DispatchTime, MaybeHashed,
 		},
-		Currency, Get, LockIdentifier, LockableCurrency, OnUnbalanced, OriginTrait, PollStatus,
-		Polling, ReservableCurrency, VoteTally,
+		Currency, Get, LockIdentifier, OnUnbalanced, OriginTrait, PollStatus, Polling,
+		ReservableCurrency, VoteTally,
 	},
 	BoundedVec,
 };
@@ -86,7 +86,7 @@ pub mod weights;
 use branch::{BeginDecidingBranch, OneFewerDecidingBranch, ServiceBranch};
 pub use pallet::*;
 pub use types::{
-	AtOrAfter, BalanceOf, CallOf, Curve, DecidingStatus, DecidingStatusOf, Deposit, InsertSorted,
+	BalanceOf, CallOf, Curve, DecidingStatus, DecidingStatusOf, Deposit, InsertSorted,
 	NegativeImbalanceOf, PalletsOriginOf, ReferendumIndex, ReferendumInfo, ReferendumInfoOf,
 	ReferendumStatus, ReferendumStatusOf, ScheduleAddressOf, TallyOf, TrackIdOf, TrackInfo,
 	TrackInfoOf, TracksInfo, VotesOf,
@@ -126,9 +126,7 @@ pub mod pallet {
 		type Scheduler: ScheduleAnon<Self::BlockNumber, CallOf<Self>, PalletsOriginOf<Self>, Hash = Self::Hash>
 			+ ScheduleNamed<Self::BlockNumber, CallOf<Self>, PalletsOriginOf<Self>, Hash = Self::Hash>;
 		/// Currency type for this pallet.
-		type Currency: ReservableCurrency<Self::AccountId>
-			+ LockableCurrency<Self::AccountId, Moment = Self::BlockNumber>;
-
+		type Currency: ReservableCurrency<Self::AccountId>;
 		// Origins and unbalances.
 		/// Origin from which any vote may be cancelled.
 		type CancelOrigin: EnsureOrigin<Self::Origin>;
@@ -175,8 +173,6 @@ pub mod pallet {
 	pub type ReferendumCount<T> = StorageValue<_, ReferendumIndex, ValueQuery>;
 
 	/// Information concerning any given referendum.
-	///
-	/// TWOX-NOTE: SAFE as indexes are not under an attacker’s control.
 	#[pallet::storage]
 	pub type ReferendumInfoFor<T: Config> =
 		StorageMap<_, Blake2_128Concat, ReferendumIndex, ReferendumInfoOf<T>>;
@@ -301,7 +297,7 @@ pub mod pallet {
 		/// Referendum is not ongoing.
 		NotOngoing,
 		/// Referendum's decision deposit is already paid.
-		HaveDeposit,
+		HasDeposit,
 		/// The track identifier given was invalid.
 		BadTrack,
 		/// There are already a full complement of referendums in progress for this track.
@@ -338,7 +334,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			proposal_origin: PalletsOriginOf<T>,
 			proposal_hash: T::Hash,
-			enactment_moment: AtOrAfter<T::BlockNumber>,
+			enactment_moment: DispatchTime<T::BlockNumber>,
 		) -> DispatchResult {
 			let who = ensure_signed(origin)?;
 
@@ -385,7 +381,7 @@ pub mod pallet {
 		) -> DispatchResultWithPostInfo {
 			let who = ensure_signed(origin)?;
 			let mut status = Self::ensure_ongoing(index)?;
-			ensure!(status.decision_deposit.is_none(), Error::<T>::HaveDeposit);
+			ensure!(status.decision_deposit.is_none(), Error::<T>::HasDeposit);
 			let track = Self::track(status.track).ok_or(Error::<T>::NoTrack)?;
 			status.decision_deposit =
 				Some(Self::take_deposit(who.clone(), track.decision_deposit)?);
@@ -598,7 +594,7 @@ impl<T: Config> Polling<T::Tally> for Pallet<T> {
 			track: class,
 			origin: frame_support::dispatch::RawOrigin::Root.into(),
 			proposal_hash: <T::Hashing as sp_runtime::traits::Hash>::hash_of(&index),
-			enactment: AtOrAfter::After(Zero::zero()),
+			enactment: DispatchTime::After(Zero::zero()),
 			submitted: now,
 			submission_deposit: Deposit { who: dummy_account_id, amount: Zero::zero() },
 			decision_deposit: None,
@@ -651,7 +647,7 @@ impl<T: Config> Pallet<T> {
 	fn schedule_enactment(
 		index: ReferendumIndex,
 		track: &TrackInfoOf<T>,
-		desired: AtOrAfter<T::BlockNumber>,
+		desired: DispatchTime<T::BlockNumber>,
 		origin: PalletsOriginOf<T>,
 		call_hash: T::Hash,
 	) {

substrate/frame/referenda/src/mock.rs

@@ -306,7 +306,7 @@ pub fn propose_set_balance(who: u64, value: u64, delay: u64) -> DispatchResult {
 		Origin::signed(who),
 		frame_system::RawOrigin::Root.into(),
 		set_balance_proposal_hash(value),
-		AtOrAfter::After(delay),
+		DispatchTime::After(delay),
 	)
 }
 
@@ -434,7 +434,7 @@ impl RefState {
 			Origin::signed(1),
 			frame_support::dispatch::RawOrigin::Root.into(),
 			set_balance_proposal_hash(1),
-			AtOrAfter::At(10),
+			DispatchTime::At(10),
 		));
 		assert_ok!(Referenda::place_decision_deposit(Origin::signed(2), 0));
 		if matches!(self, RefState::Confirming { immediate: true }) {

substrate/frame/referenda/src/tests.rs

@@ -47,7 +47,7 @@ fn basic_happy_path_works() {
 			Origin::signed(1),
 			RawOrigin::Root.into(),
 			set_balance_proposal_hash(1),
-			AtOrAfter::At(10),
+			DispatchTime::At(10),
 		));
 		assert_eq!(Balances::reserved_balance(&1), 2);
 		assert_eq!(ReferendumCount::<Test>::get(), 1);
@@ -178,7 +178,7 @@ fn queueing_works() {
 			Origin::signed(5),
 			RawOrigin::Root.into(),
 			set_balance_proposal_hash(0),
-			AtOrAfter::After(0),
+			DispatchTime::After(0),
 		));
 		assert_ok!(Referenda::place_decision_deposit(Origin::signed(5), 0));
 
@@ -190,7 +190,7 @@ fn queueing_works() {
 				Origin::signed(i),
 				RawOrigin::Root.into(),
 				set_balance_proposal_hash(i),
-				AtOrAfter::After(0),
+				DispatchTime::After(0),
 			));
 			assert_ok!(Referenda::place_decision_deposit(Origin::signed(i), i as u32));
 			// TODO: decision deposit after some initial votes with a non-highest voted coming
@@ -275,7 +275,7 @@ fn auto_timeout_should_happen_with_nothing_but_submit() {
 			Origin::signed(1),
 			RawOrigin::Root.into(),
 			set_balance_proposal_hash(1),
-			AtOrAfter::At(20),
+			DispatchTime::At(20),
 		));
 		run_to(20);
 		assert_matches!(ReferendumInfoFor::<Test>::get(0), Some(ReferendumInfo::Ongoing(..)));
@@ -295,13 +295,13 @@ fn tracks_are_distinguished() {
 			Origin::signed(1),
 			RawOrigin::Root.into(),
 			set_balance_proposal_hash(1),
-			AtOrAfter::At(10),
+			DispatchTime::At(10),
 		));
 		assert_ok!(Referenda::submit(
 			Origin::signed(2),
 			RawOrigin::None.into(),
 			set_balance_proposal_hash(2),
-			AtOrAfter::At(20),
+			DispatchTime::At(20),
 		));
 
 		assert_ok!(Referenda::place_decision_deposit(Origin::signed(3), 0));
@@ -318,7 +318,7 @@ fn tracks_are_distinguished() {
 						track: 0,
 						origin: OriginCaller::system(RawOrigin::Root),
 						proposal_hash: set_balance_proposal_hash(1),
-						enactment: AtOrAfter::At(10),
+						enactment: DispatchTime::At(10),
 						submitted: 1,
 						submission_deposit: Deposit { who: 1, amount: 2 },
 						decision_deposit: Some(Deposit { who: 3, amount: 10 }),
@@ -334,7 +334,7 @@ fn tracks_are_distinguished() {
 						track: 1,
 						origin: OriginCaller::system(RawOrigin::None),
 						proposal_hash: set_balance_proposal_hash(2),
-						enactment: AtOrAfter::At(20),
+						enactment: DispatchTime::At(20),
 						submitted: 1,
 						submission_deposit: Deposit { who: 2, amount: 2 },
 						decision_deposit: Some(Deposit { who: 4, amount: 1 }),
@@ -355,13 +355,18 @@ fn submit_errors_work() {
 		let h = set_balance_proposal_hash(1);
 		// No track for Signed origins.
 		assert_noop!(
-			Referenda::submit(Origin::signed(1), RawOrigin::Signed(2).into(), h, AtOrAfter::At(10),),
+			Referenda::submit(
+				Origin::signed(1),
+				RawOrigin::Signed(2).into(),
+				h,
+				DispatchTime::At(10),
+			),
 			Error::<Test>::NoTrack
 		);
 
 		// No funds for deposit
 		assert_noop!(
-			Referenda::submit(Origin::signed(10), RawOrigin::Root.into(), h, AtOrAfter::At(10),),
+			Referenda::submit(Origin::signed(10), RawOrigin::Root.into(), h, DispatchTime::At(10),),
 			BalancesError::<Test>::InsufficientBalance
 		);
 	});
@@ -378,13 +383,13 @@ fn decision_deposit_errors_work() {
 			Origin::signed(1),
 			RawOrigin::Root.into(),
 			h,
-			AtOrAfter::At(10),
+			DispatchTime::At(10),
 		));
 		let e = BalancesError::<Test>::InsufficientBalance;
 		assert_noop!(Referenda::place_decision_deposit(Origin::signed(10), 0), e);
 
 		assert_ok!(Referenda::place_decision_deposit(Origin::signed(2), 0));
-		let e = Error::<Test>::HaveDeposit;
+		let e = Error::<Test>::HasDeposit;
 		assert_noop!(Referenda::place_decision_deposit(Origin::signed(2), 0), e);
 	});
 }
@@ -400,7 +405,7 @@ fn refund_deposit_works() {
 			Origin::signed(1),
 			RawOrigin::Root.into(),
 			h,
-			AtOrAfter::At(10),
+			DispatchTime::At(10),
 		));
 		let e = Error::<Test>::NoDeposit;
 		assert_noop!(Referenda::refund_decision_deposit(Origin::signed(2), 0), e);
@@ -422,7 +427,7 @@ fn cancel_works() {
 			Origin::signed(1),
 			RawOrigin::Root.into(),
 			h,
-			AtOrAfter::At(10),
+			DispatchTime::At(10),
 		));
 		assert_ok!(Referenda::place_decision_deposit(Origin::signed(2), 0));
 
@@ -441,7 +446,7 @@ fn cancel_errors_works() {
 			Origin::signed(1),
 			RawOrigin::Root.into(),
 			h,
-			AtOrAfter::At(10),
+			DispatchTime::At(10),
 		));
 		assert_ok!(Referenda::place_decision_deposit(Origin::signed(2), 0));
 		assert_noop!(Referenda::cancel(Origin::signed(1), 0), BadOrigin);
@@ -459,7 +464,7 @@ fn kill_works() {
 			Origin::signed(1),
 			RawOrigin::Root.into(),
 			h,
-			AtOrAfter::At(10),
+			DispatchTime::At(10),
 		));
 		assert_ok!(Referenda::place_decision_deposit(Origin::signed(2), 0));
 
@@ -479,7 +484,7 @@ fn kill_errors_works() {
 			Origin::signed(1),
 			RawOrigin::Root.into(),
 			h,
-			AtOrAfter::At(10),
+			DispatchTime::At(10),
 		));
 		assert_ok!(Referenda::place_decision_deposit(Origin::signed(2), 0));
 		assert_noop!(Referenda::kill(Origin::signed(4), 0), BadOrigin);

substrate/frame/referenda/src/types.rs

@@ -185,26 +185,6 @@ pub trait TracksInfo<Balance, Moment> {
 	}
 }
 
-/// Indication of either a specific moment or a delay from a implicitly defined moment.
-#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
-pub enum AtOrAfter<Moment: Parameter> {
-	/// Indiciates that the event should occur at the moment given.
-	At(Moment),
-	/// Indiciates that the event should occur some period of time (defined by the parameter) after
-	/// a prior event. The prior event is defined by the context, but for the purposes of
-	/// referendum proposals, the "prior event" is the passing of the referendum.
-	After(Moment),
-}
-
-impl<Moment: AtLeast32BitUnsigned + Copy + Parameter> AtOrAfter<Moment> {
-	pub fn evaluate(&self, since: Moment) -> Moment {
-		match &self {
-			Self::At(m) => *m,
-			Self::After(m) => m.saturating_add(since),
-		}
-	}
-}
-
 /// Info regarding an ongoing referendum.
 #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
 pub struct ReferendumStatus<
@@ -224,7 +204,7 @@ pub struct ReferendumStatus<
 	/// The hash of the proposal up for referendum.
 	pub(crate) proposal_hash: Hash,
 	/// The time the proposal should be scheduled for enactment.
-	pub(crate) enactment: AtOrAfter<Moment>,
+	pub(crate) enactment: DispatchTime<Moment>,
 	/// The time of submission. Once `UndecidingTimeout` passes, it may be closed by anyone if it
 	/// `deciding` is `None`.
 	pub(crate) submitted: Moment,

substrate/frame/support/src/traits/schedule.rs

@@ -19,7 +19,7 @@
 
 use codec::{Codec, Decode, Encode, EncodeLike, MaxEncodedLen};
 use scale_info::TypeInfo;
-use sp_runtime::{DispatchError, RuntimeDebug};
+use sp_runtime::{traits::Saturating, DispatchError, RuntimeDebug};
 use sp_std::{fmt::Debug, prelude::*, result::Result};
 
 /// Information relating to the period of a scheduled task. First item is the length of the
@@ -32,7 +32,7 @@ pub type Period<BlockNumber> = (BlockNumber, u32);
 pub type Priority = u8;
 
 /// The dispatch time of a scheduled task.
-#[derive(Encode, Decode, Copy, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo)]
+#[derive(Encode, Decode, Copy, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
 pub enum DispatchTime<BlockNumber> {
 	/// At specified block.
 	At(BlockNumber),
@@ -40,6 +40,15 @@ pub enum DispatchTime<BlockNumber> {
 	After(BlockNumber),
 }
 
+impl<BlockNumber: Saturating + Copy> DispatchTime<BlockNumber> {
+	pub fn evaluate(&self, since: BlockNumber) -> BlockNumber {
+		match &self {
+			Self::At(m) => *m,
+			Self::After(m) => m.saturating_add(since),
+		}
+	}
+}
+
 /// The highest priority. We invert the value so that normal sorting will place the highest
 /// priority at the beginning of the list.
 pub const HIGHEST_PRIORITY: Priority = 0;