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

dApp staking v3 - Cycle Alignment #1134

Merged
merged 18 commits into from
Jan 17, 2024
Merged

Conversation

Dinonard
Copy link
Member

@Dinonard Dinonard commented Jan 11, 2024

Pull Request Summary

Related to: #1127

Even though the implementation in this PR is labeled as Less Proper Solution, it proved to be more proper than extracting the logic to a new pallet.

Two main reasons being:

  • It's more logical for dApp staking to handle it's own maintenance mode than for it to be done via the new pallet. And if it is done via dAPp staking, this would need to be propagated to the new pallet, creating a two way communication channel between two pallets. It seems overly complex and unnatural for such a simple thing as setting maintenance mode. Additionally, it's better to keep this logic only within the dApp staking pallet itself (more secure, no one from the outside can edit it).
  • Handling eras, subperiods & periods is tightly tied with dApp staking. It seems most natural and correct to handle it within that pallet. Majority of the logic relies on the information about these values, and moving them out would mean they are fetched from the outside of the pallet, via some trait. It would create additionally connections & complexities, especially if we try to add more features in the future.

For that reason, the following architecture is used:

  • pallet_dapp_staking_v3 has a new Observers trait which it uses to notify observers about events of interest
  • pallet_inflation keeps track at which era should it trigger inflation recalculation, and relies on the notification from dApp staking pallet about era changes

Check list

  • re-run benchmarks
  • integration tests
  • force should also inform observers
  • runtime upgrade logic, to migrate between the old & the new values
  • try-runtime test
  • sort out code TODOs

@Dinonard Dinonard added the runtime This PR/Issue is related to the topic “runtime”. label Jan 12, 2024
@Dinonard Dinonard linked an issue Jan 12, 2024 that may be closed by this pull request
@Dinonard Dinonard self-assigned this Jan 12, 2024
Base automatically changed from feat/dsv3-retro-review to master January 15, 2024 10:40
@Dinonard
Copy link
Member Author

/bench shibuya-dev pallet_inflation,pallet_dapp_staking_v3

Copy link

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7527934157.
Please wait for a while.
Branch: feat/cycle-alignment-handler
SHA: cf8e665

Copy link

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

@Dinonard Dinonard marked this pull request as ready for review January 15, 2024 14:00
ashutoshvarma
ashutoshvarma previously approved these changes Jan 16, 2024
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!
(I didn't thoroughly go though inflation migration logic)

Copy link

Code Coverage

Package Line Rate Branch Rate Health
pallets/dapps-staking/src/pallet 85% 0%
pallets/astar-xcm-benchmarks/src/generic 0% 0%
pallets/collator-selection/src 69% 0%
pallets/block-rewards-hybrid/src 87% 0%
chain-extensions/pallet-assets/src 56% 0%
pallets/unified-accounts/src 84% 0%
precompiles/dapps-staking/src 94% 0%
precompiles/sr25519/src 64% 0%
precompiles/substrate-ecdsa/src 74% 0%
precompiles/xvm/src 74% 0%
primitives/src 58% 0%
pallets/ethereum-checked/src 48% 0%
chain-extensions/types/xvm/src 0% 0%
pallets/dapps-staking/src 81% 0%
pallets/static-price-provider/src 58% 0%
pallets/astar-xcm-benchmarks/src 0% 0%
pallets/xvm/src 40% 0%
pallets/dapp-staking-migration/src 39% 0%
pallets/dynamic-evm-base-fee/src 81% 0%
primitives/src/xcm 66% 0%
pallets/dapp-staking-v3/src/benchmarking 0% 0%
pallets/astar-xcm-benchmarks/src/fungible 0% 0%
chain-extensions/unified-accounts/src 0% 0%
chain-extensions/types/assets/src 0% 0%
pallets/dapp-staking-v3/rpc/runtime-api/src 0% 0%
precompiles/assets-erc20/src 81% 0%
precompiles/xcm/src 72% 0%
pallets/inflation/src 69% 0%
precompiles/unified-accounts/src 100% 0%
pallets/dapp-staking-v3/src/test 0% 0%
pallets/dapp-staking-v3/src 76% 0%
precompiles/dapp-staking-v3/src 90% 0%
precompiles/dapp-staking-v3/src/test 0% 0%
chain-extensions/types/unified-accounts/src 0% 0%
pallets/xc-asset-config/src 53% 0%
chain-extensions/xvm/src 0% 0%
Summary 70% (3259 / 4686) 0% (0 / 0)

Minimum allowed line rate is 50%

Copy link
Member

@PierreOssun PierreOssun left a comment

Choose a reason for hiding this comment

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

LGTM

@Dinonard Dinonard merged commit ada298f into master Jan 17, 2024
9 checks passed
@Dinonard Dinonard deleted the feat/cycle-alignment-handler branch January 17, 2024 10:15
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.

Cycle Alignment Between dApp Staking & Inflation
3 participants