More refactoring, improvements, TODO solving

pallets/dapp-staking-v3/src/lib.rs

@@ -223,6 +223,7 @@ pub mod pallet {
         },
         BonusReward {
             account: T::AccountId,
+            smart_contract: T::SmartContract,
             period: PeriodNumber,
             amount: Balance,
         },
@@ -289,14 +290,10 @@ pub mod pallet {
         InternalUnstakeError,
         /// Rewards are no longer claimable since they are too old.
         RewardExpired,
-        /// All staker rewards for the period have been claimed.
-        StakerRewardsAlreadyClaimed,
         /// There are no claimable rewards.
         NoClaimableRewards,
         /// An unexpected error occured while trying to claim staker rewards.
         InternalClaimStakerError,
-        /// Bonus rewards have already been claimed.
-        BonusRewardAlreadyClaimed,
         /// Account is has no eligible stake amount for bonus reward.
         NotEligibleForBonusReward,
         /// An unexpected error occured while trying to claim bonus reward.
@@ -840,7 +837,7 @@ pub mod pallet {
                 0
             };
 
-            // TODO: discussion point - it's possible that this will "kill" users ability to withdraw past rewards.
+            // TODO: discussion point - this will "kill" users ability to withdraw past rewards.
             // This can be handled by the frontend though.
 
             Self::update_ledger(&account, ledger);
@@ -1130,10 +1127,7 @@ pub mod pallet {
             // Check if the rewards have expired
             let protocol_state = ActiveProtocolState::<T>::get();
             ensure!(
-                staked_period
-                    >= protocol_state
-                        .period_number()
-                        .saturating_sub(T::RewardRetentionInPeriods::get()),
+                staked_period >= Self::oldest_claimable_period(protocol_state.period_number()),
                 Error::<T>::RewardExpired
             );
 
@@ -1203,8 +1197,7 @@ pub mod pallet {
             Ok(())
         }
 
-        // TODO: perhaps this should be changed to include smart contract from which rewards are being claimed.
-        /// Used to claim bonus reward for the eligible era, if applicable.
+        /// Used to claim bonus reward for a smart contract, if eligible.
         #[pallet::call_index(12)]
         #[pallet::weight(Weight::zero())]
         pub fn claim_bonus_reward(
@@ -1218,7 +1211,10 @@ pub mod pallet {
                 .ok_or(Error::<T>::NoClaimableRewards)?;
             let protocol_state = ActiveProtocolState::<T>::get();
 
-            // Check if period has ended
+            // Ensure:
+            // 1. Period for which rewards are being claimed has ended.
+            // 2. Account has been a loyal staker.
+            // 3. Rewards haven't expired.
             let staked_period = staker_info.period_number();
             ensure!(
                 staked_period < protocol_state.period_number(),
@@ -1230,15 +1226,10 @@ pub mod pallet {
             );
             ensure!(
                 staker_info.period_number()
-                    >= protocol_state
-                        .period_number()
-                        .saturating_sub(T::RewardRetentionInPeriods::get()),
+                    >= Self::oldest_claimable_period(protocol_state.period_number()),
                 Error::<T>::RewardExpired
             );
 
-            // Check if user is applicable for bonus reward
-            let eligible_amount = staker_info.staked_amount(PeriodType::Voting);
-
             let period_end_info =
                 PeriodEnd::<T>::get(&staked_period).ok_or(Error::<T>::InternalClaimBonusError)?;
             // Defensive check - we should never get this far in function if no voting period stake exists.
@@ -1247,6 +1238,7 @@ pub mod pallet {
                 Error::<T>::InternalClaimBonusError
             );
 
+            let eligible_amount = staker_info.staked_amount(PeriodType::Voting);
             let bonus_reward =
                 Perbill::from_rational(eligible_amount, period_end_info.total_vp_stake)
                     * period_end_info.bonus_reward_pool;
@@ -1260,6 +1252,7 @@ pub mod pallet {
 
             Self::deposit_event(Event::<T>::BonusReward {
                 account: account.clone(),
+                smart_contract,
                 period: staked_period,
                 amount: bonus_reward,
             });
@@ -1287,6 +1280,11 @@ pub mod pallet {
 
             // 'Consume' dApp reward for the specified era, if possible.
             let mut dapp_tiers = DAppTiers::<T>::get(&era).ok_or(Error::<T>::NoDAppTierInfo)?;
+            ensure!(
+                Self::oldest_claimable_period(dapp_tiers.period) <= protocol_state.period_number(),
+                Error::<T>::RewardExpired
+            );
+
             let (amount, tier_id) =
                 dapp_tiers
                     .try_consume(dapp_info.id)
@@ -1408,8 +1406,7 @@ pub mod pallet {
             }
 
             // Remove expired ledger stake entries, if needed.
-            let threshold_period =
-                current_period.saturating_sub(T::RewardRetentionInPeriods::get());
+            let threshold_period = Self::oldest_claimable_period(current_period);
             let mut ledger = Ledger::<T>::get(&account);
             if ledger.maybe_cleanup_expired(threshold_period) {
                 Self::update_ledger(&account, ledger);
@@ -1476,6 +1473,12 @@ pub mod pallet {
             era.saturating_sub(era % T::EraRewardSpanLength::get())
         }
 
+        /// Return the oldest period for which rewards can be claimed.
+        /// All rewards before that period are considered to be expired.
+        pub fn oldest_claimable_period(current_period: PeriodNumber) -> PeriodNumber {
+            current_period.saturating_sub(T::RewardRetentionInPeriods::get())
+        }
+
         /// Assign eligible dApps into appropriate tiers, and calculate reward for each tier.
         ///
         /// The returned object contains information about each dApp that made it into a tier.
@@ -1541,7 +1544,7 @@ pub mod pallet {
                 // 2. dApp doesn't satisfy the tier threshold (since they're sorted, none of the following dApps will satisfy the condition either)
                 for (dapp_id, stake_amount) in dapp_stakes[global_idx..max_idx].iter() {
                     if tier_threshold.is_satisfied(*stake_amount) {
-                        global_idx.saturating_accrue(1);
+                        global_idx.saturating_inc();
                         dapp_tiers.push(DAppTier {
                             dapp_id: *dapp_id,
                             tier_id: Some(tier_id),
@@ -1551,7 +1554,7 @@ pub mod pallet {
                     }
                 }
 
-                tier_id.saturating_accrue(1);
+                tier_id.saturating_inc();
             }
 
             // 4.
@@ -1571,6 +1574,7 @@ pub mod pallet {
             DAppTierRewards::<MaxNumberOfContractsU32<T>, T::NumberOfTiers>::new(
                 dapp_tiers,
                 tier_rewards,
+                period,
             )
             .unwrap_or_default()
         }

pallets/dapp-staking-v3/src/test/mock.rs

@@ -199,7 +199,7 @@ impl ExtBuilder {
                 maintenance: false,
             });
 
-            // TODO: improve this laterm should be handled via genesis?
+            // TODO: improve this later, should be handled via genesis?
             let tier_params = TierParameters::<<Test as Config>::NumberOfTiers> {
                 reward_portion: BoundedVec::try_from(vec![
                     Permill::from_percent(40),

pallets/dapp-staking-v3/src/test/testing_utils.rs

@@ -484,7 +484,7 @@ pub(crate) fn assert_stake(
         pre_ledger.stakeable_amount(stake_period) - amount,
         "Stakeable amount must decrease by the 'amount'"
     );
-    // TODO: maybe expand checks here?
+    // TODO: expand with more detailed checks of staked and staked_future
 
     // 2. verify staker info
     // =====================
@@ -885,6 +885,7 @@ pub(crate) fn assert_claim_bonus_reward(account: AccountId, smart_contract: &Moc
     ));
     System::assert_last_event(RuntimeEvent::DappStaking(Event::BonusReward {
         account,
+        smart_contract: *smart_contract,
         period: staked_period,
         amount: reward,
     }));

pallets/dapp-staking-v3/src/test/tests.rs

@@ -102,6 +102,33 @@ fn maintenace_mode_call_filtering_works() {
             DappStaking::unstake(RuntimeOrigin::signed(1), MockSmartContract::default(), 100),
             Error::<Test>::Disabled
         );
+        assert_noop!(
+            DappStaking::claim_staker_rewards(RuntimeOrigin::signed(1)),
+            Error::<Test>::Disabled
+        );
+        assert_noop!(
+            DappStaking::claim_bonus_reward(RuntimeOrigin::signed(1), MockSmartContract::default()),
+            Error::<Test>::Disabled
+        );
+        assert_noop!(
+            DappStaking::claim_dapp_reward(
+                RuntimeOrigin::signed(1),
+                MockSmartContract::default(),
+                1
+            ),
+            Error::<Test>::Disabled
+        );
+        assert_noop!(
+            DappStaking::unstake_from_unregistered(
+                RuntimeOrigin::signed(1),
+                MockSmartContract::default()
+            ),
+            Error::<Test>::Disabled
+        );
+        assert_noop!(
+            DappStaking::cleanup_expired_entries(RuntimeOrigin::signed(1)),
+            Error::<Test>::Disabled
+        );
     })
 }
 

pallets/dapp-staking-v3/src/test/tests_types.rs

@@ -335,9 +335,6 @@ fn account_ledger_add_stake_amount_basic_example_works() {
         .is_ok());
     assert!(acc_ledger.staked.is_empty());
     assert!(acc_ledger.staked_future.is_none());
-    assert!(acc_ledger
-        .staked_amount_for_type(PeriodType::Voting, period_number)
-        .is_zero());
 
     // 1st scenario - stake some amount, and ensure values are as expected.
     let first_era = 1;

pallets/dapp-staking-v3/src/types.rs

@@ -740,7 +740,7 @@ impl Iterator for EraStakePairIter {
         // Afterwards, just keep returning the same amount for different eras
         if self.start_era <= self.end_era {
             let value = (self.start_era, self.amount);
-            self.start_era.saturating_accrue(1);
+            self.start_era.saturating_inc();
             return Some(value);
         } else {
             None
@@ -1559,25 +1559,36 @@ pub struct DAppTierRewards<MD: Get<u32>, NT: Get<u32>> {
     pub dapps: BoundedVec<DAppTier, MD>,
     /// Rewards for each tier. First entry refers to the first tier, and so on.
     pub rewards: BoundedVec<Balance, NT>,
-    // TODO: perhaps I can add 'PeriodNumber' here so it's easy to identify expired rewards?
+    /// Period during which this struct was created.
+    #[codec(compact)]
+    pub period: PeriodNumber,
 }
 
 impl<MD: Get<u32>, NT: Get<u32>> Default for DAppTierRewards<MD, NT> {
     fn default() -> Self {
         Self {
             dapps: BoundedVec::default(),
             rewards: BoundedVec::default(),
+            period: 0,
         }
     }
 }
 
 impl<MD: Get<u32>, NT: Get<u32>> DAppTierRewards<MD, NT> {
     /// Attempt to construct `DAppTierRewards` struct.
     /// If the provided arguments exceed the allowed capacity, return an error.
-    pub fn new(dapps: Vec<DAppTier>, rewards: Vec<Balance>) -> Result<Self, ()> {
+    pub fn new(
+        dapps: Vec<DAppTier>,
+        rewards: Vec<Balance>,
+        period: PeriodNumber,
+    ) -> Result<Self, ()> {
         let dapps = BoundedVec::try_from(dapps).map_err(|_| ())?;
         let rewards = BoundedVec::try_from(rewards).map_err(|_| ())?;
-        Ok(Self { dapps, rewards })
+        Ok(Self {
+            dapps,
+            rewards,
+            period,
+        })
     }
 
     /// Consume reward for the specified dapp id, returning its amount and tier Id.