feat(dAppStaking): Move actions for bonus rewards #1418

Dinonard · 2025-02-02T09:51:24Z

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.

The approach aims to first adapt existing pallet to easily support move functionality.
Pallet has a lot of tests covering stake & unstake functioality, and in case MAX_BONUS_MOVES = 1, every legacy test must work same as before.

Changes

logic from stake, unstake and unstake_from_unregistered is moved to separate functions, with minimal modifications
it's important for unstake to return more comprehensive information of what was unstaked
types are modified to return necessary information for the higher level functionality
test files are adapted to ensure that high level (extrinsic) tests are passing
move extrinsic, using inner_unstake and inner_stake functions (or unstake_from_unregistered) as the building blocks

Bonus Reward Eligibility

The previously used loyal_staker flag is replaced with a bonus_status u8 type.
The number of allowed move actions is defined by a config parameter (MaxBonusSafeMovesPerPeriod); bonus is forfeited if move actions exceed this value.

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 a 'stake'. 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

This PR introduces the update_bonus extrinsic, which updates loyal_staker flags from the previous 0 move actions limit to the new value set by MaxBonusSafeMovesPerPeriod. The change will take effect in a future runtime upgrade when the config parameters are updated.

Check list

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

Dinonard · 2025-02-02T09:51:56Z

@ipapandinas please check the concept.

Dinonard

Great work 👏
The types tests look like it was hellish to update 🙈

pallets/dapp-staking/src/benchmarking/mod.rs

pallets/dapp-staking/src/lib.rs

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

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

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

pallets/dapp-staking/src/types.rs

pallets/dapp-staking/src/lib.rs

… bonus_status

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

Dinonard

Haven't checked testing_utils in detail.

pallets/dapp-staking/src/lib.rs

pallets/dapp-staking/src/types.rs

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

Dinonard

Few additional comments, hope to discuss it more since I might not be aware of some issues.

pallets/dapp-staking/src/types.rs

pallets/dapp-staking/src/lib.rs

…EraInfo

ermalkaleci

LGTM

Dinonard

@ipapandinas checked the delta since last review - LGTM!
(cannot approve since I made the PR 🙈 )

ermalkaleci · 2025-02-18T08:39:35Z

pallets/dapp-staking/src/lib.rs

