pallet-lottery: add generate_storage_info (#10594)

* pallet-lottery: add generate_storage_info Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * pallet-lottery: test call_to_indices with TooManyCalls Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * review: move try_push above transfer Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * pallet-lottery: test stop_repeat Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * pallet-lottery: test do_buy_ticket as white-box Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * pallet-lottery: use BoundedVec in bechmarks Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * pallet-lottery: fix zero div panic Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * review: extend buy_ticket tests Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * review: test buy_ticket AlreadyParticipating Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * fmt Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * review: use /// comments on private functions Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * review: use with_bounded_capacity Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
2026-06-13 02:21:14 +00:00 · 2022-01-11 15:22:27 +01:00
parent 5e4035dcaa
commit b630f1b0e6
3 changed files with 235 additions and 32 deletions
@@ -22,7 +22,10 @@
 use super::*;

 use frame_benchmarking::{account, benchmarks, whitelisted_caller};
-use frame_support::traits::{EnsureOrigin, OnInitialize};
+use frame_support::{
+	storage::bounded_vec::BoundedVec,
+	traits::{EnsureOrigin, OnInitialize},
+};
 use frame_system::RawOrigin;
 use sp_runtime::traits::{Bounded, Zero};

@@ -55,12 +58,12 @@ benchmarks! {
 		let set_code_index: CallIndex = Lottery::<T>::call_to_index(
 			&frame_system::Call::<T>::set_code{ code: vec![] }.into()
 		)?;
-		let already_called: (u32, Vec<CallIndex>) = (
+		let already_called: (u32, BoundedVec<CallIndex, T::MaxCalls>) = (
 			LotteryIndex::<T>::get(),
-			vec![
+			BoundedVec::<CallIndex, T::MaxCalls>::try_from(vec![
 				set_code_index;
 				T::MaxCalls::get().saturating_sub(1) as usize
-			],
+			]).unwrap(),
 		);
 		Participants::<T>::insert(&caller, already_called);

@@ -28,7 +28,7 @@
 //! users to make those calls on your network. An example of how this could be
 //! used is to set validator nominations as a valid lottery call. If the lottery
 //! is set to repeat every month, then users would be encouraged to re-nominate
-//! validators every month. A user can ony purchase one ticket per valid call
+//! validators every month. A user can only purchase one ticket per valid call
 //! per lottery.
 //!
 //! This pallet can be configured to use dynamically set calls or statically set
@@ -58,6 +58,8 @@ use codec::{Decode, Encode};
 use frame_support::{
 	dispatch::{DispatchResult, Dispatchable, GetDispatchInfo},
 	ensure,
+	pallet_prelude::MaxEncodedLen,
+	storage::bounded_vec::BoundedVec,
 	traits::{Currency, ExistenceRequirement::KeepAlive, Get, Randomness, ReservableCurrency},
 	PalletId, RuntimeDebug,
 };
@@ -76,7 +78,9 @@ type BalanceOf<T> =
 // We use this to uniquely match someone's incoming call with the calls configured for the lottery.
 type CallIndex = (u8, u8);

-#[derive(Encode, Decode, Default, Eq, PartialEq, RuntimeDebug, scale_info::TypeInfo)]
+#[derive(
+	Encode, Decode, Default, Eq, PartialEq, RuntimeDebug, scale_info::TypeInfo, MaxEncodedLen,
+)]
 pub struct LotteryConfig<BlockNumber, Balance> {
 	/// Price per entry.
 	price: Balance,
@@ -120,6 +124,7 @@ pub mod pallet {

 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	#[pallet::generate_storage_info]
 	pub struct Pallet<T>(_);

 	/// The pallet's config trait.
@@ -209,8 +214,13 @@ pub mod pallet {

 	/// Users who have purchased a ticket. (Lottery Index, Tickets Purchased)
 	#[pallet::storage]
-	pub(crate) type Participants<T: Config> =
-		StorageMap<_, Twox64Concat, T::AccountId, (u32, Vec<CallIndex>), ValueQuery>;
+	pub(crate) type Participants<T: Config> = StorageMap<
+		_,
+		Twox64Concat,
+		T::AccountId,
+		(u32, BoundedVec<CallIndex, T::MaxCalls>),
+		ValueQuery,
+	>;

 	/// Total number of tickets sold.
 	#[pallet::storage]
@@ -226,7 +236,8 @@ pub mod pallet {
 	/// The calls stored in this pallet to be used in an active lottery if configured
 	/// by `Config::ValidateCall`.
 	#[pallet::storage]
-	pub(crate) type CallIndices<T> = StorageValue<_, Vec<CallIndex>, ValueQuery>;
+	pub(crate) type CallIndices<T: Config> =
+		StorageValue<_, BoundedVec<CallIndex, T::MaxCalls>, ValueQuery>;

 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
@@ -237,10 +248,8 @@ pub mod pallet {
 						config.start.saturating_add(config.length).saturating_add(config.delay);
 					if payout_block <= n {
 						let (lottery_account, lottery_balance) = Self::pot();
-						let ticket_count = TicketsCount::<T>::get();

-						let winning_number = Self::choose_winner(ticket_count);
-						let winner = Tickets::<T>::get(winning_number).unwrap_or(lottery_account);
+						let winner = Self::choose_account().unwrap_or(lottery_account);
 						// Not much we can do if this fails...
 						let res = T::Currency::transfer(
 							&Self::account_id(),
@@ -385,7 +394,7 @@ impl<T: Config> Pallet<T> {
 	}

 	/// Return the pot account and amount of money in the pot.
-	// The existential deposit is not part of the pot so lottery account never gets deleted.
+	/// The existential deposit is not part of the pot so lottery account never gets deleted.
 	fn pot() -> (T::AccountId, BalanceOf<T>) {
 		let account_id = Self::account_id();
 		let balance =
@@ -394,17 +403,19 @@ impl<T: Config> Pallet<T> {
 		(account_id, balance)
 	}

-	// Converts a vector of calls into a vector of call indices.
-	fn calls_to_indices(calls: &[<T as Config>::Call]) -> Result<Vec<CallIndex>, DispatchError> {
-		let mut indices = Vec::with_capacity(calls.len());
+	/// Converts a vector of calls into a vector of call indices.
+	fn calls_to_indices(
+		calls: &[<T as Config>::Call],
+	) -> Result<BoundedVec<CallIndex, T::MaxCalls>, DispatchError> {
+		let mut indices = BoundedVec::<CallIndex, T::MaxCalls>::with_bounded_capacity(calls.len());
 		for c in calls.iter() {
 			let index = Self::call_to_index(c)?;
-			indices.push(index)
+			indices.try_push(index).map_err(|_| Error::<T>::TooManyCalls)?;
 		}
 		Ok(indices)
 	}

-	// Convert a call to it's call index by encoding the call and taking the first two bytes.
+	/// Convert a call to it's call index by encoding the call and taking the first two bytes.
 	fn call_to_index(call: &<T as Config>::Call) -> Result<CallIndex, DispatchError> {
 		let encoded_call = call.encode();
 		if encoded_call.len() < 2 {
@@ -413,7 +424,7 @@ impl<T: Config> Pallet<T> {
 		return Ok((encoded_call[0], encoded_call[1]))
 	}

-	// Logic for buying a ticket.
+	/// Logic for buying a ticket.
 	fn do_buy_ticket(caller: &T::AccountId, call: &<T as Config>::Call) -> DispatchResult {
 		// Check the call is valid lottery
 		let config = Lottery::<T>::get().ok_or(Error::<T>::NotConfigured)?;
@@ -433,7 +444,7 @@ impl<T: Config> Pallet<T> {
 				let index = LotteryIndex::<T>::get();
 				// If lottery index doesn't match, then reset participating calls and index.
 				if *lottery_index != index {
-					*participating_calls = Vec::new();
+					*participating_calls = Default::default();
 					*lottery_index = index;
 				} else {
 					// Check that user is not already participating under this call.
@@ -442,12 +453,12 @@ impl<T: Config> Pallet<T> {
 						Error::<T>::AlreadyParticipating
 					);
 				}
+				participating_calls.try_push(call_index).map_err(|_| Error::<T>::TooManyCalls)?;
 				// Check user has enough funds and send it to the Lottery account.
 				T::Currency::transfer(caller, &Self::account_id(), config.price, KeepAlive)?;
 				// Create a new ticket.
 				TicketsCount::<T>::put(new_ticket_count);
 				Tickets::<T>::insert(ticket_count, caller.clone());
-				participating_calls.push(call_index);
 				Ok(())
 			},
 		)?;
@@ -457,8 +468,22 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}

-	// Randomly choose a winner from among the total number of participants.
-	fn choose_winner(total: u32) -> u32 {
+	/// Randomly choose a winning ticket and return the account that purchased it.
+	/// The more tickets an account bought, the higher are its chances of winning.
+	/// Returns `None` if there is no winner.
+	fn choose_account() -> Option<T::AccountId> {
+		match Self::choose_ticket(TicketsCount::<T>::get()) {
+			None => None,
+			Some(ticket) => Tickets::<T>::get(ticket),
+		}
+	}
+
+	/// Randomly choose a winning ticket from among the total number of tickets.
+	/// Returns `None` if there are no tickets.
+	fn choose_ticket(total: u32) -> Option<u32> {
+		if total == 0 {
+			return None
+		}
 		let mut random_number = Self::generate_random_number(0);

 		// Best effort attempt to remove bias from modulus operator.
@@ -470,15 +495,15 @@ impl<T: Config> Pallet<T> {
 			random_number = Self::generate_random_number(i);
 		}

-		random_number % total
+		Some(random_number % total)
 	}

-	// Generate a random number from a given seed.
-	// Note that there is potential bias introduced by using modulus operator.
-	// You should call this function with different seed values until the random
-	// number lies within `u32::MAX - u32::MAX % n`.
-	// TODO: deal with randomness freshness
-	// https://github.com/paritytech/substrate/issues/8311
+	/// Generate a random number from a given seed.
+	/// Note that there is potential bias introduced by using modulus operator.
+	/// You should call this function with different seed values until the random
+	/// number lies within `u32::MAX - u32::MAX % n`.
+	/// TODO: deal with randomness freshness
+	/// https://github.com/paritytech/substrate/issues/8311
 	fn generate_random_number(seed: u32) -> u32 {
 		let (random_seed, _) = T::Randomness::random(&(T::PalletId::get(), seed).encode());
 		let random_number = <u32>::decode(&mut random_seed.as_ref())
@@ -18,7 +18,7 @@
 //! Tests for the module.

 use super::*;
-use frame_support::{assert_noop, assert_ok};
+use frame_support::{assert_noop, assert_ok, assert_storage_noop};
 use mock::{
 	new_test_ext, run_to_block, Balances, BalancesCall, Call, Lottery, Origin, SystemCall, Test,
 };
@@ -30,7 +30,7 @@ fn initial_state() {
 	new_test_ext().execute_with(|| {
 		assert_eq!(Balances::free_balance(Lottery::account_id()), 0);
 		assert!(crate::Lottery::<Test>::get().is_none());
-		assert_eq!(Participants::<Test>::get(&1), (0, vec![]));
+		assert_eq!(Participants::<Test>::get(&1), (0, Default::default()));
 		assert_eq!(TicketsCount::<Test>::get(), 0);
 		assert!(Tickets::<Test>::get(0).is_none());
 	});
@@ -90,6 +90,37 @@ fn basic_end_to_end_works() {
 	});
 }

+/// Only the manager can stop the Lottery from repeating via `stop_repeat`.
+#[test]
+fn stop_repeat_works() {
+	new_test_ext().execute_with(|| {
+		let price = 10;
+		let length = 20;
+		let delay = 5;
+
+		// Set no calls for the lottery.
+		assert_ok!(Lottery::set_calls(Origin::root(), vec![]));
+		// Start lottery, it repeats.
+		assert_ok!(Lottery::start_lottery(Origin::root(), price, length, delay, true));
+
+		// Non-manager fails to `stop_repeat`.
+		assert_noop!(Lottery::stop_repeat(Origin::signed(1)), DispatchError::BadOrigin);
+		// Manager can `stop_repeat`, even twice.
+		assert_ok!(Lottery::stop_repeat(Origin::root()));
+		assert_ok!(Lottery::stop_repeat(Origin::root()));
+
+		// Lottery still exists.
+		assert!(crate::Lottery::<Test>::get().is_some());
+		// End and pick a winner.
+		run_to_block(length + delay);
+
+		// Lottery stays dead and does not repeat.
+		assert!(crate::Lottery::<Test>::get().is_none());
+		run_to_block(length + delay + 1);
+		assert!(crate::Lottery::<Test>::get().is_none());
+	});
+}
+
 #[test]
 fn set_calls_works() {
 	new_test_ext().execute_with(|| {
@@ -120,6 +151,27 @@ fn set_calls_works() {
 	});
 }

+#[test]
+fn call_to_indices_works() {
+	new_test_ext().execute_with(|| {
+		let calls = vec![
+			Call::Balances(BalancesCall::force_transfer { source: 0, dest: 0, value: 0 }),
+			Call::Balances(BalancesCall::transfer { dest: 0, value: 0 }),
+		];
+		let indices = Lottery::calls_to_indices(&calls).unwrap().into_inner();
+		// Only comparing the length since it is otherwise dependant on the API
+		// of `BalancesCall`.
+		assert_eq!(indices.len(), calls.len());
+
+		let too_many_calls = vec![
+			Call::Balances(BalancesCall::force_transfer { source: 0, dest: 0, value: 0 }),
+			Call::Balances(BalancesCall::transfer { dest: 0, value: 0 }),
+			Call::System(SystemCall::remark { remark: vec![] }),
+		];
+		assert_noop!(Lottery::calls_to_indices(&too_many_calls), Error::<Test>::TooManyCalls);
+	});
+}
+
 #[test]
 fn start_lottery_works() {
 	new_test_ext().execute_with(|| {
@@ -239,6 +291,106 @@ fn buy_ticket_works() {
 	});
 }

+/// Test that `do_buy_ticket` returns an `AlreadyParticipating` error.
+/// Errors of `do_buy_ticket` are ignored by `buy_ticket`, therefore this white-box test.
+#[test]
+fn do_buy_ticket_already_participating() {
+	new_test_ext().execute_with(|| {
+		let calls = vec![Call::Balances(BalancesCall::transfer { dest: 0, value: 0 })];
+		assert_ok!(Lottery::set_calls(Origin::root(), calls.clone()));
+		assert_ok!(Lottery::start_lottery(Origin::root(), 1, 10, 10, false));
+
+		// Buying once works.
+		assert_ok!(Lottery::do_buy_ticket(&1, &calls[0]));
+		// Buying the same ticket again fails.
+		assert_noop!(Lottery::do_buy_ticket(&1, &calls[0]), Error::<Test>::AlreadyParticipating);
+	});
+}
+
+/// `buy_ticket` is a storage noop when called with the same ticket again.
+#[test]
+fn buy_ticket_already_participating() {
+	new_test_ext().execute_with(|| {
+		let calls = vec![Call::Balances(BalancesCall::transfer { dest: 0, value: 0 })];
+		assert_ok!(Lottery::set_calls(Origin::root(), calls.clone()));
+		assert_ok!(Lottery::start_lottery(Origin::root(), 1, 10, 10, false));
+
+		// Buying once works.
+		let call = Box::new(calls[0].clone());
+		assert_ok!(Lottery::buy_ticket(Origin::signed(1), call.clone()));
+
+		// Buying the same ticket again returns Ok, but changes nothing.
+		assert_storage_noop!(Lottery::buy_ticket(Origin::signed(1), call).unwrap());
+
+		// Exactly one ticket exists.
+		assert_eq!(TicketsCount::<Test>::get(), 1);
+	});
+}
+
+/// `buy_ticket` is a storage noop when called with insufficient balance.
+#[test]
+fn buy_ticket_insufficient_balance() {
+	new_test_ext().execute_with(|| {
+		let calls = vec![Call::Balances(BalancesCall::transfer { dest: 0, value: 0 })];
+		assert_ok!(Lottery::set_calls(Origin::root(), calls.clone()));
+		// Price set to 100.
+		assert_ok!(Lottery::start_lottery(Origin::root(), 100, 10, 10, false));
+		let call = Box::new(calls[0].clone());
+
+		// Buying a ticket returns Ok, but changes nothing.
+		assert_storage_noop!(Lottery::buy_ticket(Origin::signed(1), call).unwrap());
+		assert!(TicketsCount::<Test>::get().is_zero());
+	});
+}
+
+#[test]
+fn do_buy_ticket_insufficient_balance() {
+	new_test_ext().execute_with(|| {
+		let calls = vec![Call::Balances(BalancesCall::transfer { dest: 0, value: 0 })];
+		assert_ok!(Lottery::set_calls(Origin::root(), calls.clone()));
+		// Price set to 101.
+		assert_ok!(Lottery::start_lottery(Origin::root(), 101, 10, 10, false));
+
+		// Buying fails with InsufficientBalance.
+		assert_noop!(
+			Lottery::do_buy_ticket(&1, &calls[0]),
+			BalancesError::<Test, _>::InsufficientBalance
+		);
+		assert!(TicketsCount::<Test>::get().is_zero());
+	});
+}
+
+#[test]
+fn do_buy_ticket_keep_alive() {
+	new_test_ext().execute_with(|| {
+		let calls = vec![Call::Balances(BalancesCall::transfer { dest: 0, value: 0 })];
+		assert_ok!(Lottery::set_calls(Origin::root(), calls.clone()));
+		// Price set to 100.
+		assert_ok!(Lottery::start_lottery(Origin::root(), 100, 10, 10, false));
+
+		// Buying fails with KeepAlive.
+		assert_noop!(Lottery::do_buy_ticket(&1, &calls[0]), BalancesError::<Test, _>::KeepAlive);
+		assert!(TicketsCount::<Test>::get().is_zero());
+	});
+}
+
+/// The lottery handles the case that no one participated.
+#[test]
+fn no_participants_works() {
+	new_test_ext().execute_with(|| {
+		let length = 20;
+		let delay = 5;
+
+		// Set no calls for the lottery.
+		assert_ok!(Lottery::set_calls(Origin::root(), vec![]));
+		// Start lottery.
+		assert_ok!(Lottery::start_lottery(Origin::root(), 10, length, delay, false));
+
+		// End the lottery, no one wins.
+		run_to_block(length + delay);
+	});
+}
+
 #[test]
 fn start_lottery_will_create_account() {
 	new_test_ext().execute_with(|| {
@@ -251,3 +403,26 @@ fn start_lottery_will_create_account() {
 		assert_eq!(Balances::total_balance(&Lottery::account_id()), 1);
 	});
 }
+
+#[test]
+fn choose_ticket_trivial_cases() {
+	new_test_ext().execute_with(|| {
+		assert!(Lottery::choose_ticket(0).is_none());
+		assert_eq!(Lottery::choose_ticket(1).unwrap(), 0);
+	});
+}
+
+#[test]
+fn choose_account_one_participant() {
+	new_test_ext().execute_with(|| {
+		let calls = vec![Call::Balances(BalancesCall::transfer { dest: 0, value: 0 })];
+		assert_ok!(Lottery::set_calls(Origin::root(), calls.clone()));
+		assert_ok!(Lottery::start_lottery(Origin::root(), 10, 10, 10, false));
+		let call = Box::new(calls[0].clone());
+
+		// Buy one ticket with account 1.
+		assert_ok!(Lottery::buy_ticket(Origin::signed(1), call));
+		// Account 1 is always the winner.
+		assert_eq!(Lottery::choose_account().unwrap(), 1);
+	});
+}