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

Implements partial inflation minting to treasury #1483

Closed
wants to merge 16 commits into from

Conversation

gpestana
Copy link
Contributor

@gpestana gpestana commented Sep 11, 2023

This PR implements an optional minting of the inflation directly into a pot.

The staking pallet now has a percent storage item, InflationLevyFraction, which defines the percentage of the era inflation that should be minted directly into an account (the treasury's account). The remaining of the fraction is the validators rewards for that era. The InflationLevyFraction can be set via extrinsic by the staking admin origin through the set_treasury_fraction callable. The InflationLevyFraction default is 0.

Treasury Get<AccountId> implementation

This PR also implements the Get<AccountId> in the treasury pallet which exposes an account id derived at the origin. This helps the staking pallet to request the account ID of the treasury pallet, instead of deriving it locally based on its pallet ID.

Closes #403

@gpestana gpestana requested a review from Ank4n September 11, 2023 01:13
@gpestana gpestana self-assigned this Sep 11, 2023
@gpestana gpestana added I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. T5-host_functions This PR/Issue is related to host functions. T8-polkadot This PR/Issue is related to/affects the Polkadot network. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Sep 11, 2023
@gpestana gpestana removed the T5-host_functions This PR/Issue is related to host functions. label Sep 11, 2023
@gpestana gpestana requested review from a team September 11, 2023 09:47
@gpestana gpestana requested a review from a team September 11, 2023 10:24
@gpestana
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_staking

@gpestana
Copy link
Contributor Author

bot help

…=dev --target_dir=substrate --pallet=pallet_staking
@gpestana gpestana removed the request for review from jakoblell September 11, 2023 14:34
@gpestana
Copy link
Contributor Author

bot bench polkadot-pallet --runtime polkadot

@gpestana
Copy link
Contributor Author

bot bench polkadot-pallet --pallet=pallet_staking --runtime polkadot

@gpestana
Copy link
Contributor Author

bot clean

substrate/frame/staking/src/pallet/mod.rs Outdated Show resolved Hide resolved
substrate/frame/staking/src/pallet/mod.rs Outdated Show resolved Hide resolved
substrate/frame/staking/src/pallet/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Somewhat skeptical of this implementation being in the right path.

Staking already have a way to communicate "what should I do with the rest of my rewards?", which is RewardRemainder.

It seems a bit sloppy to add another similar mechanism that says "before I distribute my rewards, what should I do with it?".

I kinda wish there was a way to reuse RewardRemainder to bundle it into a single new hook that expresses what should be done with rewards.

Lastly, you are making a few important assumptions:

  1. That a treasury pallet exists.
  2. How this treasury pallet's pallet id should be converted into an account id.

Ideally, all of this should be more abstract.

@gpestana
Copy link
Contributor Author

gpestana commented Sep 13, 2023

Staking already have a way to communicate "what should I do with the rest of my rewards?", which is RewardRemainder.

My idea is that the treasury payment and validator rewards are both first citizen reward "receivers", thus there could be still a remainder. I will look into this direction again though.

Ideally, all of this should be more abstract.

Good point, I think that there are a few things that could be changed: 1) renaming "TreasuryPot" to something else, 2) have a way to request the account ID of a type that implements a new Pot trait that exposes a way to get an account_id. This last point is already implemented in the last commit.

Using the trait Pot also helps making the implementation safer, since it offloads the burden of deriving the account ID to the origin so that we don't need to keep the account ID derivation in 2 different places, which may be error-prone in case of updates.

EDIT: instead of creating a new Pot trait, the PR is using the Get<AccountId> instead, which is implemented by the treasury pallet.

@gpestana
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Sep 14, 2023

@gpestana https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3697257 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 3-a5d29906-5be9-4931-8af1-42a711901856 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 14, 2023

@gpestana Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3697257 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3697257/artifacts/download.

@gpestana gpestana requested a review from kianenigma September 14, 2023 14:05
@gpestana
Copy link
Contributor Author

gpestana commented Sep 18, 2023

Staking already have a way to communicate "what should I do with the rest of my rewards?", which is RewardRemainder.

