Bug fixes, improvements

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

@@ -352,10 +352,10 @@ pub mod pallet {
             }
 
             let mut era_info = CurrentEraInfo::<T>::get();
-            let staker_reward_pool = Balance::from(1000_u128); // TODO: calculate this properly, inject it from outside (Tokenomics 2.0 pallet?)
+            let staker_reward_pool = Balance::from(1_000_000_000_000u128); // TODO: calculate this properly, inject it from outside (Tokenomics 2.0 pallet?)
             let era_reward = EraReward::new(staker_reward_pool, era_info.total_staked_amount());
-            let ending_era = protocol_state.era;
-            let next_era = ending_era.saturating_add(1);
+            let current_era = protocol_state.era;
+            let next_era = current_era.saturating_add(1);
             let maybe_period_event = match protocol_state.period_type() {
                 PeriodType::Voting => {
                     // For the sake of consistency
@@ -377,6 +377,17 @@ pub mod pallet {
 
                     // Switch to `Voting` period if conditions are met.
                     if protocol_state.period_info.is_next_period(next_era) {
+                        // Store info about period end
+                        let bonus_reward_pool = Balance::from(3_000_000_u32); // TODO: get this from Tokenomics 2.0 pallet
+                        PeriodEnd::<T>::insert(
+                            &protocol_state.period_number(),
+                            PeriodEndInfo {
+                                bonus_reward_pool,
+                                total_vp_stake: era_info.staked_amount(PeriodType::Voting),
+                                final_era: current_era,
+                            },
+                        );
+
                         // For the sake of consistency we treat the whole `Voting` period as a single era.
                         // This means no special handling is required for this period, it only lasts potentially longer than a single standard era.
                         let ending_era = next_era.saturating_add(1);
@@ -411,12 +422,12 @@ pub mod pallet {
 
             CurrentEraInfo::<T>::put(era_info);
 
-            let era_span_index = Self::era_reward_span_index(ending_era);
+            let era_span_index = Self::era_reward_span_index(current_era);
             let mut span = EraRewards::<T>::get(&era_span_index).unwrap_or(EraRewardSpan::new());
             // TODO: error must not happen here. Log an error if it does.
             // The consequence will be that some rewards will be temporarily lost/unavailable, but nothing protocol breaking.
             // Will require a fix from the runtime team though.
-            let _ = span.push(ending_era, era_reward);
+            let _ = span.push(current_era, era_reward);
             EraRewards::<T>::insert(&era_span_index, span);
 
             Self::deposit_event(Event::<T>::NewEra { era: next_era });
@@ -1006,24 +1017,35 @@ pub mod pallet {
             );
 
             // Calculate the reward claim span
-            let first_claim_era = ledger
-                .oldest_stake_era()
-                .ok_or(Error::<T>::InternalClaimStakerError)?;
-            let era_rewards = EraRewards::<T>::get(Self::era_reward_span_index(first_claim_era))
+            let (first_chunk, last_chunk) = ledger
+                .first_and_last_stake_chunks()
                 .ok_or(Error::<T>::InternalClaimStakerError)?;
+            let era_rewards =
+                EraRewards::<T>::get(Self::era_reward_span_index(first_chunk.get_era()))
+                    .ok_or(Error::<T>::InternalClaimStakerError)?;
 
-            // last_period_era or current_era - 1
+            // The last era for which we can theoretically claim rewards.
             let last_period_era = if staked_period == protocol_state.period_number() {
                 protocol_state.era.saturating_sub(1)
             } else {
                 PeriodEnd::<T>::get(&staked_period)
                     .ok_or(Error::<T>::InternalClaimStakerError)?
                     .final_era
             };
-            let last_claim_era = era_rewards.last_era().min(last_period_era);
 
-            let is_full_period_claimed =
-                staked_period < protocol_state.period_number() && last_period_era == last_claim_era;
+            // The last era for which we can claim rewards for this account.
+            // Limiting factors are:
+            //    1. era reward span entries
+            //    2. last era in period limit
+            //    3. last era in which staker had non-zero stake
+            let last_claim_era = if last_chunk.get_amount().is_zero() {
+                era_rewards
+                    .last_era()
+                    .min(last_period_era)
+                    .min(last_chunk.get_era())
+            } else {
+                era_rewards.last_era().min(last_period_era)
+            };
 
             // Get chunks for reward claiming
             let chunks_for_claim = ledger
@@ -1032,10 +1054,17 @@ pub mod pallet {
                 .map_err(|_| Error::<T>::InternalClaimStakerError)?;
             ensure!(chunks_for_claim.0.len() > 0, Error::<T>::NoClaimableRewards);
 
+            // Full reward claim is done if no more rewards are claimable.
+            // This can be either due to: 1) we've claimed the last period
+            // TODO: this is wrong, the first part of check needs to be improved
+            let is_full_period_claimed = staked_period < protocol_state.period_number()
+                && last_period_era == last_claim_era
+                || ledger.staked.0.is_empty();
+
             // Calculate rewards
             let mut rewards: Vec<_> = Vec::new();
             let mut reward_sum = Balance::zero();
-            for era in first_claim_era..=last_claim_era {
+            for era in first_chunk.get_era()..=last_claim_era {
                 let era_reward = era_rewards
                     .get(era)
                     .ok_or(Error::<T>::InternalClaimStakerError)?;
@@ -1046,6 +1075,9 @@ pub mod pallet {
 
                 let staker_reward = Perbill::from_rational(chunk.get_amount(), era_reward.staked())
                     * era_reward.staker_reward_pool();
+                if staker_reward.is_zero() {
+                    continue;
+                }
                 rewards.push((era, staker_reward));
                 reward_sum.saturating_accrue(staker_reward);
             }
@@ -1123,7 +1155,7 @@ pub mod pallet {
         }
 
         /// Calculates the `EraRewardSpan` index for the specified era.
-        fn era_reward_span_index(era: EraNumber) -> EraNumber {
+        pub fn era_reward_span_index(era: EraNumber) -> EraNumber {
             era.saturating_sub(era % T::EraRewardSpanLength::get())
         }
     }

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

@@ -788,8 +788,6 @@ pub(crate) fn assert_claim_staker_rewards(account: AccountId) {
     let (last_claim_era, is_full_claim) = if is_current_period_stake {
         (pre_snapshot.active_protocol_state.era - 1, false)
     } else {
-        let last_claim_era = era_rewards_span.last_era();
-
         let claim_period = pre_ledger.staked_period.unwrap();
         let period_end = pre_snapshot
             .period_end
@@ -825,6 +823,9 @@ pub(crate) fn assert_claim_staker_rewards(account: AccountId) {
         .iter()
         .fold(Balance::zero(), |acc, (_, reward)| acc + reward);
 
+    //clean up possible leftover events
+    System::reset_events();
+
     // Unstake from smart contract & verify event(s)
     assert_ok!(DappStaking::claim_staker_rewards(RuntimeOrigin::signed(
         account
@@ -874,3 +875,54 @@ pub(crate) fn assert_claim_staker_rewards(account: AccountId) {
         );
     }
 }
+
+/// Returns from which starting era to which ending era can rewards be claimed for the specified account.
+///
+/// If `None` is returned, there is nothing to claim.
+/// Doesn't consider reward expiration.
+pub(crate) fn claimable_reward_range(account: AccountId) -> Option<(EraNumber, EraNumber)> {
+    let ledger = Ledger::<Test>::get(&account);
+    let protocol_state = ActiveProtocolState::<Test>::get();
+
+    let (first_chunk, last_chunk) = if let Some(chunks) = ledger.first_and_last_stake_chunks() {
+        chunks
+    } else {
+        return None;
+    };
+
+    // Full unstake happened, no rewards past this.
+    let last_claim_era = if last_chunk.get_amount().is_zero() {
+        last_chunk.get_era() - 1
+    } else if ledger.staked_period == Some(protocol_state.period_number()) {
+        // Staked in the ongoing period, best we can do is claim up to last era
+        protocol_state.era - 1
+    } else {
+        // Period finished, we can claim up to its final era
+        let period_end = PeriodEnd::<Test>::get(ledger.staked_period.unwrap()).unwrap();
+        period_end.final_era
+    };
+
+    Some((first_chunk.get_era(), last_claim_era))
+}
+
+/// Number of times it's required to call `claim_staker_rewards` to claim all pending rewards.
+///
+/// In case no rewards are pending, return **zero**.
+pub(crate) fn required_number_of_reward_claims(account: AccountId) -> u32 {
+    let range = if let Some(range) = claimable_reward_range(account) {
+        range
+    } else {
+        return 0;
+    };
+
+    let era_span_length: EraNumber =
+        <Test as pallet_dapp_staking::Config>::EraRewardSpanLength::get();
+    let first = DappStaking::era_reward_span_index(range.0)
+        .checked_div(era_span_length)
+        .unwrap();
+    let second = DappStaking::era_reward_span_index(range.1)
+        .checked_div(era_span_length)
+        .unwrap();
+
+    second - first + 1
+}

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

@@ -1235,3 +1235,39 @@ fn unstake_fails_due_to_too_many_chunks() {
         );
     })
 }
+
+#[test]
+fn claim_staker_rewards_basic_example_is_ok() {
+    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 Build&Earn period, and allow one era to pass. Claim reward for 1 era.
+        advance_to_era(ActiveProtocolState::<Test>::get().era + 2);
+        assert_claim_staker_rewards(account);
+
+        // Advance a few more eras, and claim multiple rewards this time.
+        advance_to_era(ActiveProtocolState::<Test>::get().era + 3);
+        assert_eq!(
+            ActiveProtocolState::<Test>::get().period_number(),
+            1,
+            "Sanity check, we must still be in the 1st period."
+        );
+        assert_claim_staker_rewards(account);
+
+        // Advance into the next period, make sure we can still claim old rewards.
+        // TODO: I should calculate number of expected claims, so I know how much times `claim_staker_rewards` should be called.
+        advance_to_next_period();
+        for _ in 0..required_number_of_reward_claims(account) {
+            assert_claim_staker_rewards(account);
+        }
+    })
+}

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

@@ -243,6 +243,8 @@ where
     ///  b) [1,2] -- split(4) --> [1,2],[5]
     ///  c) [1,2,4,5] -- split(4) --> [1,2,4],[5]
     ///  d) [1,2,4,6] -- split(4) --> [1,2,4],[5,6]
+    ///  e) [1,2(0)]  -- split(4) --> [1,2],[]   // 2nd entry has 'zero' amount, so no need to keep it
+    // TODO: what if last element on the right is zero? We should cleanup the vector then.
     pub fn left_split(&mut self, era: EraNumber) -> Result<Self, AccountLedgerError> {
         // TODO: this implementation can once again be optimized, sacrificing the code readability a bit.
         // But I don't think it's worth doing, since we're aiming for the safe side here.
@@ -260,16 +262,24 @@ where
 
         if let Some(&last_l_chunk) = left.last() {
             // In case 'right' part is missing an entry covering the specified era, add it.
-            match right.first() {
+            let maybe_chunk = match right.first() {
                 Some(first_r_chunk) if first_r_chunk.get_era() > era.saturating_add(1) => {
                     let mut new_chunk = last_l_chunk.clone();
                     new_chunk.set_era(era.saturating_add(1));
-                    right.insert(0, new_chunk);
+                    Some(new_chunk)
                 }
                 None => {
                     let mut new_chunk = last_l_chunk.clone();
                     new_chunk.set_era(era.saturating_add(1));
-                    right.insert(0, new_chunk);
+                    Some(new_chunk)
+                }
+                _ => None,
+            };
+
+            // Only insert the chunk if it's non-zero
+            match maybe_chunk {
+                Some(chunk) if chunk.get_amount() > Balance::zero() => {
+                    right.insert(0, chunk);
                 }
                 _ => (),
             }
@@ -775,18 +785,31 @@ where
         self.staked.subtract_amount(amount, era)
     }
 
+    // TODO: remove this
     /// Last era for which a stake entry exists.
     /// If no stake entries exist, returns `None`.
     pub fn last_stake_era(&self) -> Option<EraNumber> {
         self.staked.0.last().map(|chunk| chunk.era)
     }
 
+    // TODO: remove this
     /// First entry for which a stake entry exists.
     /// If no stake entries exist, returns `None`.
     pub fn oldest_stake_era(&self) -> Option<EraNumber> {
         self.staked.0.first().map(|chunk| chunk.era)
     }
 
+    // TODO
+    pub fn first_and_last_stake_chunks(&self) -> Option<(StakeChunk, StakeChunk)> {
+        let first = self.staked.0.first().map(|chunk| *chunk);
+        let last = self.staked.0.last().map(|chunk| *chunk);
+
+        match (first, last) {
+            (Some(first), Some(last)) => Some((first, last)),
+            _ => None,
+        }
+    }
+
     /// Notify ledger that all `stake` rewards have been claimed for the staked era.
     pub fn all_stake_rewards_claimed(&mut self) {
         // TODO: improve handling once bonus reward tracking is added