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

pallet_staking: Introduce Additional Account for Inflation Distribution Alongside PBR #2976

Merged
merged 45 commits into from
Oct 7, 2024

Conversation

TarekkMA
Copy link
Contributor

@TarekkMA TarekkMA commented Sep 25, 2024

New version of #2958, with a simpler design and no storage maps being used

Checklist

  • Write tests for the new changes
  • Fix/Add all required rustdocs
  • Write a migration
  • Discuss the possibility of moving to pallet_parameters
    • This would require more refactoring for tests and changing code, so I opted to have this in another PR.
  • Write benchmark for the new ext

What does it do?

  • Add a new account (Treasury) to have inflation be distributed to along side PBR Parachain Bond Reserve account.
  • I kept the old extrinsics of changing PBR account/percentage as is for backward compatibility.

What important points reviewers should know?

  • I've wrapped the static array with a struct since it seems like an array on it's own would not work correctly with ts typegen.

Is there something left for follow-up PRs?

  • If we will pursuit using pallet parameters with this change, we will need another PR

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?

@TarekkMA TarekkMA added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited breaking Needs to be mentioned in breaking changes labels Sep 25, 2024
Copy link
Contributor

github-actions bot commented Sep 25, 2024

WASM runtime size check:

Compared to target branch

Moonbase runtime: 2288 KB (no changes) ✅

Moonbeam runtime: 2240 KB (no changes) ✅

Moonriver runtime: 2236 KB (no changes) ✅

Compared to latest release (runtime-3200)

Moonbase runtime: 2288 KB (+328 KB compared to latest release) ⚠️

Moonbeam runtime: 2240 KB (+316 KB compared to latest release) ⚠️

Moonriver runtime: 2236 KB (+312 KB compared to latest release) ⚠️

Copy link
Contributor

github-actions bot commented Sep 25, 2024

Coverage Report

@@                        Coverage Diff                         @@
##           master   tarekkma/staking-add-address-2      +/-   ##
==================================================================
- Coverage   79.18%                           79.12%   -0.06%     
+ Files         299                              300       +1     
+ Lines       87673                            87957     +284     
==================================================================
+ Hits        69419                            69594     +175     
+ Misses      18254                            18363     +109     
Files Changed Coverage
/pallets/parachain-staking/src/lib.rs 91.79% (+0.05%) 🔼
/pallets/parachain-staking/src/mock.rs 97.70% (+0.07%) 🔼
/pallets/parachain-staking/src/tests.rs 90.85% (-0.51%) 🔽
/pallets/parachain-staking/src/types.rs 78.05% (-0.27%) 🔽
/pallets/parachain-staking/src/weights.rs 38.35% (-0.49%) 🔽
/runtime/common/src/migrations.rs 72.09% (-0.24%) 🔽
/runtime/common/src/weights/pallet_parachain_staking.rs 19.59% (-0.27%) 🔽

Coverage generated Mon Oct 7 08:34:30 UTC 2024

@TarekkMA TarekkMA marked this pull request as draft September 27, 2024 12:51
@noandrea noandrea added the A8-mergeoncegreen Pull request is reviewed well. label Sep 30, 2024
@TarekkMA TarekkMA marked this pull request as ready for review October 2, 2024 13:34
librelois
librelois previously approved these changes Oct 4, 2024
Copy link
Collaborator

@librelois librelois left a comment

Choose a reason for hiding this comment

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

Some minor comments, but overall it's good 👍

pLabarta
pLabarta previously approved these changes Oct 4, 2024
@TarekkMA TarekkMA dismissed stale reviews from pLabarta and librelois via 3f5ccc6 October 7, 2024 07:58
@pLabarta pLabarta merged commit c779eb3 into master Oct 7, 2024
39 checks passed
@pLabarta pLabarta deleted the tarekkma/staking-add-address-2 branch October 7, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-mergeoncegreen Pull request is reviewed well. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants