From c29607af49a76fb7bbd4acb361117f28992e7586 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dino=20Pa=C4=8Dandi?= <3002868+Dinonard@users.noreply.github.com> Date: Thu, 25 Jan 2024 14:03:46 +0100 Subject: [PATCH] dApp staking v3 - claim staker benchmark fix (#1151) --- .../dapp-staking-v3/src/benchmarking/mod.rs | 28 ++++++++-------- .../dapp-staking-v3/src/benchmarking/utils.rs | 17 +++++++++- pallets/dapp-staking-v3/src/test/tests.rs | 33 ++++++++++++++----- 3 files changed, 56 insertions(+), 22 deletions(-) diff --git a/pallets/dapp-staking-v3/src/benchmarking/mod.rs b/pallets/dapp-staking-v3/src/benchmarking/mod.rs index 0069ec9b55..fbe1a97b9a 100644 --- a/pallets/dapp-staking-v3/src/benchmarking/mod.rs +++ b/pallets/dapp-staking-v3/src/benchmarking/mod.rs @@ -426,7 +426,7 @@ mod benchmarks { } #[benchmark] - fn claim_staker_rewards_past_period(x: Linear<1, { T::EraRewardSpanLength::get() }>) { + fn claim_staker_rewards_past_period(x: Linear<1, { max_claim_size_past_period::() }>) { initial_config::(); // Prepare staker & register smart contract @@ -452,15 +452,16 @@ mod benchmarks { amount )); - // Advance to era just after the last era covered by the first span - force_advance_to_era::(T::EraRewardSpanLength::get()); + // Hacky era advancement to ensure we have the exact number of eras to claim, but are already in the next period. + force_advance_to_era::(max_claim_size_past_period::() - 1); + force_advance_to_next_period::(); - // Hack - modify staker's stake so it seems as if stake was valid from the 'first stake era'/ + // Hack - modify staker's stake so it seems as if stake was valid from the 'first stake era'. // Also fill up the reward span. // // This allows us to easily control how many rewards are claimed, without having to advance large amount of blocks/eras/periods // to find an appropriate scenario. - let first_stake_era = T::EraRewardSpanLength::get() - x; + let first_stake_era = max_claim_size_past_period::() - x; Ledger::::mutate(&staker, |ledger| { ledger.staked = ledger.staked_future.unwrap(); ledger.staked_future = None; @@ -481,9 +482,6 @@ mod benchmarks { } EraRewards::::insert(&0, reward_span); - // This ensures we claim from the past period. - force_advance_to_next_period::(); - // For testing purposes System::::reset_events(); @@ -499,7 +497,7 @@ mod benchmarks { } #[benchmark] - fn claim_staker_rewards_ongoing_period(x: Linear<1, { T::EraRewardSpanLength::get() }>) { + fn claim_staker_rewards_ongoing_period(x: Linear<1, { max_claim_size_ongoing_period::() }>) { initial_config::(); // Prepare staker & register smart contract @@ -525,16 +523,20 @@ mod benchmarks { amount )); - // Advance to era just after the last era covered by the first span - // This means we'll be able to claim all of the rewards from the previous span. - force_advance_to_era::(T::EraRewardSpanLength::get()); + // Advance to era at the end of the first period or first span. + force_advance_to_era::(max_claim_size_ongoing_period::()); + assert_eq!( + ActiveProtocolState::::get().period_number(), + 1, + "Sanity check, we must still be in the first period." + ); // Hack - modify staker's stake so it seems as if stake was valid from the 'first stake era'/ // Also fill up the reward span. // // This allows us to easily control how many rewards are claimed, without having to advance large amount of blocks/eras/periods // to find an appropriate scenario. - let first_stake_era = T::EraRewardSpanLength::get() - x; + let first_stake_era = max_claim_size_ongoing_period::() - x; Ledger::::mutate(&staker, |ledger| { ledger.staked = ledger.staked_future.unwrap(); ledger.staked_future = None; diff --git a/pallets/dapp-staking-v3/src/benchmarking/utils.rs b/pallets/dapp-staking-v3/src/benchmarking/utils.rs index 81a788da4b..83651decfc 100644 --- a/pallets/dapp-staking-v3/src/benchmarking/utils.rs +++ b/pallets/dapp-staking-v3/src/benchmarking/utils.rs @@ -53,7 +53,7 @@ pub(super) fn advance_to_era(era: EraNumber) { /// /// Relies on the `force` approach to advance one era per block. pub(super) fn force_advance_to_era(era: EraNumber) { - assert!(era >= ActiveProtocolState::::get().era); + assert!(era > ActiveProtocolState::::get().era); while ActiveProtocolState::::get().era < era { assert_ok!(DappStaking::::force( RawOrigin::Root.into(), @@ -260,3 +260,18 @@ fn trivial_fisher_yates_shuffle(vector: &mut Vec, random_seed: u64) { rng = (rng.wrapping_mul(8427637) + 1) as usize; // Some random number generation } } + +/// Returns max amount of rewards that can be claimed in a single claim reward call from a past period. +/// +/// Bounded by era reward span length & number of eras per period (not length but absolute number). +pub(super) fn max_claim_size_past_period() -> u32 { + T::EraRewardSpanLength::get().min(T::CycleConfiguration::eras_per_build_and_earn_subperiod()) +} + +/// Returns max amount of rewards that can be claimed in a single claim reward call from an ongoing period. +/// +/// Bounded by era reward span length & number of eras per period (not length but absolute number). +pub(super) fn max_claim_size_ongoing_period() -> u32 { + T::EraRewardSpanLength::get() + .min(T::CycleConfiguration::eras_per_build_and_earn_subperiod() - 1) +} diff --git a/pallets/dapp-staking-v3/src/test/tests.rs b/pallets/dapp-staking-v3/src/test/tests.rs index 3e409379a7..f2faf0cc52 100644 --- a/pallets/dapp-staking-v3/src/test/tests.rs +++ b/pallets/dapp-staking-v3/src/test/tests.rs @@ -1459,7 +1459,7 @@ fn claim_staker_rewards_no_claimable_rewards_fails() { } #[test] -fn claim_staker_rewards_after_expiry_fails() { +fn claim_staker_rewards_era_after_expiry_works() { ExtBuilder::build().execute_with(|| { // Register smart contract, lock&stake some amount let dev_account = 1; @@ -1486,17 +1486,34 @@ fn claim_staker_rewards_after_expiry_fails() { .next_subperiod_start_era - 1, ); + + // Claim must still work assert_claim_staker_rewards(account); + }) +} - // Ensure we're still in the first period for the sake of test validity - assert_eq!( - Ledger::::get(&account).staked.period, - 1, - "Sanity check." +#[test] +fn claim_staker_rewards_after_expiry_fails() { + ExtBuilder::build().execute_with(|| { + // Register smart contract, lock&stake some amount + let dev_account = 1; + let smart_contract = MockSmartContract::wasm(1 as AccountId); + assert_register(dev_account, &smart_contract); + + let account = 2; + let lock_amount = 300; + assert_lock(account, lock_amount); + let stake_amount = 93; + assert_stake(account, &smart_contract, stake_amount); + + let reward_retention_in_periods: PeriodNumber = + ::RewardRetentionInPeriods::get(); + + // Advance to the period at which rewards expire. + advance_to_period( + ActiveProtocolState::::get().period_number() + reward_retention_in_periods + 1, ); - // Trigger next period, rewards should be marked as expired - advance_to_next_era(); assert_eq!( ActiveProtocolState::::get().period_number(), reward_retention_in_periods + 2