Claim improvements & some tests

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

@@ -352,13 +352,15 @@ pub mod pallet {
             }
 
             let mut era_info = CurrentEraInfo::<T>::get();
-            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 current_era = protocol_state.era;
             let next_era = current_era.saturating_add(1);
-            let maybe_period_event = match protocol_state.period_type() {
+            let (maybe_period_event, era_reward) = match protocol_state.period_type() {
                 PeriodType::Voting => {
-                    // For the sake of consistency
+                    // For the sake of consistency, we put zero reward into storage
+                    let era_reward =
+                        EraReward::new(Balance::zero(), era_info.total_staked_amount());
+
                     let ending_era =
                         next_era.saturating_add(T::StandardErasPerBuildAndEarnPeriod::get());
                     let build_and_earn_start_block =
@@ -367,13 +369,20 @@ pub mod pallet {
 
                     era_info.migrate_to_next_era(Some(protocol_state.period_type()));
 
-                    Some(Event::<T>::NewPeriod {
-                        period_type: protocol_state.period_type(),
-                        number: protocol_state.period_number(),
-                    })
+                    (
+                        Some(Event::<T>::NewPeriod {
+                            period_type: protocol_state.period_type(),
+                            number: protocol_state.period_number(),
+                        }),
+                        era_reward,
+                    )
                 }
                 PeriodType::BuildAndEarn => {
-                    // TODO: trigger reward calculation here. This will be implemented later.
+                    // TODO: trigger dAPp tier reward calculation here. This will be implemented later.
+
+                    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());
 
                     // Switch to `Voting` period if conditions are met.
                     if protocol_state.period_info.is_next_period(next_era) {
@@ -400,17 +409,20 @@ pub mod pallet {
 
                         // TODO: trigger tier configuration calculation based on internal & external params.
 
-                        Some(Event::<T>::NewPeriod {
-                            period_type: protocol_state.period_type(),
-                            number: protocol_state.period_number(),
-                        })
+                        (
+                            Some(Event::<T>::NewPeriod {
+                                period_type: protocol_state.period_type(),
+                                number: protocol_state.period_number(),
+                            }),
+                            era_reward,
+                        )
                     } else {
                         let next_era_start_block = now.saturating_add(T::StandardEraLength::get());
                         protocol_state.next_era_start = next_era_start_block;
 
                         era_info.migrate_to_next_era(None);
 
-                        None
+                        (None, era_reward)
                     }
                 }
             };
@@ -1022,15 +1034,16 @@ pub mod pallet {
                 .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)?;
+                    .ok_or(Error::<T>::NoClaimableRewards)?;
 
             // 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)
+            // And indicator if we know the period's ending era.
+            let (last_period_era, period_end) = if staked_period == protocol_state.period_number() {
+                (protocol_state.era.saturating_sub(1), None)
             } else {
                 PeriodEnd::<T>::get(&staked_period)
+                    .map(|info| (info.final_era, Some(info.final_era)))
                     .ok_or(Error::<T>::InternalClaimStakerError)?
-                    .final_era
             };
 
             // The last era for which we can claim rewards for this account.
@@ -1048,23 +1061,19 @@ pub mod pallet {
             };
 
             // Get chunks for reward claiming
-            let chunks_for_claim = ledger
-                .staked
-                .left_split(last_claim_era)
-                .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();
+            let chunks_for_claim =
+                ledger
+                    .claim_up_to_era(last_claim_era, period_end)
+                    .map_err(|err| match err {
+                        AccountLedgerError::SplitEraInvalid => Error::<T>::NoClaimableRewards,
+                        _ => Error::<T>::InternalClaimStakerError,
+                    })?;
 
             // Calculate rewards
             let mut rewards: Vec<_> = Vec::new();
             let mut reward_sum = Balance::zero();
             for era in first_chunk.get_era()..=last_claim_era {
+                // TODO: this should be zipped, and values should be fetched only once
                 let era_reward = era_rewards
                     .get(era)
                     .ok_or(Error::<T>::InternalClaimStakerError)?;
@@ -1073,23 +1082,22 @@ pub mod pallet {
                     .get(era)
                     .ok_or(Error::<T>::InternalClaimStakerError)?;
 
-                let staker_reward = Perbill::from_rational(chunk.get_amount(), era_reward.staked())
-                    * era_reward.staker_reward_pool();
-                if staker_reward.is_zero() {
+                // Optimization, and zero-division protection
+                if chunk.get_amount().is_zero() || era_reward.staked().is_zero() {
                     continue;
                 }
+                let staker_reward = Perbill::from_rational(chunk.get_amount(), era_reward.staked())
+                    * era_reward.staker_reward_pool();
+
                 rewards.push((era, staker_reward));
                 reward_sum.saturating_accrue(staker_reward);
             }
 
-            // Write updated ledger back to storage
-            if is_full_period_claimed {
-                ledger.all_stake_rewards_claimed();
-            }
-            Self::update_ledger(&account, ledger);
-
             T::Currency::deposit_into_existing(&account, reward_sum)
                 .map_err(|_| Error::<T>::InternalClaimStakerError)?;
+
+            Self::update_ledger(&account, ledger);
+
             rewards.into_iter().for_each(|(era, reward)| {
                 Self::deposit_event(Event::<T>::Reward {
                     account: account.clone(),

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

@@ -155,7 +155,6 @@ impl ExtBuilder {
         let mut ext = TestExternalities::from(storage);
         ext.execute_with(|| {
             System::set_block_number(1);
-            DappStaking::on_initialize(System::block_number());
 
             // TODO: not sure why the mess with type happens here, I can check it later
             let era_length: BlockNumber =
@@ -175,6 +174,8 @@ impl ExtBuilder {
                 },
                 maintenance: false,
             });
+
+            // DappStaking::on_initialize(System::block_number());
         });
 
         ext

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

@@ -817,6 +817,10 @@ pub(crate) fn assert_claim_staker_rewards(account: AccountId) {
 
         let reward = Perbill::from_rational(stake_chunk.amount, era_reward_info.staked())
             * era_reward_info.staker_reward_pool();
+        if reward.is_zero() {
+            continue;
+        }
+
         rewards.push((era, reward));
     }
     let total_reward = rewards

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

@@ -19,8 +19,8 @@
 use crate::test::mock::*;
 use crate::test::testing_utils::*;
 use crate::{
-    pallet as pallet_dapp_staking, ActiveProtocolState, DAppId, EraNumber, Error, IntegratedDApps,
-    Ledger, NextDAppId, PeriodType, StakerInfo,
+    pallet as pallet_dapp_staking, ActiveProtocolState, DAppId, EraNumber, EraRewards, Error,
+    IntegratedDApps, Ledger, NextDAppId, PeriodType, StakerInfo,
 };
 
 use frame_support::{assert_noop, assert_ok, error::BadOrigin, traits::Get};
@@ -1264,10 +1264,49 @@ fn claim_staker_rewards_basic_example_is_ok() {
         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);
         }
     })
 }
+
+#[test]
+fn claim_staker_rewards_no_claimable_rewards_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 scenario - try to claim with no stake at all.
+        assert_noop!(
+            DappStaking::claim_staker_rewards(RuntimeOrigin::signed(account)),
+            Error::<Test>::NoClaimableRewards,
+        );
+
+        // 2nd scenario - stake some amount, and try to claim in the same era.
+        // It's important this is the 1st era, when no `EraRewards` entry exists.
+        assert_eq!(ActiveProtocolState::<Test>::get().era, 1, "Sanity check");
+        assert!(EraRewards::<Test>::iter().next().is_none(), "Sanity check");
+        let stake_amount = 93;
+        assert_stake(account, &smart_contract, stake_amount);
+        assert_noop!(
+            DappStaking::claim_staker_rewards(RuntimeOrigin::signed(account)),
+            Error::<Test>::NoClaimableRewards,
+        );
+
+        // 3rd scenario - move over to the next era, but we still expect failure because
+        // stake is valid from era 2 (current era), and we're trying to claim rewards for era 1.
+        advance_to_next_era();
+        assert!(EraRewards::<Test>::iter().next().is_some(), "Sanity check");
+        assert_noop!(
+            DappStaking::claim_staker_rewards(RuntimeOrigin::signed(account)),
+            Error::<Test>::NoClaimableRewards,
+        );
+    })
+}

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

