AstarNetwork/Astar

dAppStaking - Bonus rewards mechanism rework

Opened this issue · 6 comments

ipapandinas commented

Context

This issue proposes improvements to the bonus rewards mechanism in dAppStaking to enhance user flexibility and align with community feedback.

  • "The user is not free to move their stake during the Build&Earn subperiod: stake and forget, making it difficult to arbitrate, especially with the dynamic thresholds." – source
  • "The current bonus system is completely incompatible with the dynamic threshold system introduced recently." – source

Suggested Solutions

Introduce a new move extrinsic that enables users to reallocate their staked voting bonus between dApps while preserving the bonus reward.

This new extrinsic will support two main use cases:

  • a configurable number MaxBonusMovesPerPeriod of maximum allowed regular moves per period (2 by default) to allow limited bonus reallocation during the Build&Earn subperiod. Using move during the Voting subperiod, reallocates the voting stake without increasing the move counter.
  • a free reallocation if a dApp is unregistered (no impact on user’s move count),

Nov 6 EDIT: From discussion below
Dec 11 EDIT: Partial unstacking should be treated as a move to no dApp (reduce SafeMovesRemaining counter - check scenario 3)

Interface Proposal

The new move extrinsic

pub fn move(
    origin: OriginFor<T>,
    from_smart_contract: T::SmartContract,
    to_smart_contract: T::SmartContract,
    amount: Balance
) -> DispatchResult

The new config params

  • TempBonusMovesForOngoingPeriod - used in the ongoing period
  • MaxBonusMovesPerPeriod - use in the next period

The updated SingularStakingInfo struct

struct SingularStakingInfo {
    previous_staked: StakeAmount,
    staked: StakeAmount,
    bonus_status: BonusStatus,
}

The bonus status definition

enum BonusStatus {
  BonusForfeited,
  SafeMovesRemaining(u8),
}
  • The initial value of SafeMovesRemaining will be set to MaxBonusMovesPerPeriod in each period, with an initial default set to TempBonusMovesForOngoingPeriod of 1 for the ongoing period.
  • The BonusForfeited and SafeMovesRemaining(_) are respectively encoded to false and true to avoid a storage migration.

Logic Steps and Checks

  1. Validate Origin
  2. Confirm that from_smart_contract and to_smart_contract are distinct.
  3. Verify to_smart_contract registration
  4. Check amount to move:
    • Check if it is not too small/large
    • Ensure the specified amount does not exceed from_smart_contract’s staked balance
  5. Handle bonus_status:
    • If from_smart_contract is unregistered, maintain the bonus_status.
    • If to_smart_contract is registered and SafeMovesRemaining > 0, decrement SafeMovesRemaining.
    • If SafeMovesRemaining reaches zero, set bonus_status to BonusForfeited.
  6. Staking Changes
    • Unstake from from_smart_contract using existing unstake logic.
    • Stake on to_smart_contract using existing stake logic.
    • Check correct storage updates.
  7. Emit Move event with: account / from_smart_contract / to_smart_contract / amount / bonus_status

💡 Note: Previous nomination_transfer from dAppStaking v2 can be used as reference

⚠️ Note: a partial unstake must also be adapted to act as if move to no dApp was used
This decrease the SafeMovesRemaining counter, preserving the bonus for the remaining stake.
However, by using the move extrinsic you actually move your voting stake to a new (or an already staked) dApp, and your bonus is still preserved on both stakes.

Some expected outcomes scenarios

Stake reallocation to a new or previously unstaked dApp

// dApp 1
SingularStakingInfo {
    previous_staked: X,
    staked: X,
    bonus_status: SafeMovesRemaining(1),
}

// MOVE 1: dApp1 -> dApp2

// dApp 2
SingularStakingInfo {
    previous_staked: X,
    staked: X,
    bonus_status: SafeMovesRemaining(0),
}

// MOVE 2: dApp2 -> dApp3

// dApp 3
SingularStakingInfo {
    previous_staked: X,
    staked: X,
    bonus_status: BonusForfeited,
}

Bonus migration between already staked dApps

// dApp 1
SingularStakingInfo {
    previous_staked: X,
    staked: X,
    bonus_status: SafeMovesRemaining(1),
}

// MOVE 1: dApp1 -> dApp2 (where Y is original stake on dApp2 & Z is all or some stake of dApp1)

// dApp 2
SingularStakingInfo {
    previous_staked: Y + Z,
    staked: Y + Z,
    bonus_status: SafeMovesRemaining(0),
}

Partial unstacking should be treated as a move to no dApp

