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(inflation): daily epoch mint provision #1193

Merged
merged 11 commits into from Jan 5, 2023
Merged

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Dec 23, 2022

Description

Closes: ENG-1207


All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

PR review checkboxes:

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • included the correct type prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link in the PR description to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all required CI checks have passed

Code maintenance:

I have...

  • written unit and integration tests
  • added relevant godoc and code comments.
  • updated relevant documentation (docs/) or specification (x/<module>/spec/)

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code

@linear
Copy link

linear bot commented Dec 23, 2022

ENG-1207 Inflation is only recalculate once a year, even when parameters change

Context

We want to create a proposal to change inflation, but there is a bug that prevents is. Check out the Specification.

Current

The inflation module mints new tokens every day (through hooks). To calculate how many tokens are minted per day (epoch mint provision), we calculate the total amount per year (period provision) and divide it by the epochs per year. The calculation, however, is only performed once a year to adjust inflation on a yearly basis, so that the same amount of tokens are minted every day for one year (illustration from token model)

The inflation parameters that calculate the inflation can be changed through governance. Since the epoch mint provision is only recalculated once a year, a proposal to change these parameters wouldn’t change the inflation until next April (one year after genesis).

evmosd q inflation params --node https://tendermint.bd.evmos.org:26657
enable_inflation: true
exponential_calculation:
  a: "300000000.000000000000000000"
  bonding_target: "0.660000000000000000"
  c: "9375000.000000000000000000"
  max_variance: "0.000000000000000000"
  r: "0.500000000000000000"
inflation_distribution:
  community_pool: "0.133333333000000000"
  staking_rewards: "0.533333334000000000"
  usage_incentives: "0.333333333000000000"
mint_denom: aevmos

Expected - Change calculation to run daily but keep yearly intervals

Inflation should be recalculated every day, so that it reacts to fluctuations in calculation parameters (e.g. bonding ratio or bonding target) without delay.

  • move CalculateEpochMintProvision out of check if period has passed. Place before MintAndAllocateInflation so that inflation is calcualted every day instead of just when a period passes
  • We don’t need to store EpochMintProvision in the store anymore, remove it from the store and its getter and setters. Queries need to be adjusted to calculate EpochMintProvision on the fly and push the response to the client
  • Update tests and documentation

Test cases

  • Epoch mint provision is recalculate every day using the current parameters
  • If inflation parameters change (e.g. through governance), they are used for the next calculation => effective during calculation on next day.

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #1193 (a1d86b1) into main (0e0a566) will increase coverage by 0.07%.
The diff coverage is 91.66%.

❗ Current head a1d86b1 differs from pull request most recent head e5b70a8. Consider uploading reports for the commit e5b70a8 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1193      +/-   ##
==========================================
+ Coverage   76.34%   76.41%   +0.07%     
==========================================
  Files         149      148       -1     
  Lines        8099     8073      -26     
==========================================
- Hits         6183     6169      -14     
+ Misses       1716     1708       -8     
+ Partials      200      196       -4     
Impacted Files Coverage Δ
app/app.go 81.70% <0.00%> (-0.12%) ⬇️
x/inflation/keeper/inflation.go 82.17% <90.90%> (+2.39%) ⬆️
x/inflation/keeper/grpc_query.go 100.00% <100.00%> (+8.57%) ⬆️
x/inflation/keeper/hooks.go 65.90% <100.00%> (-3.03%) ⬇️

@github-actions github-actions bot added the docs label Jan 4, 2023
@4rgon4ut 4rgon4ut marked this pull request as ready for review January 4, 2023 23:12
@4rgon4ut 4rgon4ut requested review from ramacarlucho and a team as code owners January 4, 2023 23:12
@4rgon4ut 4rgon4ut requested review from facs95 and removed request for a team January 4, 2023 23:12
Copy link
Contributor Author

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM, note that we also need to do a migration o remove the EpochMintProvision from the store in a separate PR.

cc @Vvaradinov

Copy link
Contributor

@Vvaradinov Vvaradinov left a comment

Choose a reason for hiding this comment

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

tACK. @fedekunze will follow up with the migration and tests in a separate PR.

@4rgon4ut 4rgon4ut enabled auto-merge (squash) January 5, 2023 11:40
@4rgon4ut 4rgon4ut merged commit 7b4c456 into main Jan 5, 2023
@4rgon4ut 4rgon4ut deleted the fedekunze/inflation-fixes branch January 5, 2023 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants