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

fix: update AnnualProvisions once per year #1871

Merged
merged 11 commits into from
Jun 5, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jun 2, 2023

Closes #1870

Note: this wasn't caught by the existing TestAnnualProvisions unit test because the test app used stake as its bond denomination but the mint module uses utia as the default bond denomination. So this PR also updates the test app to use utia as the default bond denomination. After this change, new tokens that get emitted due to inflation increase the total supply of utia which will impact the annual provisions calculation.

@rootulp rootulp added the bug Something isn't working label Jun 2, 2023
@rootulp rootulp self-assigned this Jun 2, 2023
@rootulp rootulp marked this pull request as ready for review June 2, 2023 21:39
stakingGenesis := stakingtypes.NewGenesisState(stakingtypes.DefaultParams(), validators, delegations)
genesisState[stakingtypes.ModuleName] = app.AppCodec().MustMarshalJSON(stakingGenesis)
params := stakingtypes.DefaultParams()
params.BondDenom = app.BondDenom
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this line makes the test app's staking module use utia instead of stake

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2023

Codecov Report

Merging #1871 (abcbb6e) into main (348f795) will increase coverage by 21.85%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           main    #1871       +/-   ##
=========================================
+ Coverage      0   21.85%   +21.85%     
=========================================
  Files         0      117      +117     
  Lines         0    13304    +13304     
=========================================
+ Hits          0     2907     +2907     
- Misses        0    10107    +10107     
- Partials      0      290      +290     
Impacted Files Coverage Δ
x/mint/types/minter.go 74.41% <ø> (ø)
x/mint/abci.go 93.44% <100.00%> (ø)

... and 115 files with indirect coverage changes

evan-forbes
evan-forbes previously approved these changes Jun 2, 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.

really nice catch!

x/mint/abci.go Outdated
Comment on lines 38 to 43
if newInflationRate == minter.InflationRate {

isNonZeroAnnualProvisions := !minter.AnnualProvisions.IsZero()
if newInflationRate.Equal(minter.InflationRate) && isNonZeroAnnualProvisions {
// The minter's InflationRate AnnualProvisions already reflect the
// values for this year. Exit early because we don't need to update
// them.
Copy link
Member

Choose a reason for hiding this comment

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

the good news is this appears to not break state or the result of the inflation, but previous to this change, this code was literally never getting hit, so we would always end up updating inflation. That might actually still change the apphash even if the state is not changing, depending on if this is causing an insertion to the iavl tree

@evan-forbes
Copy link
Member

evan-forbes commented Jun 3, 2023

@rootulp while we're fixing this, can we also only call SetMinter only once here? I don't think there is any benefit to calling this more than once, if anything only weirdness could occur. It's also less efficient, although I'm not too worried about that given it is not a hot path.

It could also be also definitely be done in a separate PR, but we should do it before cutting the next release

minter.InflationRate = newInflationRate
k.SetMinter(ctx, minter)
totalSupply := k.StakingTokenSupply(ctx)
minter.AnnualProvisions = minter.CalculateAnnualProvisions(totalSupply)
k.SetMinter(ctx, minter)

@rootulp
Copy link
Collaborator Author

rootulp commented Jun 5, 2023

can we also only call SetMinter only once here?

Sure done in 2d7dab5

@rootulp rootulp requested a review from evan-forbes June 5, 2023 03:22
@evan-forbes evan-forbes merged commit 6e5f418 into celestiaorg:main Jun 5, 2023
15 checks passed
evan-forbes pushed a commit that referenced this pull request Jun 5, 2023
* test: use utia as bond denom in test app

* implement failing test

* refactor test to use test cases

* green test

* fix: TestAnnualProvisions

* consolidate tests

* remove comment

* only call SetMinter once

* add another test

* fix lint
@rootulp rootulp deleted the rp/fix-maybe-update-minter branch June 5, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AnnualProvisions may be updated more than once per year
4 participants