// dApp 1
SingularStakingInfo {
    previous_staked: X,
    staked: X,
    bonus_status: SafeMovesRemaining(1),
}

// UNSTAKE 1: Y = X - unstake1_amount

// dApp 1
SingularStakingInfo {
    previous_staked: Y,
    staked: Y,
    bonus_status: SafeMovesRemaining(0),
}

// UNSTAKE 2: Z = Y - unstake2_amount

// dApp 1
SingularStakingInfo {
    previous_staked: Z,
    staked: Z,
    bonus_status: BonusForfeited,
}

Consistency Steps

  • Prepare and test a multi-block storage migration to support new bonus_status and remove loyal_staker from SingularStakingInfo
  • Some unit tests ideas - Cap Logic Checks:
    - Verify that the move counter resets appropriately at the start of each new period
    - Prevents moves beyond the configurable cap
    - Verify that the move counter is not increased for unregistered dApps
    - Verify that the move counter is not increased for moves during the Voting subperiod
  • More testing scenarios:
    • A staker reallocates bonuses multiple times
    • A staker migrates bonuses from an unregistered dApp
    • A staker moves partial stake
  • Documentation

Ensure that the actual bonus reward calculation (also for unregistered dApps) & staking / unstaking / claiming mechanisms still work.

Dinonard commented

@ipapandinas I would suggest a bit different solution which should be technically simpler & better for the user IMO.

The bonus reward or the loyalty flag is tied to a particular stake, not the account.
E.g. if Alice stakes on 1 dApp, she must keep the Build&Earn stake to get the bonus. If she goes below that amount at any point, she looses 100% of the reward.

On the other side, Bob stakes on 3 dApps, using the same amount for the sake of simplicity. If he at any points e.g. decides to unstake from 1 dApp, he will loose only 1/3 of the bonus, retaining the other 2/3. So Bob's stake is more "robust".

Therefore I'd suggest to track whether the stake can be moved on the level of singular stake.
It is more fair to the users who support multiple dApps compared to just one.

Tech Solution Suggestion

AccountLedger should be kept as-is.
Instead, we should just modify the SingularStakingInfo. This is how it looks today.

#[derive(Encode, Decode, MaxEncodedLen, Copy, Clone, Debug, PartialEq, Eq, TypeInfo, Default)]
pub struct SingularStakingInfo {
    /// Amount staked before, if anything.
    pub(crate) previous_staked: StakeAmount,
    /// Staked amount
    pub(crate) staked: StakeAmount,
    /// Indicates whether a staker is a loyal staker or not.
    pub(crate) loyal_staker: bool,
}

We can modify it to something like:

#[derive(Encode, Decode, MaxEncodedLen, Copy, Clone, Debug, PartialEq, Eq, TypeInfo, Default)]
pub struct SingularStakingInfo {
    /// Amount staked before, if anything.
    pub(crate) previous_staked: StakeAmount,
    /// Staked amount
    pub(crate) staked: StakeAmount,
    /// Indicates whether a staker is a loyal staker or not.
    pub(crate) period_stake: PeriodStake
}

enum PeriodStake {
  /// Bonus for this stake has been forfeited, staker won't receive anything
  BonusForfeited,
  /// Staker can move some or all stake one more time without loosing the reward.
  /// This value would be deprecated later.
  OneMoveRemaining,
  /// The amount of safe 'moves' remaning, where staker can move all or part of this stake to another dApp
  SafeMovesRemaining(u8),
}

The benefit of this approach is that we don't need to do any storage migration since false will encode the the BonusForfeited and true will encode to OneMoveRemaining.
Since we'd be deploying this in the mid of the ongoing period, having only one move remaining shouldn't be an issue.
Later we'd use SafeMovesRemaining(u8) which would use the constant you mentioned.

ipapandinas commented

Thank you for the insights! I agree that tracking moves at the singular stake level makes sense for improving user fairness and control, especially for those supporting multiple dApps.

Your suggested design is also very clever—I really like it! I propose simplifying the enum slightly:

enum BonusStatus {
  BonusForfeited,
  SafeMovesRemaining(u8),
}

With SafeMovesRemaining(_) encoded to true and BonusForfeited encoded to false as you suggested; I've removed OneMoveRemaining to keep it straightforward.

The initial value for SafeMovesRemaining(u8) will be set by MaxBonusMovesPerPeriod from the config, with a starting at 1 for the ongoing period. We can then upgrade the runtime to allow 2 moves with the next period later.

Here are the expected outcomes for the given scenarios:

