dApp Staking v3 - improvements & fixes (#1128)

* Fix for incorrect add stake

* Minor adjustments

* Adjustments

* Typo & renaming

* More review adjustments

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

@@ -19,17 +19,17 @@
 //! # dApp Staking v3 Pallet
 //!
 //! For detailed high level documentation, please refer to the attached README.md file.
-//! The crate level docs will cover overal pallet structure & implementation details.
+//! The crate level docs will cover overall pallet structure & implementation details.
 //!
 //! ## Overview
 //!
 //! Pallet that implements the dApp staking v3 protocol.
 //! It covers everything from locking, staking, tier configuration & assignment, reward calculation & payout.
 //!
 //! The `types` module contains all of the types used to implement the pallet.
-//! All of these _types_ are exentisvely tested in their dedicated `test_types` module.
+//! All of these _types_ are extensively tested in their dedicated `test_types` module.
 //!
-//! Rest of the pallet logic is concenrated in the lib.rs file.
+//! Rest of the pallet logic is concentrated in the lib.rs file.
 //! This logic is tested in the `tests` module, with the help of extensive `testing_utils`.
 //!
 
@@ -315,7 +315,7 @@ pub mod pallet {
         UnavailableStakeFunds,
         /// There are unclaimed rewards remaining from past eras or periods. They should be claimed before attempting any stake modification again.
         UnclaimedRewards,
-        /// An unexpected error occured while trying to stake.
+        /// An unexpected error occurred while trying to stake.
         InternalStakeError,
         /// Total staked amount on contract is below the minimum required value.
         InsufficientStakeAmount,
@@ -327,26 +327,26 @@ pub mod pallet {
         UnstakeAmountTooLarge,
         /// Account has no staking information for the contract.
         NoStakingInfo,
-        /// An unexpected error occured while trying to unstake.
+        /// An unexpected error occurred while trying to unstake.
         InternalUnstakeError,
         /// Rewards are no longer claimable since they are too old.
         RewardExpired,
         /// Reward payout has failed due to an unexpected reason.
         RewardPayoutFailed,
         /// There are no claimable rewards.
         NoClaimableRewards,
-        /// An unexpected error occured while trying to claim staker rewards.
+        /// An unexpected error occurred while trying to claim staker rewards.
         InternalClaimStakerError,
         /// Account is has no eligible stake amount for bonus reward.
         NotEligibleForBonusReward,
-        /// An unexpected error occured while trying to claim bonus reward.
+        /// An unexpected error occurred while trying to claim bonus reward.
         InternalClaimBonusError,
         /// Claim era is invalid - it must be in history, and rewards must exist for it.
         InvalidClaimEra,
         /// No dApp tier info exists for the specified era. This can be because era has expired
         /// or because during the specified era there were no eligible rewards or protocol wasn't active.
         NoDAppTierInfo,
-        /// An unexpected error occured while trying to claim dApp reward.
+        /// An unexpected error occurred while trying to claim dApp reward.
         InternalClaimDAppError,
         /// Contract is still active, not unregistered.
         ContractStillActive,
@@ -632,7 +632,7 @@ pub mod pallet {
                     owner: owner.clone(),
                     id: dapp_id,
                     state: DAppState::Registered,
-                    reward_destination: None,
+                    reward_beneficiary: None,
                 },
             );
 
@@ -671,7 +671,7 @@ pub mod pallet {
 
                     ensure!(dapp_info.owner == dev_account, Error::<T>::OriginNotOwner);
 
-                    dapp_info.reward_destination = beneficiary.clone();
+                    dapp_info.reward_beneficiary = beneficiary.clone();
 
                     Ok(())
                 },
@@ -745,10 +745,7 @@ pub mod pallet {
             let mut dapp_info =
                 IntegratedDApps::<T>::get(&smart_contract).ok_or(Error::<T>::ContractNotFound)?;
 
-            ensure!(
-                dapp_info.state == DAppState::Registered,
-                Error::<T>::NotOperatedDApp
-            );
+            ensure!(dapp_info.is_registered(), Error::<T>::NotOperatedDApp);
 
             ContractStake::<T>::remove(&dapp_info.id);
 
@@ -1694,12 +1691,11 @@ pub mod pallet {
                 counter.saturating_inc();
 
                 // Skip dApps which don't have ANY amount staked
-                let stake_amount = match stake_amount.get(era, period) {
-                    Some(stake_amount) if !stake_amount.total().is_zero() => stake_amount,
-                    _ => continue,
-                };
-
-                dapp_stakes.push((dapp_id, stake_amount.total()));
+                if let Some(stake_amount) = stake_amount.get(era, period) {
+                    if !stake_amount.total().is_zero() {
+                        dapp_stakes.push((dapp_id, stake_amount.total()));
+                    }
+                }
             }
 
             // 2.

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

@@ -110,7 +110,7 @@ pub(crate) fn assert_register(owner: AccountId, smart_contract: &MockSmartContra
     assert_eq!(dapp_info.state, DAppState::Registered);
     assert_eq!(dapp_info.owner, owner);
     assert_eq!(dapp_info.id, pre_snapshot.next_dapp_id);
-    assert!(dapp_info.reward_destination.is_none());
+    assert!(dapp_info.reward_beneficiary.is_none());
 
     assert_eq!(pre_snapshot.next_dapp_id + 1, NextDAppId::<Test>::get());
     assert_eq!(
@@ -142,7 +142,7 @@ pub(crate) fn assert_set_dapp_reward_beneficiary(
     assert_eq!(
         IntegratedDApps::<Test>::get(&smart_contract)
             .unwrap()
-            .reward_destination,
+            .reward_beneficiary,
         beneficiary
     );
 }
@@ -467,21 +467,49 @@ pub(crate) fn assert_stake(
     let post_staker_info = post_snapshot
         .staker_info
         .get(&(account, *smart_contract))
-        .expect("Entry must exist since 'stake' operation was successfull.");
+        .expect("Entry must exist since 'stake' operation was successful.");
     let post_contract_stake = post_snapshot
         .contract_stake
         .get(&pre_snapshot.integrated_dapps[&smart_contract].id)
-        .expect("Entry must exist since 'stake' operation was successfull.");
+        .expect("Entry must exist since 'stake' operation was successful.");
     let post_era_info = post_snapshot.current_era_info;
 
     // 1. verify ledger
     // =====================
     // =====================
-    assert_eq!(
-        post_ledger.staked, pre_ledger.staked,
-        "Must remain exactly the same."
-    );
+    if is_account_ledger_expired(pre_ledger, stake_period) {
+        assert!(
+            post_ledger.staked.is_empty(),
+            "Must be cleaned up if expired."
+        );
+    } else {
+        match pre_ledger.staked_future {
+            Some(stake_amount) => {
+                if stake_amount.era == pre_snapshot.active_protocol_state.era {
+                    assert_eq!(
+                        post_ledger.staked, stake_amount,
+                        "Future entry must be moved over to the current entry."
+                    );
+                } else if stake_amount.era == pre_snapshot.active_protocol_state.era + 1 {
+                    assert_eq!(
+                        post_ledger.staked, pre_ledger.staked,
+                        "Must remain exactly the same, only future must be updated."
+                    );
+                } else {
+                    panic!("Invalid future entry era.");
+                }
+            }
+            None => {
+                assert_eq!(
+                    post_ledger.staked, pre_ledger.staked,
+                    "Must remain exactly the same since there's nothing to be moved."
+                );
+            }
+        }
+    }
+
     assert_eq!(post_ledger.staked_future.unwrap().period, stake_period);
+    assert_eq!(post_ledger.staked_future.unwrap().era, stake_era);
     assert_eq!(
         post_ledger.staked_amount(stake_period),
         pre_ledger.staked_amount(stake_period) + amount,
@@ -625,7 +653,7 @@ pub(crate) fn assert_unstake(
     let post_contract_stake = post_snapshot
         .contract_stake
         .get(&pre_snapshot.integrated_dapps[&smart_contract].id)
-        .expect("Entry must exist since 'unstake' operation was successfull.");
+        .expect("Entry must exist since 'unstake' operation was successful.");
     let post_era_info = post_snapshot.current_era_info;
 
     // 1. verify ledger
@@ -652,9 +680,11 @@ pub(crate) fn assert_unstake(
         );
     } else {
         let post_staker_info = post_snapshot
-        .staker_info
-        .get(&(account, *smart_contract))
-        .expect("Entry must exist since 'stake' operation was successfull and it wasn't a full unstake.");
+            .staker_info
+            .get(&(account, *smart_contract))
+            .expect(
+            "Entry must exist since 'stake' operation was successful and it wasn't a full unstake.",
+        );
         assert_eq!(post_staker_info.period_number(), unstake_period);
         assert_eq!(
             post_staker_info.total_staked_amount(),
@@ -989,7 +1019,7 @@ pub(crate) fn assert_claim_dapp_reward(
     assert_eq!(
         pre_reward_info.dapps.len(),
         post_reward_info.dapps.len() + 1,
-        "Entry must have been removed after successfull reward claim."
+        "Entry must have been removed after successful reward claim."
     );
 }
 
@@ -1463,3 +1493,17 @@ pub(crate) fn required_number_of_reward_claims(account: AccountId) -> u32 {
 
     second - first + 1
 }
+
+/// Check whether the given account ledger's stake rewards have expired.
+///
+/// `true` if expired, `false` otherwise.
+pub(crate) fn is_account_ledger_expired(
+    ledger: &AccountLedgerFor<Test>,
+    current_period: PeriodNumber,
+) -> bool {
+    let valid_threshold_period = DappStaking::oldest_claimable_period(current_period);
+    match ledger.staked_period() {
+        Some(staked_period) if staked_period < valid_threshold_period => true,
+        _ => false,
+    }
+}

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

@@ -332,7 +332,7 @@ fn set_dapp_reward_beneficiary_for_contract_is_ok() {
         // Update beneficiary
         assert!(IntegratedDApps::<Test>::get(&smart_contract)
             .unwrap()
-            .reward_destination
+            .reward_beneficiary
             .is_none());
         assert_set_dapp_reward_beneficiary(owner, &smart_contract, Some(3));
         assert_set_dapp_reward_beneficiary(owner, &smart_contract, Some(5));

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

@@ -158,14 +158,14 @@ fn dapp_info_basic_checks() {
         owner,
         id: 7,
         state: DAppState::Registered,
-        reward_destination: None,
+        reward_beneficiary: None,
     };
 
     // Owner receives reward in case no beneficiary is set
     assert_eq!(*dapp_info.reward_beneficiary(), owner);
 
     // Beneficiary receives rewards in case it is set
-    dapp_info.reward_destination = Some(beneficiary);
+    dapp_info.reward_beneficiary = Some(beneficiary);
     assert_eq!(*dapp_info.reward_beneficiary(), beneficiary);
 
     // Check if dApp is registered
@@ -507,7 +507,7 @@ fn account_ledger_staked_era_period_works() {
 }
 
 #[test]
-fn account_ledger_add_stake_amount_basic_example_works() {
+fn account_ledger_add_stake_amount_basic_example_with_different_subperiods_works() {
     get_u32_type!(UnlockingDummy, 5);
     let mut acc_ledger = AccountLedger::<UnlockingDummy>::default();
 
@@ -554,6 +554,7 @@ fn account_ledger_add_stake_amount_basic_example_works() {
             .period,
         period_1
     );
+    assert_eq!(acc_ledger.staked_future.unwrap().era, era_1 + 1);
     assert_eq!(acc_ledger.staked_future.unwrap().voting, stake_amount);
     assert!(acc_ledger.staked_future.unwrap().build_and_earn.is_zero());
     assert_eq!(acc_ledger.staked_amount(period_1), stake_amount);
@@ -566,7 +567,7 @@ fn account_ledger_add_stake_amount_basic_example_works() {
         .is_zero());
 
     // Second scenario - stake some more, but to the next period type
-    let snapshot = acc_ledger.staked;
+    let snapshot = acc_ledger.staked_future.unwrap();
     let period_info_2 = PeriodInfo {
         number: period_1,
         subperiod: Subperiod::BuildAndEarn,
@@ -583,6 +584,82 @@ fn account_ledger_add_stake_amount_basic_example_works() {
         acc_ledger.staked_amount_for_type(Subperiod::BuildAndEarn, period_1),
         1
     );
+
+    assert_eq!(acc_ledger.staked_future.unwrap().era, era_2 + 1);
+    assert_eq!(acc_ledger.staked_future.unwrap().voting, stake_amount);
+    assert_eq!(acc_ledger.staked_future.unwrap().build_and_earn, 1);
+
+    assert_eq!(acc_ledger.staked, snapshot);
+}
+
+#[test]
+fn account_ledger_add_stake_amount_basic_example_with_same_subperiods_works() {
+    get_u32_type!(UnlockingDummy, 5);
+    let mut acc_ledger = AccountLedger::<UnlockingDummy>::default();
+
+    // 1st scenario - stake some amount in first era of the `Build&Earn` subperiod, and ensure values are as expected.
+    let era_1 = 2;
+    let period_1 = 1;
+    let period_info = PeriodInfo {
+        number: period_1,
+        subperiod: Subperiod::BuildAndEarn,
+        next_subperiod_start_era: 100,
+    };
+    let lock_amount = 17;
+    let stake_amount = 11;
+    acc_ledger.add_lock_amount(lock_amount);
+
+    assert!(acc_ledger
+        .add_stake_amount(stake_amount, era_1, period_info)
+        .is_ok());
+
+    assert!(
+        acc_ledger.staked.is_empty(),
+        "Current era must remain unchanged."
+    );
+    assert_eq!(acc_ledger.staked_future.unwrap().period, period_1);
+    assert_eq!(acc_ledger.staked_future.unwrap().era, era_1 + 1);
+    assert_eq!(
+        acc_ledger.staked_future.unwrap().build_and_earn,
+        stake_amount
+    );
+    assert!(acc_ledger.staked_future.unwrap().voting.is_zero());
+    assert_eq!(acc_ledger.staked_amount(period_1), stake_amount);
+    assert_eq!(
+        acc_ledger.staked_amount_for_type(Subperiod::BuildAndEarn, period_1),
+        stake_amount
+    );
+    assert!(acc_ledger
+        .staked_amount_for_type(Subperiod::Voting, period_1)
+        .is_zero());
+
+    // 2nd scenario - stake again, in the same era
+    let snapshot = acc_ledger.staked;
+    assert!(acc_ledger.add_stake_amount(1, era_1, period_info).is_ok());
+    assert_eq!(acc_ledger.staked, snapshot);
+    assert_eq!(acc_ledger.staked_amount(period_1), stake_amount + 1);
+
+    // 2nd scenario - advance an era, and stake some more
+    let snapshot = acc_ledger.staked_future.unwrap();
+    let era_2 = era_1 + 1;
+    assert!(acc_ledger.add_stake_amount(1, era_2, period_info).is_ok());
+
+    assert_eq!(acc_ledger.staked_amount(period_1), stake_amount + 2);
+    assert!(acc_ledger
+        .staked_amount_for_type(Subperiod::Voting, period_1)
+        .is_zero(),);
+    assert_eq!(
+        acc_ledger.staked_amount_for_type(Subperiod::BuildAndEarn, period_1),
+        stake_amount + 2
+    );
+    assert_eq!(acc_ledger.staked_future.unwrap().period, period_1);
+    assert_eq!(acc_ledger.staked_future.unwrap().era, era_2 + 1);
+    assert_eq!(
+        acc_ledger.staked_future.unwrap().build_and_earn,
+        stake_amount + 2
+    );
+    assert!(acc_ledger.staked_future.unwrap().voting.is_zero());
+
     assert_eq!(acc_ledger.staked, snapshot);
 }