+                // Enable maintenance mode on the first run
+                // Likely to be already set in the runtime upgrade migration
+                ActiveProtocolState::<T>::mutate(|state| {
+                    state.maintenance = true;


does this mean anyone can put protocol in maintenance mode?

True, in a sense yes, anyone can trigger do_update, this is not the best. Maintenance mode is supposed to be entered with the on_runtime_upgrade here, but during chopstick testing it was not the case, so that's why I've put it also here too. I can remove it.

I haven't noticed this before tbh - but does this mean that even after the migration finishes, this code can still be entered, and maintenance mode re-enabled?

Btw, it's fine if anyone can trigger do_update, as long as it's protected properly.

weight is limited

once migration is finished, call has no effect

You can test migration with chopsticks as well. Don't override wasm but instead do system::set_code and then build a block to trigger runtime upgrade then build enough blocks to run all the migrations

I was testing it by overriding wasm, now I've followed your advice by manually building blocks, much better, thanks!

Dinonard · 2025-02-18T09:14:13Z

pallets/dapp-staking/src/lib.rs

+    /// Temporary cursor to persist latest BonusStatus item updated.
+    /// TODO: remove it once all BonusStatus are updated and this storage value is cleanup.
+    #[pallet::storage]
+    pub type ActiveBonusUpdateCursor<T: Config> =
+        StorageValue<_, (T::AccountId, T::SmartContract), OptionQuery>;


With respect to the comment about enabling maintenance mode in the migration - keep it only in the OnRuntimeUpgradeHook.

As for this particular storage, I can suggest two approaches:

OnRuntimeUpgrade sets this to Some, and None means it's finished. The migration extrinsic should immediately fail if this is None

A more clear approach would be to replace this value with:

enum MigrationState { NotStarted, InProgress(T::AccountId, T::SmartContract), Finished, }

failing the migration extrinsic if state is set to Finished

what if we set this cursor and enable maintenance on_runtime_upgrade? It will require to trigger updates immediately after migration though

My comments assume this is all setup in the on_runtime_upgrade.

In previous review I completely missed that it's not, my bad. I assumed it was 🤦.

I've used a proper BonusUpdateState in this commit 03df158

It should solved the issue

What about V8ToV9?
Shouldn't it be added to parachain runtimes?

I was planing to do it in the release prep PR, but it makes more sense to do it here. It's done now ✅

Dinonard

LGTM

pallets/dapp-staking/src/lib.rs

github-actions · 2025-02-18T14:50:48Z

Package	Line Rate	Branch Rate	Health
pallets/collective-proxy/src	94%	0%	✔
pallets/price-aggregator/src	76%	0%	➖
pallets/static-price-provider/src	91%	0%	✔
primitives/src/xcm	62%	0%	➖
pallets/vesting-mbm/src	87%	0%	✔
precompiles/xcm/src	69%	0%	➖
pallets/dapp-staking/src/benchmarking	95%	0%	✔
pallets/astar-xcm-benchmarks/src/generic	100%	0%	✔
chain-extensions/types/assets/src	0%	0%	❌
pallets/dapp-staking/src/test	0%	0%	❌
pallets/astar-xcm-benchmarks/src/fungible	100%	0%	✔
precompiles/dapp-staking/src	89%	0%	✔
chain-extensions/unified-accounts/src	0%	0%	❌
chain-extensions/pallet-assets/src	54%	0%	➖
pallets/dynamic-evm-base-fee/src	82%	0%	✔
pallets/unified-accounts/src	81%	0%	✔
pallets/dapp-staking/src	78%	0%	➖
precompiles/sr25519/src	56%	0%	➖
precompiles/substrate-ecdsa/src	67%	0%	➖
precompiles/dapp-staking/src/test	0%	0%	❌
pallets/inflation/src	90%	0%	✔
chain-extensions/types/unified-accounts/src	0%	0%	❌
primitives/src	54%	0%	➖
pallets/dapp-staking/rpc/runtime-api/src	0%	0%	❌
pallets/ethereum-checked/src	76%	0%	➖
precompiles/dispatch-lockdrop/src	83%	0%	✔
precompiles/assets-erc20/src	78%	0%	➖
pallets/xc-asset-config/src	48%	0%	❌
pallets/astar-xcm-benchmarks/src	86%	0%	✔
pallets/collator-selection/src	87%	0%	✔
precompiles/unified-accounts/src	100%	0%	✔
Summary	75% (3691 / 4894)	0% (0 / 0)	➖

Minimum allowed line rate is 50%

Concept adaptation for move functionality

6050826

Dinonard and others added 4 commits February 4, 2025 10:27

Comments

53c4632

BonusStatus type, config param added & tests adjusted

9af8d29

move extrinsic

5cd733d

additional tests

2e4a849

Dinonard added shiden related to shiden runtime astar Related to Astar shibuya related to shibuya runtime This PR/Issue is related to the topic “runtime”. labels Feb 6, 2025

Dinonard commented Feb 6, 2025

View reviewed changes

ipapandinas reviewed Feb 6, 2025

View reviewed changes

pallets/dapp-staking/src/lib.rs Outdated Show resolved Hide resolved

ipapandinas added 4 commits February 7, 2025 16:56

dynamic MAX_BONUS_SAFE_MOVES config param

09f1002

test utils: assert_move_stake and composable helpers

0e61acc

types: extra compare_stake_amounts method and StakeAmount stake takes…

1c45bf8

… bonus_status

some move tests

437d9a7

ipapandinas reviewed Feb 7, 2025

View reviewed changes

pallets/dapp-staking/src/test/testing_utils.rs Outdated Show resolved Hide resolved

Dinonard commented Feb 7, 2025

View reviewed changes

pallets/dapp-staking/src/lib.rs Outdated Show resolved Hide resolved

pallets/dapp-staking/src/types.rs Outdated Show resolved Hide resolved

pallets/dapp-staking/src/types.rs Outdated Show resolved Hide resolved

pallets/dapp-staking/src/test/testing_utils.rs Outdated Show resolved Hide resolved

benchmarks for both unstake scenarios

aaf2900

Dinonard commented Feb 9, 2025

View reviewed changes

pallets/dapp-staking/src/types.rs Outdated Show resolved Hide resolved

pallets/dapp-staking/src/lib.rs Outdated Show resolved Hide resolved

pallets/dapp-staking/src/lib.rs Outdated Show resolved Hide resolved

ipapandinas added 10 commits February 10, 2025 13:10

fix: current_era usage

c362eb6

fix: unstake from future stake does not chip current_stake_amount in …

5cd0b23

…EraInfo

fix: avoid copying in subtracted_stake_amount

c0fe6cd

fix: convert bonus stake into regular stake for just forfeited bonus

1c7030a

task: renaming and cleanups

4bf6ec9

fix: move from unregistered

9ded185

extra test

2ca99a7

task: on runtime upgrade bonus update migration

e108c54

fix: tmp weights for compilation success

78ae139

README updated

8f58c3f

ermalkaleci previously approved these changes Feb 14, 2025

View reviewed changes

ipapandinas added 4 commits February 17, 2025 12:59

review comments & renames

7897bfd

feat: merge existing bonus statuses

69aeb79

fix: previous_staked is only retained for previous eras

499b5f4

fix benchmark compilation

dfb0a75

ipapandinas dismissed ermalkaleci’s stale review via dfb0a75 February 17, 2025 12:45

ipapandinas added 3 commits February 17, 2025 19:07

migration fully tested

7df699a

fix: rework unstake_amount

ddeab1a

fix: remove "assert_eq!" from runtime

c1fc9bb

ipapandinas mentioned this pull request Feb 18, 2025

dAppStaking - moveStake extrinsic & loyalStaker flag migrated to bonusStatus AstarNetwork/astar-apps#1444

Open

Dinonard commented Feb 18, 2025

View reviewed changes

ipapandinas previously approved these changes Feb 18, 2025

View reviewed changes

ermalkaleci reviewed Feb 18, 2025

View reviewed changes

Dinonard commented Feb 18, 2025

View reviewed changes

fix: add BonusUpdateState for migration

03df158

ipapandinas dismissed their stale review via 03df158 February 18, 2025 12:06

add V8ToV9 migration to parachain runtimes

f371a77

Dinonard commented Feb 18, 2025

View reviewed changes

ipapandinas previously approved these changes Feb 18, 2025

View reviewed changes

ipapandinas requested a review from ermalkaleci February 18, 2025 14:15

ermalkaleci reviewed Feb 18, 2025

View reviewed changes

pallets/dapp-staking/src/lib.rs Outdated Show resolved Hide resolved

ermalkaleci reviewed Feb 18, 2025

View reviewed changes

pallets/dapp-staking/src/lib.rs Outdated Show resolved Hide resolved

minor cleanups

662bc6a

ipapandinas dismissed their stale review via 662bc6a February 18, 2025 14:26

ermalkaleci approved these changes Feb 18, 2025

View reviewed changes

ipapandinas approved these changes Feb 18, 2025

View reviewed changes

ipapandinas merged commit ff28544 into master Feb 18, 2025
8 checks passed

ipapandinas deleted the feat/ds-move-concept branch February 18, 2025 15:38

bobo-k2 mentioned this pull request Feb 24, 2025

Move stake implementtion AstarNetwork/astar-apps#1446

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dAppStaking): Move actions for bonus rewards #1418

feat(dAppStaking): Move actions for bonus rewards #1418

Dinonard commented Feb 2, 2025 •

edited by ipapandinas

Loading

Dinonard commented Feb 2, 2025

Dinonard left a comment

Dinonard left a comment

Dinonard left a comment

ermalkaleci left a comment

Dinonard left a comment

ermalkaleci Feb 18, 2025

ipapandinas Feb 18, 2025

Dinonard Feb 18, 2025

Dinonard Feb 18, 2025

ermalkaleci Feb 18, 2025

ipapandinas Feb 18, 2025

Dinonard Feb 18, 2025

ermalkaleci Feb 18, 2025

Dinonard Feb 18, 2025

ipapandinas Feb 18, 2025

Dinonard Feb 18, 2025

ipapandinas Feb 18, 2025

Dinonard left a comment

github-actions bot commented Feb 18, 2025

feat(dAppStaking): Move actions for bonus rewards #1418

feat(dAppStaking): Move actions for bonus rewards #1418

Conversation

Dinonard commented Feb 2, 2025 • edited by ipapandinas Loading

Changes

Bonus Reward Eligibility

Move action

Stake Movement Enhancements

Migration

Dinonard commented Feb 2, 2025

Dinonard left a comment

Choose a reason for hiding this comment

Dinonard left a comment

Choose a reason for hiding this comment

Dinonard left a comment

Choose a reason for hiding this comment

ermalkaleci left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dinonard left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 18, 2025

Dinonard commented Feb 2, 2025 •

edited by ipapandinas

Loading