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.

Sign up for GitHub

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

Jump to bottom

feat!: refactor x/mint module #321

Merged
merged 23 commits into from
Mar 19, 2024
Merged

feat!: refactor x/mint module #321

merged 23 commits into from
Mar 19, 2024

Conversation

mtsitrin
Copy link
Collaborator

@mtsitrin mtsitrin commented Mar 14, 2024 •
edited by omritoptix
Loading

mint module now:

  • mint tokens on mint_epoch by calculating current_inflation of the total_supply
  • change inflation on inflation_epoch by inflation_rate_change towards the target_inflation_rate

PRD:
https://www.notion.so/dymension/RollApp-Mint-4e0e66a9c2f142ceb3dbfc188830da0d?pvs=4

Missing:

@mtsitrin mtsitrin marked this pull request as ready for review March 14, 2024 13:08
@mtsitrin mtsitrin requested a review from a team as a code owner March 14, 2024 13:08
@mtsitrin mtsitrin linked an issue Mar 14, 2024 that may be closed by this pull request
RDK mint module min inflation rate adjustments #305
Closed
@omritoptix omritoptix requested a review from srene March 15, 2024 12:26
srene
srene reviewed Mar 17, 2024
View reviewed changes
x/epochs/types/genesis.go
func DefaultGenesis() *GenesisState {
epochs := []EpochInfo{
{
Identifier: WEEK_EPOCH,
Identifier: HOUR_EPOCH,
Copy link
Contributor

Choose a reason for hiding this comment

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

why do not support minute ephocs, as stated in the PRD?

x/mint/keeper/hooks.go Outdated
if epochNumber < params.MintingRewardsDistributionStartEpoch {
// Update inflation
if epochIdentifier == params.InflationEpochIdentifier {
newInfaltion, err := k.HandleInflationChange(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

x/mint/keeper/mint.go Outdated
return mintedCoins, nil
}

func (k Keeper) CalcMintedCoins(ctx sdk.Context, totalAmt sdk.Int) sdk.Dec {
Copy link
Contributor

Choose a reason for hiding this comment

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

i guess is better to change the use of sdk.int for cosmossdk.io/math, since its deprecated

x/mint/keeper/inflation.go
sdk "github.com/cosmos/cosmos-sdk/types"
)

func (k Keeper) HandleInflationChange(ctx sdk.Context) (inflationRate sdk.Dec, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

From the function, i understand we support increasing inflation functions as well.
I thought, from the PRD, inflation was always a decreasing function, but just asking to be sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it depends on the target inflation, relatives to current inflation

x/mint/keeper/mint.go Outdated
func (k Keeper) CalcMintedCoins(ctx sdk.Context, totalAmt sdk.Int) sdk.Dec {
params := k.GetParams(ctx)
minter := k.GetMinter(ctx)
mintAmount := minter.CurrentInflationRate.MulInt(totalAmt).QuoInt(sdk.NewInt(params.MintEpochSpreadFactor))
Copy link
Contributor

Choose a reason for hiding this comment

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

i understand that MintEpochSpreadFactor param, is the number of minting epochs in a year that is used to calculate the minted tokens for each epoch.
If this is the case , i do not see why it is necessary to have a specific parameter for that, and why cannot be derived from MintEpochIdentifier to calculate how many minting epochs we have in a year. and in case we use it, i think MintEpochSpreadFactor needs to be validated to match the MintEpochIdentifier and probably a better name like MintingEpochsInAYear can be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll check if possible to get it from the epochKeeper
otherwise, we need some translation between mint_epoch - string to num of epochs in a year
anyway, it allow some generalized approach, in case inflation won't be calculated annually

@mtsitrin mtsitrin requested review from srene and zale144 March 17, 2024 19:38
srene
srene previously approved these changes Mar 18, 2024
View reviewed changes
@zale144 zale144 mentioned this pull request Mar 18, 2024
Merged
12 tasks
zale144
zale144 previously approved these changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@zale144 zale144 left a comment

Choose a reason for hiding this comment

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

lgtm

@omritoptix omritoptix mentioned this pull request Mar 18, 2024
Merged
12 tasks
@mtsitrin mtsitrin requested review from srene and zale144 March 18, 2024 16:26
@omritoptix omritoptix merged commit 0a9c24b into main Mar 19, 2024
2 checks passed
@omritoptix omritoptix deleted the mtsitrin/305-rdk-mint-module-min-inflation-rate-adjustments branch March 19, 2024 08:38
hoangdv2429 pushed a commit to decentrio/dymension-rdk that referenced this pull request Mar 19, 2024
@mtsitrin @zale144
@omritoptix @hoangdv2429
feat!: refactor x/mint module (dymensionxyz#321)
1e0e46f 
Co-authored-by: zale144 <[email protected]>
Co-authored-by: Omri <[email protected]>
hoangdv2429 pushed a commit to decentrio/dymension-rdk that referenced this pull request Mar 19, 2024
@mtsitrin @zale144
@omritoptix @hoangdv2429
feat!: refactor x/mint module (dymensionxyz#321)
d2df573 
Co-authored-by: zale144 <[email protected]>
Co-authored-by: Omri <[email protected]>
@mtsitrin mtsitrin restored the mtsitrin/305-rdk-mint-module-min-inflation-rate-adjustments branch April 8, 2024 07:18
@mtsitrin mtsitrin deleted the mtsitrin/305-rdk-mint-module-min-inflation-rate-adjustments branch April 8, 2024 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omritoptix omritoptix omritoptix approved these changes

@shaolin-flow shaolin-flow Awaiting requested review from shaolin-flow

@srene srene Awaiting requested review from srene

@zale144 zale144 Awaiting requested review from zale144

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RDK mint module min inflation rate adjustments
4 participants
@mtsitrin @omritoptix @srene @zale144