From 7396117579c4bb9d625311910857cbba9e2ecfa0 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 11 Jan 2022 15:22:27 +0100 Subject: [PATCH] pallet-lottery: add generate_storage_info (#10594) * pallet-lottery: add generate_storage_info Signed-off-by: Oliver Tale-Yazdi * pallet-lottery: test call_to_indices with TooManyCalls Signed-off-by: Oliver Tale-Yazdi * review: move try_push above transfer Signed-off-by: Oliver Tale-Yazdi * pallet-lottery: test stop_repeat Signed-off-by: Oliver Tale-Yazdi * pallet-lottery: test do_buy_ticket as white-box Signed-off-by: Oliver Tale-Yazdi * pallet-lottery: use BoundedVec in bechmarks Signed-off-by: Oliver Tale-Yazdi * pallet-lottery: fix zero div panic Signed-off-by: Oliver Tale-Yazdi * review: extend buy_ticket tests Signed-off-by: Oliver Tale-Yazdi * review: test buy_ticket AlreadyParticipating Signed-off-by: Oliver Tale-Yazdi * fmt Signed-off-by: Oliver Tale-Yazdi * review: use /// comments on private functions Signed-off-by: Oliver Tale-Yazdi * review: use with_bounded_capacity Signed-off-by: Oliver Tale-Yazdi --- frame/lottery/src/benchmarking.rs | 11 +- frame/lottery/src/lib.rs | 77 ++++++++----- frame/lottery/src/tests.rs | 179 +++++++++++++++++++++++++++++- 3 files changed, 235 insertions(+), 32 deletions(-) diff --git a/frame/lottery/src/benchmarking.rs b/frame/lottery/src/benchmarking.rs index f4a8e88e5e3f1..1c850e66f9c6e 100644 --- a/frame/lottery/src/benchmarking.rs +++ b/frame/lottery/src/benchmarking.rs @@ -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::::call_to_index( &frame_system::Call::::set_code{ code: vec![] }.into() )?; - let already_called: (u32, Vec) = ( + let already_called: (u32, BoundedVec) = ( LotteryIndex::::get(), - vec![ + BoundedVec::::try_from(vec![ set_code_index; T::MaxCalls::get().saturating_sub(1) as usize - ], + ]).unwrap(), ); Participants::::insert(&caller, already_called); diff --git a/frame/lottery/src/lib.rs b/frame/lottery/src/lib.rs index 5a985094dd1c9..b04dbc8455684 100644 --- a/frame/lottery/src/lib.rs +++ b/frame/lottery/src/lib.rs @@ -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 = // 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 { /// 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(_); /// 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 = - StorageMap<_, Twox64Concat, T::AccountId, (u32, Vec), ValueQuery>; + pub(crate) type Participants = StorageMap< + _, + Twox64Concat, + T::AccountId, + (u32, BoundedVec), + 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 = StorageValue<_, Vec, ValueQuery>; + pub(crate) type CallIndices = + StorageValue<_, BoundedVec, ValueQuery>; #[pallet::hooks] impl Hooks> for Pallet { @@ -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::::get(); - let winning_number = Self::choose_winner(ticket_count); - let winner = Tickets::::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 Pallet { } /// 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) { let account_id = Self::account_id(); let balance = @@ -394,17 +403,19 @@ impl Pallet { (account_id, balance) } - // Converts a vector of calls into a vector of call indices. - fn calls_to_indices(calls: &[::Call]) -> Result, 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: &[::Call], + ) -> Result, DispatchError> { + let mut indices = BoundedVec::::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::::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: &::Call) -> Result { let encoded_call = call.encode(); if encoded_call.len() < 2 { @@ -413,7 +424,7 @@ impl Pallet { 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: &::Call) -> DispatchResult { // Check the call is valid lottery let config = Lottery::::get().ok_or(Error::::NotConfigured)?; @@ -433,7 +444,7 @@ impl Pallet { let index = LotteryIndex::::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 Pallet { Error::::AlreadyParticipating ); } + participating_calls.try_push(call_index).map_err(|_| Error::::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::::put(new_ticket_count); Tickets::::insert(ticket_count, caller.clone()); - participating_calls.push(call_index); Ok(()) }, )?; @@ -457,8 +468,22 @@ impl Pallet { 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 { + match Self::choose_ticket(TicketsCount::::get()) { + None => None, + Some(ticket) => Tickets::::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 { + 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 Pallet { 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 = ::decode(&mut random_seed.as_ref()) diff --git a/frame/lottery/src/tests.rs b/frame/lottery/src/tests.rs index 143a4b0528773..d8dd6e4b7fe6c 100644 --- a/frame/lottery/src/tests.rs +++ b/frame/lottery/src/tests.rs @@ -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::::get().is_none()); - assert_eq!(Participants::::get(&1), (0, vec![])); + assert_eq!(Participants::::get(&1), (0, Default::default())); assert_eq!(TicketsCount::::get(), 0); assert!(Tickets::::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::::get().is_some()); + // End and pick a winner. + run_to_block(length + delay); + + // Lottery stays dead and does not repeat. + assert!(crate::Lottery::::get().is_none()); + run_to_block(length + delay + 1); + assert!(crate::Lottery::::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::::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::::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::::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::::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::::InsufficientBalance + ); + assert!(TicketsCount::::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::::KeepAlive); + assert!(TicketsCount::::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); + }); +}