diff --git a/pallets/dapp-staking-v3/src/lib.rs b/pallets/dapp-staking-v3/src/lib.rs index 1fd47435d5..01a9560a71 100644 --- a/pallets/dapp-staking-v3/src/lib.rs +++ b/pallets/dapp-staking-v3/src/lib.rs @@ -261,7 +261,9 @@ pub mod pallet { /// An unexpected error occured while trying to unstake. InternalUnstakeError, /// Rewards are no longer claimable since they are too old. - StakerRewardsExpired, + RewardExpired, + /// All staker rewards for the period have been claimed. + StakerRewardsAlreadyClaimed, /// There are no claimable rewards for the account. NoClaimableRewards, /// An unexpected error occured while trying to claim staker rewards. @@ -964,6 +966,7 @@ pub mod pallet { ledger .unstake_amount(amount, unstake_era, protocol_state.period_info) .map_err(|err| match err { + // These are all defensive checks, which should never happen since we already checked them above. AccountLedgerError::InvalidPeriod | AccountLedgerError::InvalidEra => { Error::::UnclaimedRewardsFromPastPeriods } @@ -1024,9 +1027,10 @@ pub mod pallet { ensure!( !ledger.staker_rewards_claimed, - Error::::NoClaimableRewards - ); // TODO: maybe different error type? - // Check if the rewards have expired + Error::::StakerRewardsAlreadyClaimed + ); + + // Check if the rewards have expired let staked_period = ledger .staked_period() .ok_or(Error::::NoClaimableRewards)?; @@ -1035,7 +1039,7 @@ pub mod pallet { >= protocol_state .period_number() .saturating_sub(T::RewardRetentionInPeriods::get()), - Error::::StakerRewardsExpired + Error::::RewardExpired ); // Calculate the reward claim span @@ -1088,7 +1092,6 @@ pub mod pallet { reward_sum.saturating_accrue(staker_reward); } - // TODO: add negative test for this? T::Currency::deposit_into_existing(&account, reward_sum) .map_err(|_| Error::::InternalClaimStakerError)?; @@ -1116,7 +1119,7 @@ pub mod pallet { let mut ledger = Ledger::::get(&account); ensure!( - !ledger.staker_rewards_claimed, + !ledger.bonus_reward_claimed, Error::::BonusRewardAlreadyClaimed ); // Check if the rewards have expired @@ -1128,7 +1131,7 @@ pub mod pallet { >= protocol_state .period_number() .saturating_sub(T::RewardRetentionInPeriods::get()), - Error::::StakerRewardsExpired + Error::::RewardExpired ); // Check if period has ended @@ -1145,24 +1148,28 @@ pub mod pallet { AccountLedgerError::NothingToClaim => Error::::NoClaimableRewards, _ => Error::::InternalClaimBonusError, })?; - ensure!( - !eligible_amount.is_zero(), - Error::::NotEligibleForBonusReward - ); let period_end_info = PeriodEnd::::get(&staked_period).ok_or(Error::::InternalClaimBonusError)?; - // Defensive check, situation should never happen. + + // Defensive check - we should never get this far in function if no voting period stake exists. ensure!( !period_end_info.total_vp_stake.is_zero(), Error::::InternalClaimBonusError ); + // TODO: this functionality is incomplete - what we should do is iterate over all stake entries in the storage, + // and check if the smart contract was still registered when period ended. + // + // This is important since we cannot allow unregistered contracts to be subject for bonus rewards. + // This means 'a loop' but it will be bounded by max limit of unique stakes. + // A function should also be introduced to prepare the account ledger for next era (or to cleanup old expired rewards) + // in case bonus rewards weren't claimed. + let bonus_reward = Perbill::from_rational(eligible_amount, period_end_info.total_vp_stake) * period_end_info.bonus_reward_pool; - // TODO: add negative test for this? T::Currency::deposit_into_existing(&account, bonus_reward) .map_err(|_| Error::::InternalClaimStakerError)?; diff --git a/pallets/dapp-staking-v3/src/test/testing_utils.rs b/pallets/dapp-staking-v3/src/test/testing_utils.rs index 48abb371bb..644a71f6e9 100644 --- a/pallets/dapp-staking-v3/src/test/testing_utils.rs +++ b/pallets/dapp-staking-v3/src/test/testing_utils.rs @@ -796,7 +796,7 @@ pub(crate) fn assert_claim_staker_rewards(account: AccountId) { //clean up possible leftover events System::reset_events(); - // Unstake from smart contract & verify event(s) + // Claim staker rewards & verify all events assert_ok!(DappStaking::claim_staker_rewards(RuntimeOrigin::signed( account ),)); @@ -833,11 +833,16 @@ pub(crate) fn assert_claim_staker_rewards(account: AccountId) { let post_snapshot = MemorySnapshot::new(); let post_ledger = post_snapshot.ledger.get(&account).unwrap(); - if is_full_claim { + if is_full_claim && pre_ledger.bonus_reward_claimed { + assert_eq!(post_ledger.staked, StakeAmount::default()); + assert!(post_ledger.staked_future.is_none()); + assert!(!post_ledger.staker_rewards_claimed); + assert!(!post_ledger.bonus_reward_claimed); + } else if is_full_claim { assert!(post_ledger.staker_rewards_claimed); } else { assert_eq!(post_ledger.staked.era, last_claim_era + 1); - // TODO: expand check? + assert!(post_ledger.staked_future.is_none()); } } @@ -861,7 +866,7 @@ pub(crate) fn assert_claim_bonus_reward(account: AccountId) { let reward = Perbill::from_rational(stake_amount, period_end_info.total_vp_stake) * period_end_info.bonus_reward_pool; - // Unstake from smart contract & verify event(s) + // Claim bonus reward & verify event assert_ok!(DappStaking::claim_bonus_reward(RuntimeOrigin::signed( account ),)); @@ -889,6 +894,17 @@ pub(crate) fn assert_claim_bonus_reward(account: AccountId) { let post_snapshot = MemorySnapshot::new(); let post_ledger = post_snapshot.ledger.get(&account).unwrap(); + + // All rewards for period have been claimed + if pre_ledger.staker_rewards_claimed { + assert_eq!(post_ledger.staked, StakeAmount::default()); + assert!(post_ledger.staked_future.is_none()); + assert!(!post_ledger.staker_rewards_claimed); + assert!(!post_ledger.bonus_reward_claimed); + } else { + // Staker still has some staker rewards remaining + assert!(post_ledger.bonus_reward_claimed); + } } /// Returns from which starting era to which ending era can rewards be claimed for the specified account. diff --git a/pallets/dapp-staking-v3/src/test/tests.rs b/pallets/dapp-staking-v3/src/test/tests.rs index bbae432895..87716c6c5f 100644 --- a/pallets/dapp-staking-v3/src/test/tests.rs +++ b/pallets/dapp-staking-v3/src/test/tests.rs @@ -626,6 +626,12 @@ fn claim_unlocked_is_ok() { run_for_blocks(2); assert_claim_unlocked(account); assert!(Ledger::::get(&account).unlocking.is_empty()); + + // Unlock everything + assert_unlock(account, lock_amount); + run_for_blocks(unlocking_blocks); + assert_claim_unlocked(account); + assert!(!Ledger::::contains_key(&account)); }) } @@ -1135,6 +1141,33 @@ fn claim_staker_rewards_basic_example_is_ok() { }) } +#[test] +fn claim_staker_rewards_double_call_fails() { + ExtBuilder::build().execute_with(|| { + // Register smart contract, lock&stake some amount + let dev_account = 1; + let smart_contract = MockSmartContract::default(); + 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); + + // Advance into the next period, claim all eligible rewards + advance_to_next_period(); + for _ in 0..required_number_of_reward_claims(account) { + assert_claim_staker_rewards(account); + } + + assert_noop!( + DappStaking::claim_staker_rewards(RuntimeOrigin::signed(account)), + Error::::StakerRewardsAlreadyClaimed, + ); + }) +} + #[test] fn claim_staker_rewards_no_claimable_rewards_fails() { ExtBuilder::build().execute_with(|| { @@ -1215,7 +1248,161 @@ fn claim_staker_rewards_after_expiry_fails() { ); assert_noop!( DappStaking::claim_staker_rewards(RuntimeOrigin::signed(account)), - Error::::StakerRewardsExpired, + Error::::RewardExpired, + ); + }) +} + +#[test] +fn claim_bonus_reward_works() { + ExtBuilder::build().execute_with(|| { + // Register smart contract, lock&stake some amount + let dev_account = 1; + let smart_contract = MockSmartContract::default(); + 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); + + // 1st scenario - advance to the next period, first claim bonus reward, then staker rewards + advance_to_next_period(); + assert_claim_bonus_reward(account); + for _ in 0..required_number_of_reward_claims(account) { + assert_claim_staker_rewards(account); + } + + // 2nd scenario - stake again, advance to next period, this time first claim staker rewards, then bonus reward. + assert_stake(account, &smart_contract, stake_amount); + advance_to_next_period(); + for _ in 0..required_number_of_reward_claims(account) { + assert_claim_staker_rewards(account); + } + assert!( + Ledger::::get(&account).staker_rewards_claimed, + "Sanity check." + ); + assert_claim_bonus_reward(account); + }) +} + +#[test] +fn claim_bonus_reward_double_call_fails() { + ExtBuilder::build().execute_with(|| { + // Register smart contract, lock&stake some amount + let dev_account = 1; + let smart_contract = MockSmartContract::default(); + 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); + + // Advance to the next period, claim bonus reward, then try to do it again + advance_to_next_period(); + assert_claim_bonus_reward(account); + + assert_noop!( + DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account)), + Error::::BonusRewardAlreadyClaimed, + ); + }) +} + +#[test] +fn claim_bonus_reward_when_nothing_to_claim_fails() { + ExtBuilder::build().execute_with(|| { + // Register smart contract, lock&stake some amount + let dev_account = 1; + let smart_contract = MockSmartContract::default(); + assert_register(dev_account, &smart_contract); + + let account = 2; + let lock_amount = 300; + assert_lock(account, lock_amount); + + // 1st - try to claim bonus reward when no stake is present + assert_noop!( + DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account)), + Error::::NoClaimableRewards, + ); + + // 2nd - try to claim bonus reward for the ongoing period + let stake_amount = 93; + assert_stake(account, &smart_contract, stake_amount); + assert_noop!( + DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account)), + Error::::NoClaimableRewards, + ); + }) +} + +#[test] +fn claim_bonus_reward_with_only_build_and_earn_stake_fails() { + ExtBuilder::build().execute_with(|| { + // Register smart contract, lock&stake some amount + let dev_account = 1; + let smart_contract = MockSmartContract::default(); + assert_register(dev_account, &smart_contract); + + let account = 2; + let lock_amount = 300; + assert_lock(account, lock_amount); + + // Stake in Build&Earn period type, advance to next era and try to claim bonus reward + advance_to_next_period_type(); + assert_eq!( + ActiveProtocolState::::get().period_type(), + PeriodType::BuildAndEarn, + "Sanity check." + ); + let stake_amount = 93; + assert_stake(account, &smart_contract, stake_amount); + + advance_to_next_period(); + assert_noop!( + DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account)), + Error::::NoClaimableRewards, + ); + }) +} + +#[test] +fn claim_bonus_reward_after_expiry_fails() { + ExtBuilder::build().execute_with(|| { + // Register smart contract, lock&stake some amount + let dev_account = 1; + let smart_contract = MockSmartContract::default(); + assert_register(dev_account, &smart_contract); + + let account = 2; + let lock_amount = 300; + assert_lock(account, lock_amount); + assert_stake(account, &smart_contract, lock_amount); + + // 1st scenario - Advance to one period before the expiry, claim should still work. + let reward_retention_in_periods: PeriodNumber = + ::RewardRetentionInPeriods::get(); + advance_to_period( + ActiveProtocolState::::get().period_number() + reward_retention_in_periods, + ); + assert_claim_bonus_reward(account); + for _ in 0..required_number_of_reward_claims(account) { + assert_claim_staker_rewards(account); + } + + // 2nd scenario - advance past the expiry, call must fail + assert_stake(account, &smart_contract, lock_amount); + advance_to_period( + ActiveProtocolState::::get().period_number() + reward_retention_in_periods + 1, + ); + assert_noop!( + DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account)), + Error::::RewardExpired, ); }) } diff --git a/pallets/dapp-staking-v3/src/types.rs b/pallets/dapp-staking-v3/src/types.rs index 1df13ffb8a..8065b35c90 100644 --- a/pallets/dapp-staking-v3/src/types.rs +++ b/pallets/dapp-staking-v3/src/types.rs @@ -276,6 +276,7 @@ pub struct AccountLedger< pub staker_rewards_claimed: bool, /// Indicator whether bonus rewards for the period has been claimed. pub bonus_reward_claimed: bool, + // TODO: introduce a variable which keeps track of on how many contracts this account has stake entries for. } impl Default for AccountLedger @@ -445,7 +446,6 @@ where } } - // TODO: update this /// Adds the specified amount to total staked amount, if possible. /// /// Staking can only be done for the ongoing period, and era. @@ -1225,7 +1225,7 @@ impl ContractStakeAmountSeries { fn prune(&mut self) { // Prune the oldest entry if we have more than the limit if self.0.len() == STAKING_SERIES_HISTORY as usize { - // TODO: this can be perhaps optimized so we prune entries which are very old. + // This can be perhaps optimized so we prune entries which are very old. // However, this makes the code more complex & more error prone. // If kept like this, we always make sure we cover the history, and we never exceed it. self.0.remove(0);