Stake reallocation to a newly registered or previously unstaked dApp

The bonus decreases or is forfeited if moves are exhausted. For example:

// dApp 1
SingularStakingInfo {
    previous_staked: X,
    staked: X,
    bonus_status: SafeMovesRemaining(1),
}

// MOVE 1: dApp1 -> dApp2

// dApp 2
SingularStakingInfo {
    previous_staked: X,
    staked: X,
    bonus_status: SafeMovesRemaining(0),
}

// MOVE 2: dApp2 -> dApp3

// dApp 3
SingularStakingInfo {
    previous_staked: X,
    staked: X,
    bonus_status: BonusForfeited,
}

Bonus migration within already staked dApps:

Combine existing stake and reduce move count accordingly.

// dApp 1
SingularStakingInfo {
    previous_staked: X,
    staked: X,
    bonus_status: SafeMovesRemaining(1),
}

// MOVE 1: dApp1 -> dApp2 (Y represents the original stake on dApp2)

// dApp 2
SingularStakingInfo {
    previous_staked: Y + X,
    staked: Y + X,
    bonus_status: SafeMovesRemaining(0),
}

WDYT? I’ll continue updating the original issue body to clarify the implementation as we discuss.

Dinonard commented

For the enum simplification, I'd prefer if it was as you suggested, if it works & makes life simpler.
My idea was to just initially go with this enum value, without using it in the future (deprecating it).
Best to also check with the UI team if it causes them problems/complications.

The initial value for SafeMovesRemaining(u8) will be set by MaxBonusMovesPerPeriod from the config, with a starting at 1 for the ongoing period. We can then upgrade the runtime to allow 2 moves with the next period later.

We shouldn't need to do runtime upgrade to change this value though - it's unnecessarily complex and will be something we need to keep track of & prepare. Running it through the governance will also consume time.
If we want to have a different value to start with, best to define it via an additional pallet config type.

All of that being said - we have multiblock migration now, it's simple to write & test, so might as well as do it I guess 🤷‍♂️.
It's not the worst thing to do :)

For the move call, I believe users will expect that they can specify the move amount.
This will make the logic a bit more complex.

ipapandinas commented

If we want to have a different value to start with, best to define it via an additional pallet config type.

Just to confirm I've captured your idea, are you suggesting of having two new config params? Something like:

  • TempBonusMovesForPeriod3 - used in the ongoing period
  • MaxBonusMovesPerPeriod - use in the next period

Here is a first interface proposal and logic outline for the move extrinsic:

pub fn move(
    origin: OriginFor<T>,
    from_smart_contract: T::SmartContract,
    to_smart_contract: T::SmartContract,
    amount: Option<Balance>
) -> DispatchResult

Logic: steps and checks

  1. Validate Origin
  2. Check to_smart_contract registration
  3. Check amount to move:
    • Check if it is not too small/large
    • Ensure the specified amount does not exceed from_smart_contract’s staked balance
  4. Prepare bonus_status:
    • If from_smart_contract is not registered, maintain the bonus_status value when transferring singular stake.
    • If to_smart_contract is registered and SafeMovesRemaining > 0, decrement SafeMovesRemaining for to_smart_contract.
    • If SafeMovesRemaining reaches zero, set bonus_status to BonusForfeited.
  5. Perform stake transfer
    • If amount is None, transfer everything from from_smart_contract to to_smart_contract, removing SingularStakingInfo for from_smart_contract
    • If the remaining stake amount in from_smart_contract is lower than the MinimumStakeAmount, move everything and remove SingularStakingInfo too.
    • Add the amount to to_smart_contract's staked balance
    • Update, Create or Remove SingularStakingInfo for both contracts
  6. Emit Move event: account / from_smart_contract / to_smart_contract / amount / bonus_status
Dinonard commented

Just to confirm I've captured your idea, are you suggesting of having two new config params? Something like:

TempBonusMovesForPeriod3 - used in the ongoing period
MaxBonusMovesPerPeriod - use in the next period

In case we decide to take the route without migration & without the dummy value OneMoveRemaining, then yes.
We should avoid relying on us doing another timely runtime upgrade in the future just to change a config params.

Few comments:

  • If amount is None,

    • I understand the idea, but no need to complicate the interface - let the UI handle that.
  • The logic overall looks good! Technical detail - we've had nomination_transfer call in the old dApp staking v2. The way I implemented it was by reusing the stake/unstake logic. The move was essentially divided into unstake, followed by stake. That way there's very little new code added (small tip 🙂).
ipapandinas commented

Perfect! I've edited the original issue's body accordingly