feat(dAppStaking): Move actions mechanisms for bonus rewards #1402

ipapandinas · 2025-01-03T18:41:42Z

⚠️ A fix for the code coverage job is proposed here: #1408

Pull Request Summary

This PR introduces enhancements to the bonus reward eligibility mechanism and implements robust handling for stake movement, particularly during the build&earn subperiod.

Bonus Reward Eligibility

The previously used loyal_staker flag is replaced with and decoded into a bonus_status enum:

enum BonusStatus {
    /// Bonus rewards are forfeited.
    BonusForfeited,
    /// Bonus rewards are preserved with a value which is the number of remaining 'safe moves' allowed in the ongoing B&E subperiod.
    SafeMovesRemaining(u8),
}

This update:

Introduces tracking of bonus status to ensure eligibility based on the number of allowed move actions (MaxBonusMovesPerPeriod).
Enforces forfeiture of bonus eligibility if move actions exceed the allowable threshold.

Move action

A move action is defined as either:

Partial unstaking that reduces the 'voting stake'.
Transfers of 'voting stake' between contracts.

If a move action is performed during the build&earn subperiod, the bonus_status attached to the relevant singular staking info is reduced until it is forfeited.

Stake Movement Enhancements

A new move_stake extrinsic is implemented. It is similar to an 'unstake' followed by two 'stakes' for each subperiod. Existing safeguards are extended additional checks to:

Ensure the source and destination contracts are different.
Validate the amount to be moved.

When moving stake from unregistered contracts, the bonus status is always preserved.

Migration

A multi-block migration is proposed to convert all existing loyal_staker flags to their respective BonusStatus variants:

BonusStatus::SafeMovesRemaining(0) for true.
BonusStatus::BonusForfeited for false.

Closes #1379

Check list

added or updated unit tests
updated Astar official documentation
added OnRuntimeUpgrade hook for precompile revert code registration
added benchmarks & weights for any modified runtime logics.
add E2E tests
prepare follow-up issue for Astar Portal frontend & forum post

ipapandinas · 2025-01-13T18:21:34Z

/bench shibuya-dev pallet_dapp_staking

github-actions · 2025-01-13T18:21:54Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/12753557202.
Please wait for a while.
Branch: feat/move_bonus_dapp_staking
SHA: d94b330

github-actions · 2025-01-13T19:01:16Z

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/12753557202.

Dinonard

Did a high level review of the most important components.

Overall impression is that we should simplify the solution.
This adds a lot of new code, that is a functional duplicate of what we already have.

Dinonard · 2025-01-15T12:13:09Z

pallets/dapp-staking/README.md

@@ -157,7 +160,23 @@ If unstake would reduce the staked amount below `MinimumStakeAmount`, everything

 Once period finishes, all stakes are reset back to zero. This means that no unstake operation is needed after period ends to _unstake_ funds - it's done automatically.

-If dApp has been unregistered, a special operation to unstake from unregistered contract must be used.
+During the `build&earn` subperiod, if unstaking reduces the voting stake, the bonus status will be updated, and the number of allowed _move actions_ for the ongoing period will be reduced.


Minor comment - for the sake of consistency, let's go with Build&Earn.

Dinonard · 2025-01-15T12:13:58Z

pallets/dapp-staking/README.md

+
+-   The destination contract must be different from the source contract.
+-   The user must ensure that unclaimed rewards are claimed before initiating a stake move.
+-   Only a limited number of move actions (defined by `MaxBonusMovesPerPeriod`) are allowed during the `build&earn` subperiod to preserve bonus reward eligibility (check "Claiming Bonus Reward" section below).


Suggestion - "allowed during a single period"

Dinonard · 2025-01-15T12:15:25Z

pallets/dapp-staking/src/lib.rs

+        /// retaining eligibility for bonus rewards. Exceeding this limit will result in the
+        /// forfeiture of the bonus rewards for the affected stake.
+        #[pallet::constant]
+        type MaxBonusMovesPerPeriod: Get<u8> + Default + Debug + Clone;


Why are Default + Debug + Clone needed?

Dinonard · 2025-01-15T12:16:37Z

pallets/dapp-staking/src/lib.rs

@@ -290,7 +296,7 @@ pub mod pallet {
            era: EraNumber,
            amount: Balance,
        },
-        /// Bonus reward has been paid out to a loyal staker.
+        /// Bonus reward has been paid out to a 'loyal' staker.


Maybe we can change the nomenclature to something like "engaged" or "early".

Dinonard · 2025-01-15T12:21:22Z

pallets/dapp-staking/src/lib.rs

+            origin: OriginFor<T>,
+            source_contract: T::SmartContract,
+            destination_contract: T::SmartContract,
+            maybe_amount: Option<Balance>,


My suggestion is to remove this because it's something that can easily be handled by the outside logic.

Reading the staked amount is easy for the frontend part, so no need to have any special logic in runtime to handle this. In case of unregistered contract - the whole amount can simply be ignored and moved to the new contract.

Dinonard · 2025-01-15T12:30:36Z

pallets/dapp-staking/src/types.rs

+pub enum BonusStatus<MaxBonusMoves: Get<u8>> {
+    /// Bonus rewards are forfeited.
+    BonusForfeited,
+    /// Bonus rewards are preserved with a value which is the number of remaining 'safe moves' allowed in the ongoing B&E subperiod.
+    SafeMovesRemaining(u8),
+    #[codec(skip)]
+    _Phantom(PhantomData<MaxBonusMoves>),
+}


