Skip to content

Commit

Permalink
Refactoring
Browse files Browse the repository at this point in the history
  • Loading branch information
@Dinonard
Dinonard committed Oct 25, 2023
1 parent e0f19e1 commit 4926838
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 141 deletions.
65 changes: 25 additions & 40 deletions pallets/dapp-staking-v3/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -945,14 +945,10 @@ pub mod pallet {
{
(staking_info, false)
}
// Entry exists but period doesn't match
Some(_) => (
SingularStakingInfo::new(
protocol_state.period_number(),
protocol_state.period_type(),
),
false,
),
// Entry exists but period doesn't match. Either reward should be claimed or cleaned up.
Some(_) => {
return Err(Error::<T>::UnclaimedRewardsFromPastPeriods.into());
}
// No entry exists
None => (
SingularStakingInfo::new(
Expand Down Expand Up @@ -1126,18 +1122,13 @@ pub mod pallet {
Self::ensure_pallet_enabled()?;
let account = ensure_signed(origin)?;

let protocol_state = ActiveProtocolState::<T>::get();
let mut ledger = Ledger::<T>::get(&account);

ensure!(
!ledger.staker_rewards_claimed,
Error::<T>::StakerRewardsAlreadyClaimed
);

// Check if the rewards have expired
let staked_period = ledger
.staked_period()
.ok_or(Error::<T>::NoClaimableRewards)?;

// Check if the rewards have expired
let protocol_state = ActiveProtocolState::<T>::get();
ensure!(
staked_period
>= protocol_state
Expand Down Expand Up @@ -1216,47 +1207,40 @@ pub mod pallet {
/// Used to claim bonus reward for the eligible era, if applicable.
#[pallet::call_index(12)]
#[pallet::weight(Weight::zero())]
pub fn claim_bonus_reward(origin: OriginFor<T>) -> DispatchResult {
pub fn claim_bonus_reward(
origin: OriginFor<T>,
smart_contract: T::SmartContract,
) -> DispatchResult {
Self::ensure_pallet_enabled()?;
let account = ensure_signed(origin)?;

let staker_info = StakerInfo::<T>::get(&account, &smart_contract)
.ok_or(Error::<T>::NoClaimableRewards)?;
let protocol_state = ActiveProtocolState::<T>::get();
let mut ledger = Ledger::<T>::get(&account);

// Check if period has ended
let staked_period = staker_info.period_number();
ensure!(
!ledger.bonus_reward_claimed,
Error::<T>::BonusRewardAlreadyClaimed
staked_period < protocol_state.period_number(),
Error::<T>::NoClaimableRewards
);
// Check if the rewards have expired
let staked_period = ledger
.staked_period()
.ok_or(Error::<T>::NoClaimableRewards)?;
ensure!(
staked_period
staker_info.is_loyal(),
Error::<T>::NotEligibleForBonusReward
);
ensure!(
staker_info.period_number()
>= protocol_state
.period_number()
.saturating_sub(T::RewardRetentionInPeriods::get()),
Error::<T>::RewardExpired
);

// Check if period has ended
ensure!(
staked_period < protocol_state.period_number(),
Error::<T>::NoClaimableRewards
);

// Check if user is applicable for bonus reward
let eligible_amount =
ledger
.claim_bonus_reward(staked_period)
.map_err(|err| match err {
AccountLedgerError::NothingToClaim => Error::<T>::NoClaimableRewards,
_ => Error::<T>::InternalClaimBonusError,
})?;
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.
ensure!(
!period_end_info.total_vp_stake.is_zero(),
Expand All @@ -1271,7 +1255,8 @@ pub mod pallet {
T::Currency::deposit_into_existing(&account, bonus_reward)
.map_err(|_| Error::<T>::InternalClaimStakerError)?;

Self::update_ledger(&account, ledger);
// Cleanup entry since the reward has been claimed
StakerInfo::<T>::remove(&account, &smart_contract);

Self::deposit_event(Event::<T>::BonusReward {
account: account.clone(),
Expand Down
43 changes: 16 additions & 27 deletions pallets/dapp-staking-v3/src/test/testing_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -848,30 +848,27 @@ 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 && pre_ledger.bonus_reward_claimed {
if is_full_claim {
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);
assert!(post_ledger.staked_future.is_none());
}
}

/// Claim staker rewards.
pub(crate) fn assert_claim_bonus_reward(account: AccountId) {
pub(crate) fn assert_claim_bonus_reward(account: AccountId, smart_contract: &MockSmartContract) {
let pre_snapshot = MemorySnapshot::new();
let pre_ledger = pre_snapshot.ledger.get(&account).unwrap();
let pre_staker_info = pre_snapshot
.staker_info
.get(&(account, *smart_contract))
.unwrap();
let pre_total_issuance = <Test as Config>::Currency::total_issuance();
let pre_free_balance = <Test as Config>::Currency::free_balance(&account);

let staked_period = pre_ledger
.staked_period()
.expect("Must have a staked period.");
let stake_amount = pre_ledger.staked_amount_for_type(PeriodType::Voting, staked_period);
let staked_period = pre_staker_info.period_number();
let stake_amount = pre_staker_info.staked_amount(PeriodType::Voting);

let period_end_info = pre_snapshot
.period_end
Expand All @@ -882,9 +879,10 @@ pub(crate) fn assert_claim_bonus_reward(account: AccountId) {
* period_end_info.bonus_reward_pool;

// Claim bonus reward & verify event
assert_ok!(DappStaking::claim_bonus_reward(RuntimeOrigin::signed(
account
),));
assert_ok!(DappStaking::claim_bonus_reward(
RuntimeOrigin::signed(account),
smart_contract.clone(),
));
System::assert_last_event(RuntimeEvent::DappStaking(Event::BonusReward {
account,
period: staked_period,
Expand All @@ -907,19 +905,10 @@ pub(crate) fn assert_claim_bonus_reward(account: AccountId) {
"Free balance must increase by the reward amount."
);

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);
}
assert!(
!StakerInfo::<Test>::contains_key(&account, smart_contract),
"Entry must be removed after successful reward claim."
);
}

/// Claim dapp reward for a particular era.
Expand Down
26 changes: 13 additions & 13 deletions pallets/dapp-staking-v3/src/test/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1179,7 +1179,7 @@ fn claim_staker_rewards_double_call_fails() {

assert_noop!(
DappStaking::claim_staker_rewards(RuntimeOrigin::signed(account)),
Error::<Test>::StakerRewardsAlreadyClaimed,
Error::<Test>::NoClaimableRewards,
);
})
}
Expand Down Expand Up @@ -1285,7 +1285,7 @@ fn claim_bonus_reward_works() {

// 1st scenario - advance to the next period, first claim bonus reward, then staker rewards
advance_to_next_period();
assert_claim_bonus_reward(account);
assert_claim_bonus_reward(account, &smart_contract);
for _ in 0..required_number_of_reward_claims(account) {
assert_claim_staker_rewards(account);
}
Expand All @@ -1297,10 +1297,10 @@ fn claim_bonus_reward_works() {
assert_claim_staker_rewards(account);
}
assert!(
Ledger::<Test>::get(&account).staker_rewards_claimed,
Ledger::<Test>::get(&account).staked.is_empty(),
"Sanity check."
);
assert_claim_bonus_reward(account);
assert_claim_bonus_reward(account, &smart_contract);
})
}

Expand All @@ -1320,11 +1320,11 @@ fn claim_bonus_reward_double_call_fails() {

// Advance to the next period, claim bonus reward, then try to do it again
advance_to_next_period();
assert_claim_bonus_reward(account);
assert_claim_bonus_reward(account, &smart_contract);

assert_noop!(
DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account)),
Error::<Test>::BonusRewardAlreadyClaimed,
DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account), smart_contract),
Error::<Test>::NoClaimableRewards,
);
})
}
Expand All @@ -1343,15 +1343,15 @@ fn claim_bonus_reward_when_nothing_to_claim_fails() {

// 1st - try to claim bonus reward when no stake is present
assert_noop!(
DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account)),
DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account), smart_contract),
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)),
DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account), smart_contract),
Error::<Test>::NoClaimableRewards,
);
})
Expand Down Expand Up @@ -1381,8 +1381,8 @@ fn claim_bonus_reward_with_only_build_and_earn_stake_fails() {

advance_to_next_period();
assert_noop!(
DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account)),
Error::<Test>::NoClaimableRewards,
DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account), smart_contract),
Error::<Test>::NotEligibleForBonusReward,
);
})
}
Expand All @@ -1406,7 +1406,7 @@ fn claim_bonus_reward_after_expiry_fails() {
advance_to_period(
ActiveProtocolState::<Test>::get().period_number() + reward_retention_in_periods,
);
assert_claim_bonus_reward(account);
assert_claim_bonus_reward(account, &smart_contract);
for _ in 0..required_number_of_reward_claims(account) {
assert_claim_staker_rewards(account);
}
Expand All @@ -1417,7 +1417,7 @@ fn claim_bonus_reward_after_expiry_fails() {
ActiveProtocolState::<Test>::get().period_number() + reward_retention_in_periods + 1,
);
assert_noop!(
DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account)),
DappStaking::claim_bonus_reward(RuntimeOrigin::signed(account), smart_contract),
Error::<Test>::RewardExpired,
);
})
Expand Down
64 changes: 3 additions & 61 deletions pallets/dapp-staking-v3/src/types.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,10 +299,6 @@ pub struct AccountLedger<
/// Both `stake` and `staked_future` must ALWAYS refer to the same period.
/// If `staked_future` is `Some`, it will always be **EXACTLY** one era after the `staked` field era.
pub staked_future: Option<StakeAmount>,
/// Indicator whether staker rewards for the entire period have been claimed.
pub staker_rewards_claimed: bool,
/// Indicator whether bonus rewards for the period has been claimed.
pub bonus_reward_claimed: bool,
/// Number of contract stake entries in storage.
#[codec(compact)]
pub contract_stake_count: u32,
Expand All @@ -319,8 +315,6 @@ where
unlocking: BoundedVec::default(),
staked: StakeAmount::default(),
staked_future: None,
staker_rewards_claimed: false,
bonus_reward_claimed: false,
contract_stake_count: Zero::zero(),
}
}
Expand Down Expand Up @@ -462,7 +456,6 @@ where
}

