-
Notifications
You must be signed in to change notification settings - Fork 99
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): Implementing inflation in BeginBlocker #311
Conversation
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.
I would like to add a test case.
Mock StakingKeeper
: bonded ratio is constant and within bands (eg: 66%).
Inflation is 10%, adjust context to make it run for a year, with block time always <= params.MaxBlockDuration.
Calculate the expected inflation (note: needs compounding calc).
|
||
// getting last block info | ||
lbi, found := k.GetLastBlockInfo(ctx) | ||
if !found { |
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.
when is the case for which this can be not found?
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.
I dont think there is a case where this will not be found 🤔
x/mint/keeper/minter.go
Outdated
// amount of bond tokens to mint in this block | ||
bondedTokenSupply := k.GetBondedTokenSupply(ctx) | ||
|
||
tokens = blockInflation.MulInt(bondedTokenSupply.Amount).Mul(sdk.NewDecFromBigInt(big.NewInt(int64(elapsed.Seconds()))).QuoInt64(int64(Year.Seconds()))) // amount := (inflation * bondedTokenSupply) * (elapsed/Year) |
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.
in the getBlockInflation
function, we're multiplying the inflation change by NANOSECONDS
whilst here we're dividing by SECONDS
.
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.
Also can we split the inflation * bondedTokenSupply
and the elapsed/year
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.
We can also simplify the calculation timeElapsedRatio = elapsed/year
without any conversion, just using the raw duration, and then multiply it.
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.
I am not sure we can do timeElapsedRatio := elapsed/Year
as both are internally just int64 values. and smallInt/largeInt
will just give 0.
Doing timeElapsedRate := sdk.NewDecFromBigInt(big.NewInt(int64(elapsed))).QuoInt64(int64(Year))
returns the small decimal value. But yea, there could be better ways to do this🤔
mintedTokens, found := k.GetInflationForRewards(ctx) | ||
if found { | ||
// Track inflation rewards | ||
k.TrackInflationRewards(ctx, mintedTokens) |
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.
if it's not found I'm not sure if skipping it is the correct way to do it.
Old params would be applied.
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.
if its not found, that would mean there are no inflation rewards to track right. like it would be 0stake
in the current block
…imulation Fdymylja/add inflation simulation
sdk "github.com/cosmos/cosmos-sdk/types" | ||
) | ||
|
||
const Year = 24 * time.Hour * 365 |
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.
This gets us 31 536 000
seconds per year.
Should we replace this with 31 557 600
seconds to accommodate 4 year avg to handle leap years?
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.
We should prefer the leap year average... it is better for overall financial math calculations;
PR still relevant? |
☠️ Closing cuz time based inflation RIP |
Ref: #287
x/mint
x/rewards
note
❗Reviewer pls check 🙏🏻
sdk.Coin
from asdk.Dec
valuearchway/x/mint/abci.go
Line 49 in e8897c4
archway/x/mint/keeper/minter.go
Line 62 in e8897c4