At the moment we have issues with MBM migrations, that's why we didn't go with async backing on Astar yet.

If this approach is adopted, we'll be blocked with the feature until an adequate fix is provided for that issue.

However, taking the approach with additional enum value, OneMoveRemaining solves the need for any migration whatsoever. Only the "storage version" needs to be bumped.

EDIT:
Or even better - why not just pure u8 here (ofc, it can be wrapped into a struct)? Call the variable "moves_remaining" and that's it?

Dinonard · 2025-01-15T12:33:20Z

pallets/dapp-staking/src/types.rs

+        // Bonus staking is only possible if stake is first made during the voting subperiod.
+        let bonus_status = if subperiod == Subperiod::Voting {
+            BonusStatus::<MaxBonusMoves>::default()
+        } else {
+            BonusStatus::<MaxBonusMoves>::BonusForfeited
+        };


If this has to be decided at this point - it might be better to provide the value from the outside.

Current approach seems hacky & incomplete.

Dinonard · 2025-01-15T12:36:03Z

pallets/dapp-staking/src/types.rs

+    /// Transfers stake between contracts while maintaining subperiod-specific allocations, era consistency, and bonus-safe conditions.
+    pub fn move_stake(
+        &mut self,
+        destination: &mut SingularStakingInfo<MaxBonusMoves>,
+        amount: Balance,
+        current_era: EraNumber,
+        subperiod: Subperiod,
+    ) -> (Vec<(EraNumber, Balance)>, StakeAmountMoved) {
+        let staked_snapshot = self.staked;
+        let era_and_amount_pairs = self.unstake(amount, current_era, subperiod);
+
+        let voting_stake_moved = staked_snapshot.voting.saturating_sub(self.staked.voting);
+        let build_earn_stake_moved = staked_snapshot
+            .build_and_earn
+            .saturating_sub(self.staked.build_and_earn);
+
+        // Similar to a stake on destination but for the 2 subperiods
+        destination.previous_staked = destination.staked;
+        destination.previous_staked.era = current_era;
+        if destination.previous_staked.total().is_zero() {
+            destination.previous_staked = Default::default();
+        }
+        destination
+            .staked
+            .add(voting_stake_moved, Subperiod::Voting);
+        destination
+            .staked
+            .add(build_earn_stake_moved, Subperiod::BuildAndEarn);
+
+        // Moved stake is only valid from the next era so we keep it consistent here
+        destination.staked.era = current_era.saturating_add(1);
+
+        (
+            era_and_amount_pairs,
+            StakeAmountMoved {
+                voting: voting_stake_moved,
+                build_and_earn: build_earn_stake_moved,
+            },
+        )
+    }


IMHO, this method should be removed.

The move operation is a combination of unstake and stake.
Nothing more.

We should reuse that code as best we can.
If it means modifying the methods footprint(s) - it's fine.
We have plenty of tests for stake & unstake operations, and these should continue to work as they have before.

Dinonard · 2025-01-15T12:39:24Z

pallets/dapp-staking/src/lib.rs

+            });
+
+            Ok(())
+        }


General comment about the extrinsic call - we should reuse code more.
This is essentially unstake and stake.

Logic should be extracted from those calls, and reused here.
Something like this: https://github.com/AstarNetwork/astar-frame/blob/polkadot-v0.9.39/frame/dapps-staking/src/pallet/mod.rs#L652

feat: BonusStatus implementation

6b31233

ipapandinas added enhancement New feature or request dapps-staking Dapps Staking labels Jan 3, 2025

ipapandinas added 3 commits January 10, 2025 17:28

feat: move_stake extrinsic implementation

6e6c56b

task: v9 mb migration for bonus-status

df12ac8

task: README updated

d94b330

ipapandinas marked this pull request as ready for review January 13, 2025 18:19

ipapandinas changed the title ~~WIP - feat(dAppStaking): Move actions mechanisms for bonus rewards~~ feat(dAppStaking): Move actions mechanisms for bonus rewards Jan 13, 2025

ipapandinas added 2 commits January 13, 2025 21:32

fix: tests fixed and weights updated

68edd56

task: remove custom 'equals' method

d8045f5

ipapandinas added shibuya related to shibuya runtime This PR/Issue is related to the topic “runtime”. and removed shibuya related to shibuya labels Jan 13, 2025

ipapandinas added 2 commits January 14, 2025 01:19

task: maintenance mode enable during migration

691b60d

task: update rust-toolchain

ce5d89a

Dinonard reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dAppStaking): Move actions mechanisms for bonus rewards #1402

feat(dAppStaking): Move actions mechanisms for bonus rewards #1402

ipapandinas commented Jan 3, 2025 •

edited

Loading

ipapandinas commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

Dinonard left a comment

Dinonard Jan 15, 2025

Dinonard Jan 15, 2025

Dinonard Jan 15, 2025

Dinonard Jan 15, 2025

Dinonard Jan 15, 2025

Dinonard Jan 15, 2025

Dinonard Jan 15, 2025

Dinonard Jan 15, 2025

Dinonard Jan 15, 2025

+                          });
+                          Ok(())
+                      }

feat(dAppStaking): Move actions mechanisms for bonus rewards #1402

Are you sure you want to change the base?

feat(dAppStaking): Move actions mechanisms for bonus rewards #1402

Conversation

ipapandinas commented Jan 3, 2025 • edited Loading

Bonus Reward Eligibility

Move action

Stake Movement Enhancements

Migration

ipapandinas commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

Dinonard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipapandinas commented Jan 3, 2025 •

edited

Loading