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

feat: strict inflation schedule #1578

Merged
merged 95 commits into from
May 17, 2023
Merged

feat: strict inflation schedule #1578

merged 95 commits into from
May 17, 2023

Conversation

mojtaba-esk
Copy link
Member

@mojtaba-esk mojtaba-esk commented Mar 31, 2023

Description

Part of #1570
Implement the strict inflation schedule specified in #1584

Remaining tasks

  • implement inflation rate change based on block timestamp rather than block height
  • set genesis time in x/mint state
  • move MintDenom from param to x/mint state
  • remove TestRandomizedGenState
  • investigate why TestQueryGRPC / other integration tests are never run
  • write a test that 1. starts a new chain, 2. verifies state of x/mint module for first few blocks, 3. skips ahead to a block one year later, 4. verifies inflation rate went down a bit
  • remove Params entirely
  • annual provisions for a year should be based on total supply at the beginning of that year
  • for time-based inflation, calculate block provision as current block timestamp - previous block timestamp = seconds elapsed / seconds in a year * annual provisions for a year
  • remove BlocksPerYear
  • elaborate in README.md

FLUPs

@liamsi

This comment was marked as resolved.

x/mint/types/params.go Outdated Show resolved Hide resolved
x/mint/types/params.go Outdated Show resolved Hide resolved
x/mint/types/params.go Outdated Show resolved Hide resolved
@mojtaba-esk mojtaba-esk self-assigned this Mar 31, 2023
@evan-forbes evan-forbes added this to the Mainnet milestone Apr 3, 2023
Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
@rootulp rootulp changed the title feat: inflation rate change feat: strict inflation schedule Apr 20, 2023
@rootulp rootulp self-assigned this Apr 20, 2023
app/test/fuzz_abci_test.go Outdated Show resolved Hide resolved
@evan-forbes
Copy link
Member

also, I think it should be relatively straightforward to write an integration test that measures the exact amount of inflation, and ensures the correct amount (to some level of precision) is being dished out to the pools/funds whatever. I also think we can get a little clever and make the TimeIota consensus parameter very large to force tendermint to add time that never passed to the header. That way, we can simulate time increasing waaaay faster than it actual does and not have to change any constants to watch the rate of inflation decrease/measure a year's worth of inflation.

evan-forbes
evan-forbes previously approved these changes May 16, 2023
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

nice work!! ship ittttt :shipit:

my only complaint is that we didn't get to an even 4200 lines, but I guess we can't get everything perfect
Screenshot from 2023-05-16 11-39-52

@rootulp rootulp requested a review from cmwaters May 16, 2023 18:19
@rootulp rootulp enabled auto-merge (squash) May 16, 2023 18:20
cmwaters
cmwaters previously approved these changes May 17, 2023
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Nice work on this 👍

Comment on lines +23 to +31
// maybeSetGenesisTime sets the genesis time if the current block height is 1.
func maybeSetGenesisTime(ctx sdk.Context, k keeper.Keeper) {
if ctx.BlockHeight() == 1 {
genesisTime := ctx.BlockTime()
minter := k.GetMinter(ctx)
minter.GenesisTime = &genesisTime
k.SetMinter(ctx, minter)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have an alternative proposal to this which you can do in a follow up PR if you choose to. Rather than this, add a line in:

celestia-app/app/app.go

Lines 571 to 578 in 1e60076

func (app *App) InitChainer(ctx sdk.Context, req abci.RequestInitChain) abci.ResponseInitChain {
var genesisState GenesisState
if err := tmjson.Unmarshal(req.AppStateBytes, &genesisState); err != nil {
panic(err)
}
app.UpgradeKeeper.SetModuleVersionMap(ctx, app.mm.GetVersionMap())
return app.mm.InitGenesis(ctx, app.appCodec, genesisState)
}

Something like:

app.MintKeeper.SetGenesisTime(req.Time)

Which will set the genesis time. This also avoids the problem of PreviousBlockTime being nil at height 1

@rootulp rootulp dismissed stale reviews from cmwaters and evan-forbes via c37d4aa May 17, 2023 13:27
@MSevey MSevey requested a review from a team May 17, 2023 13:27
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

githaaaaaaa

@MSevey MSevey requested a review from a team May 17, 2023 17:06
@evan-forbes
Copy link
Member

for posterity, I asked @sweexordious to approve blindly since we've already approved and should start on any follow ups

@rootulp rootulp merged commit 207ec1a into main May 17, 2023
@rootulp rootulp deleted the mojtaba/inflation-rate-change branch May 17, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants