-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat(x/mint)!: Replace InflationCalculationFn with MintFn + simple epoch minting #20363
Changes from 6 commits
603d9a7
ba0b474
cf5fd6b
63a3a89
4cd5f15
6734cab
0959509
dc3e06e
b063cbb
848cbf5
f2f8213
5ae7ce6
7f26121
5abb75f
593baab
61caa0b
969cb1d
faa40de
f89c958
183fb37
67aac7d
9696890
61dab4a
62c9b22
007d50e
aa7fccf
3d73011
18a741f
3b4264d
d80fe8d
ce185c2
ee8d6a6
a497b96
281557d
1c4e6c1
32fe4d9
dc14d2b
ace7bca
e5425a4
2c80fed
ec56d49
1c62f85
0931da6
c002f55
61250af
29970a6
043dc71
d9e30cb
712d5aa
a66e1e8
226091c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
package simapp | ||
|
||
import ( | ||
"context" | ||
|
||
"cosmossdk.io/core/appmodule" | ||
"cosmossdk.io/core/event" | ||
"cosmossdk.io/math" | ||
authtypes "cosmossdk.io/x/auth/types" | ||
bankkeeper "cosmossdk.io/x/bank/keeper" | ||
banktypes "cosmossdk.io/x/bank/types" | ||
minttypes "cosmossdk.io/x/mint/types" | ||
stakingtypes "cosmossdk.io/x/staking/types" | ||
"github.com/cosmos/cosmos-sdk/telemetry" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
) | ||
|
||
// ProvideMintFn returns the function used in x/mint's endblocker to mint new tokens. | ||
// Note that this function can not have the mint keeper as a parameter because it would create a cyclic dependency. | ||
func ProvideMintFn(bankKeeper bankkeeper.Keeper) minttypes.MintFn { | ||
return func(ctx context.Context, env appmodule.Environment, minter *minttypes.Minter) error { | ||
var stakingParams stakingtypes.QueryParamsResponse | ||
err := env.RouterService.QueryRouterService().InvokeTyped(ctx, &stakingtypes.QueryParamsRequest{}, &stakingParams) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
var bankSupply banktypes.QuerySupplyOfResponse | ||
err = env.RouterService.QueryRouterService().InvokeTyped(ctx, &banktypes.QuerySupplyOfRequest{Denom: stakingParams.Params.BondDenom}, &bankSupply) | ||
if err != nil { | ||
return err | ||
} | ||
stakingTokenSupply := bankSupply.Amount | ||
|
||
var mintParams minttypes.QueryParamsResponse | ||
err = env.RouterService.QueryRouterService().InvokeTyped(ctx, &minttypes.QueryParamsRequest{}, &mintParams) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
var stakingPool stakingtypes.QueryPoolResponse | ||
err = env.RouterService.QueryRouterService().InvokeTyped(ctx, &stakingtypes.QueryPoolRequest{}, &stakingPool) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// bondedRatio | ||
bondedRatio := math.LegacyNewDecFromInt(stakingPool.Pool.BondedTokens).QuoInt(stakingTokenSupply.Amount) | ||
minter.Inflation = minter.NextInflationRate(mintParams.Params, bondedRatio) | ||
minter.AnnualProvisions = minter.NextAnnualProvisions(mintParams.Params, stakingTokenSupply.Amount) | ||
// TODO: store minter afterwards | ||
|
||
mintedCoin := minter.BlockProvision(mintParams.Params) | ||
mintedCoins := sdk.NewCoins(mintedCoin) | ||
maxSupply := mintParams.Params.MaxSupply | ||
totalSupply := stakingTokenSupply.Amount | ||
|
||
// if maxSupply is not infinite, check against max_supply parameter | ||
if !maxSupply.IsZero() { | ||
if totalSupply.Add(mintedCoins.AmountOf(mintParams.Params.MintDenom)).GT(maxSupply) { | ||
// calculate the difference between maxSupply and totalSupply | ||
diff := maxSupply.Sub(totalSupply) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have not seen a check in the module that ensures maxSupply > totalSupply. Therefore this can be negative 🙈 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought that I had pushed that fix, found it yesterday while adding more tests 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at it again, no, I didn't add a fix. Should we return an error if totalSupply > maxSupply here? It should never be the case tho, as the minting only happens here and we don't mint over maxSupply |
||
// mint the difference | ||
diffCoin := sdk.NewCoin(mintParams.Params.MintDenom, diff) | ||
diffCoins := sdk.NewCoins(diffCoin) | ||
|
||
// mint coins | ||
if diffCoins.Empty() { | ||
// skip as no coins need to be minted | ||
return nil | ||
} | ||
|
||
if err := bankKeeper.MintCoins(ctx, minttypes.ModuleName, diffCoins); err != nil { | ||
return err | ||
} | ||
mintedCoins = diffCoins | ||
} | ||
} | ||
|
||
// mint coins if maxSupply is infinite or total staking supply is less than maxSupply | ||
if maxSupply.IsZero() || totalSupply.Add(mintedCoins.AmountOf(mintParams.Params.MintDenom)).LT(maxSupply) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when total + minted == max supply, then neither L67 or this condition fits and no coins would be minted although a sub amount would be possible. I think L67 should handle this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm simplifying it like this: if !maxSupply.IsZero() {
// supply is not infinite, check the amount to mint
remainingSupply := maxSupply.Sub(totalSupply)
if remainingSupply.LTE(math.ZeroInt()) {
// max supply reached, no new tokens will be minted
// also handles the case where totalSupply > maxSupply
return nil
}
// if the amount to mint is greater than the remaining supply, mint the remaining supply
if mintedCoin.Amount.GT(remainingSupply) {
mintedCoin.Amount = remainingSupply
}
}
if mintedCoin.Amount.IsZero() {
// skip as no coins need to be minted
return nil
}
mintedCoins := sdk.NewCoins(mintedCoin)
if err := bankKeeper.MintCoins(ctx, minttypes.ModuleName, mintedCoins); err != nil {
return err
} |
||
// mint coins | ||
if mintedCoins.Empty() { | ||
// skip as no coins need to be minted | ||
return nil | ||
} | ||
|
||
if err := bankKeeper.MintCoins(ctx, minttypes.ModuleName, mintedCoins); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
// Example of custom send while minting | ||
// Send some tokens to a "team account" | ||
// if err = bankKeeper.SendCoinsFromModuleToAccount(ctx, minttypes.ModuleName, ... ); err != nil { | ||
// return err | ||
// } | ||
|
||
// TODO: figure how to get FeeCollectorName from mint module without generating a cyclic dependency | ||
facundomedica marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err = bankKeeper.SendCoinsFromModuleToModule(ctx, minttypes.ModuleName, authtypes.FeeCollectorName, mintedCoins); err != nil { | ||
return err | ||
} | ||
|
||
if mintedCoin.Amount.IsInt64() { | ||
defer telemetry.ModuleSetGauge(minttypes.ModuleName, float32(mintedCoin.Amount.Int64()), "minted_tokens") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be be called in bank.MintCoins instead? |
||
} | ||
facundomedica marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return env.EventService.EventManager(ctx).EmitKV( | ||
minttypes.EventTypeMint, | ||
event.NewAttribute(minttypes.AttributeKeyBondedRatio, bondedRatio.String()), | ||
event.NewAttribute(minttypes.AttributeKeyInflation, minter.Inflation.String()), | ||
event.NewAttribute(minttypes.AttributeKeyAnnualProvisions, minter.AnnualProvisions.String()), | ||
event.NewAttribute(sdk.AttributeKeyAmount, mintedCoin.Amount.String()), | ||
) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some unit tests on the edge cases would be great |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,11 +6,13 @@ import ( | |
|
||
"cosmossdk.io/collections" | ||
"cosmossdk.io/core/appmodule" | ||
"cosmossdk.io/core/event" | ||
"cosmossdk.io/log" | ||
"cosmossdk.io/math" | ||
"cosmossdk.io/x/mint/types" | ||
|
||
"github.com/cosmos/cosmos-sdk/codec" | ||
"github.com/cosmos/cosmos-sdk/telemetry" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
) | ||
|
||
|
@@ -101,3 +103,72 @@ func (k Keeper) MintCoins(ctx context.Context, newCoins sdk.Coins) error { | |
func (k Keeper) AddCollectedFees(ctx context.Context, fees sdk.Coins) error { | ||
return k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, k.feeCollectorName, fees) | ||
} | ||
|
||
func (k Keeper) DefaultMintFn(ctx context.Context, env appmodule.Environment, minter *types.Minter) error { | ||
stakingTokenSupply, err := k.StakingTokenSupply(ctx) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
bondedRatio, err := k.BondedRatio(ctx) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
params, err := k.Params.Get(ctx) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
minter.Inflation = minter.NextInflationRate(params, bondedRatio) | ||
minter.AnnualProvisions = minter.NextAnnualProvisions(params, stakingTokenSupply) | ||
|
||
mintedCoin := minter.BlockProvision(params) | ||
mintedCoins := sdk.NewCoins(mintedCoin) | ||
maxSupply := params.MaxSupply | ||
totalSupply := stakingTokenSupply | ||
|
||
// if maxSupply is not infinite, check against max_supply parameter | ||
if !maxSupply.IsZero() { | ||
if totalSupply.Add(mintedCoins.AmountOf(params.MintDenom)).GT(maxSupply) { | ||
// calculate the difference between maxSupply and totalSupply | ||
diff := maxSupply.Sub(totalSupply) | ||
// mint the difference | ||
diffCoin := sdk.NewCoin(params.MintDenom, diff) | ||
diffCoins := sdk.NewCoins(diffCoin) | ||
|
||
// mint coins | ||
if err := k.MintCoins(ctx, diffCoins); err != nil { | ||
return err | ||
} | ||
mintedCoins = diffCoins | ||
} | ||
} | ||
|
||
// mint coins if maxSupply is infinite or total staking supply is less than maxSupply | ||
if maxSupply.IsZero() || totalSupply.Add(mintedCoins.AmountOf(params.MintDenom)).LT(maxSupply) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same issue when total+minted == max that is ignored There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Left this one as is to avoid changing the current behavior |
||
// mint coins | ||
if err := k.MintCoins(ctx, mintedCoins); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
// send the minted coins to the fee collector account | ||
// TODO: figure out a better way to do this | ||
err = k.AddCollectedFees(ctx, mintedCoins) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if mintedCoin.Amount.IsInt64() { | ||
defer telemetry.ModuleSetGauge(types.ModuleName, float32(mintedCoin.Amount.Int64()), "minted_tokens") | ||
} | ||
|
||
return env.EventService.EventManager(ctx).EmitKV( | ||
types.EventTypeMint, | ||
event.NewAttribute(types.AttributeKeyBondedRatio, bondedRatio.String()), | ||
event.NewAttribute(types.AttributeKeyInflation, minter.Inflation.String()), | ||
event.NewAttribute(types.AttributeKeyAnnualProvisions, minter.AnnualProvisions.String()), | ||
event.NewAttribute(sdk.AttributeKeyAmount, mintedCoin.Amount.String()), | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,8 @@ message Minter { | |
(gogoproto.customtype) = "cosmossdk.io/math.LegacyDec", | ||
(gogoproto.nullable) = false | ||
]; | ||
|
||
map<string, string> arbitrary_params = 3; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addition of + // arbitrary_params can be used to pass custom parameters for minting operations.
|
||
} | ||
|
||
// Params defines the parameters for the x/mint module. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in simapp? I would expect this code to be in the mint module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is the example that developers can use to develop their own mint function. The idea here is that anyone creates their own minting function if they want to, and it works without requiring direct usage of keepers as it uses the query routers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! Personally, I would prefer something like
x/mint/example
. I do understand your point of keeping it decoupled but it still relates more the mint package than to simapp for me. This is personal preference of course.