@@ -392,6 +392,19 @@ fn sparse_bounded_amount_era_vec_split_left_works() {
         "Amount must come from last entry in the split."
     );
     assert_eq!(vec.0[1], DummyEraAmount::new(6, 6));
+
+    // 5th scenario: [1,2(0)]  -- split(4) --> [1,2],[]
+    let vec: Vec<DummyEraAmount> = vec![(1, 1), (0, 2)]
+        .into_iter()
+        .map(|(amount, era)| DummyEraAmount::new(amount as Balance, era))
+        .collect();
+    let mut vec = SparseBoundedAmountEraVec::<_, MaxLen>(BoundedVec::try_from(vec).unwrap());
+    let result = vec.left_split(4).expect("Split should succeed.");
+    assert_eq!(result.0.len(), 2);
+    assert_eq!(result.0[0], DummyEraAmount::new(1, 1));
+    assert_eq!(result.0[1], DummyEraAmount::new(0, 2));
+
+    assert!(vec.0.is_empty());
 }
 
 #[test]

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

@@ -244,11 +244,7 @@ where
     ///  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.
-
         // Split the inner vector into two parts
         let (left, mut right): (Vec<P>, Vec<P>) = self
             .0
@@ -810,11 +806,38 @@ where
         }
     }
 
-    /// 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
-        self.staked = SparseBoundedAmountEraVec(BoundedVec::<StakeChunk, StakedLen>::default());
-        self.staked_period = None;
+    /// Claim up stake chunks up to the specified `era`.
+    /// Returns the vector describing claimable chunks.
+    ///
+    /// If `period_end` is provided, it's used to determine whether all applicable chunks have been claimed.
+    pub fn claim_up_to_era(
+        &mut self,
+        era: EraNumber,
+        period_end: Option<EraNumber>,
+    ) -> Result<SparseBoundedAmountEraVec<StakeChunk, StakedLen>, AccountLedgerError> {
+        let claim_chunks = self.staked.left_split(era)?;
+
+        // Check if all possible chunks have been claimed
+        match self.staked.0.first() {
+            Some(chunk) => {
+                match period_end {
+                    // If first chunk is after the period end, clearly everything has been claimed
+                    Some(period_end) if chunk.get_era() > period_end => {
+                        self.staked_period = None;
+                        self.staked = SparseBoundedAmountEraVec::new();
+                    }
+                    _ => (),
+                }
+            }
+            // No more chunks remain, meaning everything has been claimed
+            None => {
+                self.staked_period = None;
+            }
+        }
+
+        // TODO: this is a bit clunky - introduce variables instead that keep track whether rewards were claimed or not.
+
+        Ok(claim_chunks)
     }
 }