/// How much is staked for the specified period type, in respect to the specified era.
// TODO: if period is force-ended, this could be abused to get bigger rewards. I need to make the check more strict when claiming bonus rewards!
pub fn staked_amount_for_type(&self, period_type: PeriodType, period: PeriodNumber) -> Balance {
// First check the 'future' entry, afterwards check the 'first' entry
match self.staked_future {
Expand Down Expand Up @@ -643,10 +636,6 @@ where
era: EraNumber,
period_end: Option<EraNumber>,
) -> Result<EraStakePairIter, AccountLedgerError> {
if self.staker_rewards_claimed {
return Err(AccountLedgerError::AlreadyClaimed);
}

// Main entry exists, but era isn't 'in history'
if !self.staked.is_empty() && era <= self.staked.era {
return Err(AccountLedgerError::NothingToClaim);
Expand Down Expand Up @@ -682,64 +671,17 @@ where
}
self.staked.era = era.saturating_add(1);

// Make sure to clean up the entries
// Make sure to clean up the entries if all rewards for the period have been claimed.
match period_end {
Some(ending_era) if era >= ending_era => {
self.staker_rewards_claimed = true;
self.maybe_cleanup_stake_entries();
self.staked = Default::default();
self.staked_future = None;
}
_ => (),
}

Ok(result)
}

/// Claim bonus reward, if possible.
///
/// Returns the amount eligible for bonus reward calculation, or an error.
pub fn claim_bonus_reward(
&mut self,
period: PeriodNumber,
) -> Result<Balance, AccountLedgerError> {
if self.bonus_reward_claimed {
return Err(AccountLedgerError::AlreadyClaimed);
}

let amount = self.staked_amount_for_type(PeriodType::Voting, period);

if amount.is_zero() {
Err(AccountLedgerError::NothingToClaim)
} else {
self.bonus_reward_claimed = true;
self.maybe_cleanup_stake_entries();
Ok(amount)
}
}

/// Cleanup fields related to stake & rewards, in case all possible rewards for the current state
/// have been claimed.
fn maybe_cleanup_stake_entries(&mut self) {
// Stake rewards are either all claimed, or there's nothing to claim.
let stake_cleanup =
self.staker_rewards_claimed || (self.staked.is_empty() && self.staked_future.is_none());

// Bonus reward might be covered by the 'future' entry.
let has_future_entry_stake = if let Some(stake_amount) = self.staked_future {
!stake_amount.voting.is_zero()
} else {
false
};
// Either rewards have already been claimed, or there are no possible bonus rewards to claim.
let bonus_cleanup =
self.bonus_reward_claimed || self.staked.voting.is_zero() && !has_future_entry_stake;

if stake_cleanup && bonus_cleanup {
self.staked = Default::default();
self.staked_future = None;
self.staker_rewards_claimed = false;
self.bonus_reward_claimed = false;
}
}
}

/// Helper internal struct for iterating over `(era, stake amount)` pairs.
Expand Down

0 comments on commit 4926838

Please sign in to comment.