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

fix: auto-compound fail when free balance is too low #2853

Merged
merged 12 commits into from
Jul 4, 2024

Conversation

librelois
Copy link
Collaborator

@librelois librelois commented Jun 27, 2024

What does it do?

This PR fix a bug that impact the auto-compound of staking rewards and add a dev test to reproduce the bug.

The bug is that the auto-compound logic check if stakable amount is high enough, but this check doesn't take into account the reserve, so in some corner cases the auto-compound fail silently, then the user receive less staking rewards than expected.

Old buggy logic: stakable amount is free- frozen
New logic (fix): stakable amount is free + reserve - frozen

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@librelois librelois added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes labels Jun 27, 2024
Copy link
Contributor

github-actions bot commented Jun 27, 2024

Coverage Report

@@                     Coverage Diff                     @@
##           master   elois-fix-auto-compound      +/-   ##
===========================================================
+ Coverage   81.30%                    81.33%   +0.03%     
  Files         301                       301              
- Lines       85701                     85669      -32     
===========================================================
- Hits        69675                     69672       -3     
- Misses      16026                     15997      -29     
Files Changed Coverage
/client/rpc/manual-xcm/src/lib.rs 88.00% (+25.33%) 🔼
/client/rpc/txpool/src/lib.rs 65.38% (+8.65%) 🔼
/client/rpc-core/txpool/src/types/content.rs 70.37% (+1.85%) 🔼
/pallets/parachain-staking/src/lib.rs 91.74% (+0.01%) 🔼
/pallets/parachain-staking/src/mock.rs 97.63% (-0.02%) 🔽
/pallets/parachain-staking/src/tests.rs 91.36% (-0.03%) 🔽

Coverage generated Thu Jul 4 10:57:53 UTC 2024

@librelois librelois marked this pull request as ready for review July 2, 2024 09:51
@librelois librelois requested review from a team as code owners July 2, 2024 09:51
@librelois librelois requested a review from RomarQ July 3, 2024 12:47
RomarQ
RomarQ previously approved these changes Jul 3, 2024
…compound7.ts

Co-authored-by: Agustín Rodriguez <agus@moonsonglabs.com>
@librelois librelois dismissed stale reviews from Agusrodri and RomarQ via be03273 July 4, 2024 10:14
…compound7.ts

Co-authored-by: Agustín Rodriguez <agus@moonsonglabs.com>
@librelois librelois merged commit 47ba799 into master Jul 4, 2024
27 checks passed
@librelois librelois deleted the elois-fix-auto-compound branch July 4, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants