Comments, tests, improved coverage

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::<T>::UnclaimedRewardsFromPastPeriods
                     }
@@ -1024,9 +1027,10 @@ pub mod pallet {
 
             ensure!(
                 !ledger.staker_rewards_claimed,
-                Error::<T>::NoClaimableRewards
-            ); // TODO: maybe different error type?
-               // Check if the rewards have expired
+                Error::<T>::StakerRewardsAlreadyClaimed
+            );
+
+            // Check if the rewards have expired
             let staked_period = ledger
                 .staked_period()
                 .ok_or(Error::<T>::NoClaimableRewards)?;
@@ -1035,7 +1039,7 @@ pub mod pallet {
                     >= protocol_state
                         .period_number()
                         .saturating_sub(T::RewardRetentionInPeriods::get()),
-                Error::<T>::StakerRewardsExpired
+                Error::<T>::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::<T>::InternalClaimStakerError)?;
 
@@ -1116,7 +1119,7 @@ pub mod pallet {
             let mut ledger = Ledger::<T>::get(&account);
 
             ensure!(
-                !ledger.staker_rewards_claimed,
+                !ledger.bonus_reward_claimed,
                 Error::<T>::BonusRewardAlreadyClaimed
             );
             // Check if the rewards have expired
@@ -1128,7 +1131,7 @@ pub mod pallet {
                     >= protocol_state
                         .period_number()
                         .saturating_sub(T::RewardRetentionInPeriods::get()),
-                Error::<T>::StakerRewardsExpired
+                Error::<T>::RewardExpired
             );
 
             // Check if period has ended
@@ -1145,24 +1148,28 @@ pub mod pallet {
                         AccountLedgerError::NothingToClaim => Error::<T>::NoClaimableRewards,
                         _ => Error::<T>::InternalClaimBonusError,
                     })?;
-            ensure!(
-                !eligible_amount.is_zero(),
-                Error::<T>::NotEligibleForBonusReward
-            );
 
             let period_end_info =
                 PeriodEnd::<T>::get(&staked_period).ok_or(Error::<T>::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::<T>::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::<T>::InternalClaimStakerError)?;
 

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.

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::<Test>::get(&account).unlocking.is_empty());
+
+        // Unlock everything
+        assert_unlock(account, lock_amount);
+        run_for_blocks(unlocking_blocks);
+        assert_claim_unlocked(account);
+        assert!(!Ledger::<Test>::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::<Test>::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::<Test>::StakerRewardsExpired,
+            Error::<Test>::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::<Test>::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::<Test>::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::<Test>::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::<Test>::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::<Test>::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::<Test>::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 =
+            <Test as Config>::RewardRetentionInPeriods::get();
+        advance_to_period(
+            ActiveProtocolState::<Test>::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::<Test>::get().period_number() + reward_retention_in_periods + 1,
+        );
+        assert_noop!(
+            DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account)),
+            Error::<Test>::RewardExpired,
         );
     })
 }

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<BlockNumber, UnlockingLen> Default for AccountLedger<BlockNumber, UnlockingLen>
@@ -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);