My idea is that the treasury payment and validator rewards are both first citizen reward "receivers", thus there could be still a remainder. I will look into this direction again though.

Ideally, all of this should be more abstract.

Good point, I think that there are a few things that could be changed: 1) renaming "TreasuryPot" to something else, 2) have a way to request the account ID of a type that implements a new Pot trait that exposes a way to get an account_id. This last point is already implemented in the last commit.

Using the trait Pot also helps making the implementation safer, since it offloads the burden of deriving the account ID to the origin so that we don't need to keep the account ID derivation in 2 different places, which may be error-prone in case of updates.

EDIT: instead of creating a new Pot trait, the PR is using the Get<AccountId> instead, which is implemented by the treasury pallet.

@gpestana gpestana closed this Sep 18, 2023
@gpestana gpestana reopened this Sep 18, 2023
@gpestana
Copy link
Contributor Author

@kianenigma to touch base on your comment to abstract this further and use the RewardRemainder as a way to pay the treasury fraction:

My current perspective is that the RewardRemainder should be the remainder of the total payout (validator rewards and treasury %), based on the yearly inflation and the total payouts.

Following that, I think that having an explicit InflationLevyFraction and InflationLevyDestination = Get<AccountId>, and explicitly use those types to calculate and mint directly into the treasury is a reasonable way to implement this. wdyt?

Otoh, we could refactor the way we handle the reward remainder and RewardRemainder to both handle the inflation remainder and the treasury payment, but I think that's not conceptually the best way (i.e. the treasury payout is not a "remainder").

@gpestana
Copy link
Contributor Author

Being replaced by #1660

@gpestana gpestana closed this Sep 21, 2023
github-merge-queue bot pushed a commit that referenced this pull request Feb 15, 2024
This PR implements an (optional) cap of the era inflation that is
allocated to staking rewards. The remaining is minted directly into the
[`RewardRemainder`](https://github.com/paritytech/polkadot-sdk/blob/fb0fd3e62445eb2dee2b2456a0c8574d1ecdcc73/substrate/frame/staking/src/pallet/mod.rs#L160)
account, which is the treasury pot account in Polkadot and Kusama.

The staking pallet now has a percent storage item, `MaxStakersRewards`,
which defines the max percentage of the era inflation that should be
allocated to staking rewards. The remaining era inflation (i.e.
`remaining = max_era_payout - staking_payout.min(staking_payout *
MaxStakersRewards))` is minted directly into the treasury.

The `MaxStakersRewards` can be set by a privileged origin through the
`set_staking_configs` extrinsic.

**To finish**
- [x] run benchmarks for westend-runtime

Replaces #1483
Closes #403

---------

Co-authored-by: command-bot <>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…#1660)

This PR implements an (optional) cap of the era inflation that is
allocated to staking rewards. The remaining is minted directly into the
[`RewardRemainder`](https://github.com/paritytech/polkadot-sdk/blob/d349b45d086199af7ad0195534e2b913ba9b6827/substrate/frame/staking/src/pallet/mod.rs#L160)
account, which is the treasury pot account in Polkadot and Kusama.

The staking pallet now has a percent storage item, `MaxStakersRewards`,
which defines the max percentage of the era inflation that should be
allocated to staking rewards. The remaining era inflation (i.e.
`remaining = max_era_payout - staking_payout.min(staking_payout *
MaxStakersRewards))` is minted directly into the treasury.

The `MaxStakersRewards` can be set by a privileged origin through the
`set_staking_configs` extrinsic.

**To finish**
- [x] run benchmarks for westend-runtime

Replaces paritytech#1483
Closes paritytech#403

---------

Co-authored-by: command-bot <>
bkchr pushed a commit that referenced this pull request Apr 10, 2024
Unify the operating mode for bridge pallets

- define the OperationMode trait and BasicOperatingMode enum
- use the OperationMode trait in all the bridge pallets
- use BasicOperatingMode instead of IsHalted for the Grandpa pallet
- use BasicOperatingMode as part of MessagesOperatingMode

Signed-off-by: Serban Iorga <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[NPoS] Mint fixed portion of inflation directly into the treasury
3 participants