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

Stake/unstake fix #1187

Merged
merged 30 commits into from
Mar 18, 2024
Merged

Stake/unstake fix #1187

merged 30 commits into from
Mar 18, 2024

Conversation

Dinonard
Copy link
Member

@Dinonard Dinonard commented Feb 29, 2024

Pull Request Summary

Fixes an issue where it's possible to end up with inconsistent staked amount
for the ongoing era for some smart contract.

TODO

  • issue reproduced via tests
  • (init) solution implemented & tested
  • improved types test
    • previous_staked tests
    • unstake return values test
    • unstake with different tuples test
  • change approach for unstake to return multiple tuples
  • remove redundant subperiod argument
  • implement & test migration logic

@Dinonard Dinonard self-assigned this Feb 29, 2024
@Dinonard Dinonard added the runtime This PR/Issue is related to the topic “runtime”. label Feb 29, 2024
@Dinonard Dinonard marked this pull request as ready for review February 29, 2024 13:55
@Dinonard Dinonard changed the title Fix/dapp staking unstake inconsistency Stake/unstake fix Feb 29, 2024
@Dinonard Dinonard marked this pull request as draft February 29, 2024 23:05
@Dinonard Dinonard marked this pull request as ready for review March 5, 2024 16:04
Copy link
Member

@ashutoshvarma ashutoshvarma left a comment

Choose a reason for hiding this comment

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

Looks good!
Thanks for those extensive comments in unstake method and sharing the summary.

I didn't check the benchmarks properly, will do in final review after migration tests & conflicts resolved

@Dinonard
Copy link
Member Author

@ashutoshvarma thanks!

I was postponing to push the conflict resolution not to mess up the review, but sure, here it goes 🙂

ashutoshvarma
ashutoshvarma previously approved these changes Mar 13, 2024
ashutoshvarma
ashutoshvarma previously approved these changes Mar 13, 2024
Copy link

Code Coverage

Package Line Rate Branch Rate Health
chain-extensions/xvm/src 0% 0%
pallets/block-rewards-hybrid/src 91% 0%
pallets/dapps-staking/src/pallet 86% 0%
precompiles/dapp-staking-v3/src 90% 0%
precompiles/dispatch-lockdrop/src 86% 0%
precompiles/xvm/src 74% 0%
chain-extensions/pallet-assets/src 56% 0%
precompiles/dapps-staking/src 93% 0%
primitives/src 29% 0%
precompiles/assets-erc20/src 81% 0%
primitives/src/migrations 0% 0%
chain-extensions/types/assets/src 0% 0%
pallets/dapp-staking-v3/src 91% 0%
pallets/unified-accounts/src 85% 0%
primitives/src/xcm 65% 0%
precompiles/unified-accounts/src 100% 0%
precompiles/sr25519/src 64% 0%
pallets/dapps-staking/src 89% 0%
pallets/dapp-staking-v3/src/benchmarking 99% 0%
pallets/astar-xcm-benchmarks/src 87% 0%
chain-extensions/types/unified-accounts/src 0% 0%
precompiles/substrate-ecdsa/src 74% 0%
precompiles/xcm/src 73% 0%
pallets/dapp-staking-migration/src 0% 0%
pallets/xc-asset-config/src 64% 0%
pallets/dapp-staking-v3/src/test 0% 0%
pallets/astar-xcm-benchmarks/src/fungible 100% 0%
chain-extensions/types/xvm/src 0% 0%
chain-extensions/unified-accounts/src 0% 0%
pallets/static-price-provider/src 58% 0%
pallets/ethereum-checked/src 79% 0%
pallets/xvm/src 51% 0%
pallets/astar-xcm-benchmarks/src/generic 100% 0%
pallets/dapp-staking-v3/rpc/runtime-api/src 0% 0%
pallets/dynamic-evm-base-fee/src 92% 0%
pallets/inflation/src 83% 0%
precompiles/dapp-staking-v3/src/test 0% 0%
pallets/collator-selection/src 91% 0%
Summary 76% (4305 / 5687) 0% (0 / 0)

Minimum allowed line rate is 50%

@Dinonard Dinonard merged commit b392564 into master Mar 18, 2024
8 checks passed
@Dinonard Dinonard deleted the fix/dapp-staking-unstake-inconsistency branch March 18, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants