Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(dapp-staking): Rework bonus rewards mechanism (#1379) #1385

Conversation

sylvaincormier
Copy link

Context

This pull request addresses issue #1379, reworking the dAppStaking bonus rewards mechanism to improve user flexibility and align with community feedback.

Features Implemented

  • Introduced a move extrinsic for reallocating stake while preserving bonuses.
  • Configurable limits for bonus moves (MaxBonusMovesPerPeriod, TempBonusMovesForOngoingPeriod).
  • Updated SingularStakingInfo to include BonusStatus:
    • SafeMovesRemaining(u8) tracks remaining moves.
    • BonusForfeited indicates forfeited bonuses.
  • Logic to handle stake movement, bonus preservation, and forfeiture.
  • Comprehensive unit tests to validate the feature.

Notes

  • Utilized existing unstake and stake logic for minimal code duplication.
  • Added configurations to support dynamic thresholds and bonus tracking.

Tests

All related tests pass:

  • Stake reallocation scenarios.
  • Bonus preservation and forfeiture checks.
  • Move counter logic across periods and subperiods.

References

  • Issue #1379
  • Original nomination_transfer logic for reference.

@sylvaincormier
Copy link
Author

sylvaincormier commented Nov 21, 2024

Hi, I tried to make this pull request more complete than the last one but it is missing documentation in case this needs to be reworked.

Still working on the benchmarking.
Thank you for your comments.

Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you for the effort, but you should have checked if issue is available before starting to work on it.

I did a quick glance, and there are lots of either minor issues or breaking bugs. E.g. the way decoding was done before and now would completely break dApp staking without proper migration.

We don't have capacity to support comments/improvements now, thank you for understanding.

@sylvaincormier
Copy link
Author

Thank you for reviewing. I realize I should have asked first and completed all aspects, including benchmarking and proper storage migration, before submitting the PR. Given this issue is part of the Kudos Carnival initiative, I'd appreciate completing these remaining components properly if you're willing to provide guidance or not. I'm committed to delivering a comprehensive implementation.
Is this still possible?

@sylvaincormier sylvaincormier marked this pull request as draft November 23, 2024 15:59
@sylvaincormier sylvaincormier force-pushed the dAppStaking-Bonus-rewards-mechanism-rework-#1379 branch 2 times, most recently from fad04c4 to 0e070a7 Compare November 24, 2024 11:37
- Introduced move extrinsic for stake reallocation
- Implemented bonus status tracking with SafeMovesRemaining
- Configured logic for max bonus moves and forfeiture
- Added unit tests to validate bonus rewards functionality
- Added benchmarking and V8 to V9 migration logic
- Added comprehensive documentation for stake movement
- Updated bonus reward documentation with move conditions

Part of dApp Staking bonus rewards mechanism rework AstarNetwork#1379
@sylvaincormier sylvaincormier force-pushed the dAppStaking-Bonus-rewards-mechanism-rework-#1379 branch from 22c081b to c3d5e0d Compare November 24, 2024 11:47
@sylvaincormier
Copy link
Author

I have completed a full implementation of the bonus rewards mechanism rework, addressing all major components:

Core Implementation:

  • Added move extrinsic for stake reallocation between contracts
  • Implemented BonusStatus tracking with SafeMovesRemaining
  • Added configurable MaxBonusMovesPerPeriod
  • Implemented proper bonus forfeiture logic

Technical Requirements:

  • Added V8 to V9 storage migration for the new bonus status encoding
  • Completed benchmarking for the move extrinsic
  • Added comprehensive test coverage including:
    • Stake movement scenarios
    • Bonus preservation/forfeiture
    • Migration tests
    • Period boundary handling

Documentation:

  • Added "Moving Stake Between Contracts" section to README
  • Updated bonus rewards documentation with move conditions
  • Documented all new functionality and constraints

All tests are passing and the code is up to date with master. Ready for review.

@sylvaincormier sylvaincormier marked this pull request as ready for review November 24, 2024 11:52
@Dinonard
Copy link
Member

Hey @sylvaincormier, sorry but it's not possible at the moment.

Out of interest, I just took a glance at the migration you implemented - it would brick the chain because of extensive resource usage.

@Dinonard Dinonard closed this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants