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

Implemented compounding inflation for native tokens. #1985

Merged
merged 6 commits into from
Oct 24, 2023

Conversation

murisi
Copy link
Contributor

@murisi murisi commented Oct 13, 2023

Describe your changes

Implemented compounding inflation for native tokens and now use a PD-controller to determine inflation values. More specifically, the changes are as follows:

  • Implemented compounding inflation for native tokens so that users can now get correspondingly denominated rewards for holding NAM tokens.
  • Generalized the PD-controller to allow the token used for computing the shielded pool inflation cap to be different from the one used to compute the locked token amount. And then implemented inflation based on this PD-controller's outputs.
  • Updated the masp_incentives integration test so that it no longer assumes fixed reward rates, but instead has PD-controller outputs hardcoded into it.
  • Increased the precision of inflation computations for each asset so that high denomination tokens like ETH will be able to receive non-zero NAM rewards. This involved representing reward components as i128s.

This PR is based on:

Indicate on which release or other PRs this topic is based on

v0.23.0

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@murisi murisi force-pushed the murisi/compounding-rewards branch 2 times, most recently from 0380ea2 to 640003c Compare October 16, 2023 11:43
Copy link
Member

@mariari mariari left a comment

Choose a reason for hiding this comment

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

I can do a more thorough review later, but looking now, nothing looks too out of the ordinary, I would like to see more what was changed, I should diff this against the mess of a current branch @juped and I worked on.

Was the solution just running some of the calculations only once instead of 4 times?

Comment on lines +172 to +173
// Operations that happen exactly once for each token
if denom == MaspDenom::Three {
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, any reason why denom::Three was chosen?

I guess before it would call this 4 times, causing issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that the body of this conditional was called 4 times before. This denom == MaspDenom::Three check became necessary to stop repetitions once I had moved the logic occurring before the for denom in token::MaspDenom::iter() { loop into the for denom in token::MaspDenom::iter() { loop. I made this movement as a matter of convenience, but I'm sure that there are other ways this logic could be structured.

I chose denom::Three because the conditional's body modifies variables (namely total_reward and normed_inflation) that would affect the next iteration of for denom in token::MaspDenom::iter() {. In the current context, the normed_inflation can only be modified once we have finished working with the native token.

Comment on lines 125 to 134
crate::types::uint::Uint::try_into(
(inflation.raw_amount() * PRECISION)
/ total_token_in_masp.raw_amount(),
)
.unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

Is this any different? Otherwise I don't see much about this which is different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other than use of unsigned amounts, this code is probably the same as what's in the other branches/PRs.

@murisi murisi force-pushed the murisi/compounding-rewards branch 2 times, most recently from a242c78 to a625126 Compare October 17, 2023 16:05
@murisi murisi marked this pull request as ready for review October 17, 2023 17:22
@murisi murisi mentioned this pull request Oct 17, 2023
core/src/types/token.rs Outdated Show resolved Hide resolved
core/src/types/token.rs Outdated Show resolved Hide resolved
core/src/types/token.rs Outdated Show resolved Hide resolved
core/src/types/token.rs Outdated Show resolved Hide resolved
@brentstone
Copy link
Contributor

In Shell::apply_inflation of finalize_block.rs, there is some dummy MASP inflation code that should be removed, since this now effectively occurs in update_allowed_conversions

@murisi murisi force-pushed the murisi/compounding-rewards branch 3 times, most recently from c0f2add to 4270900 Compare October 18, 2023 08:06
apps/src/lib/config/genesis.rs Show resolved Hide resolved
core/src/ledger/inflation.rs Outdated Show resolved Hide resolved
core/src/ledger/storage/masp_conversions.rs Outdated Show resolved Hide resolved
@murisi
Copy link
Contributor Author

murisi commented Oct 18, 2023

I can do a more thorough review later, but looking now, nothing looks too out of the ordinary, I would like to see more what was changed, I should diff this against the mess of a current branch @juped and I worked on.

Was the solution just running some of the calculations only once instead of 4 times?

Hi! I assume you want this PR to be differentiated from ray/0.23-rewards? I took most of what you and @juped had implemented and mostly just altered the arithmetic in update_allowed_conversions and upgraded the masp_incentives integration test. The main changes in this PR:

  • The reward asset for a token of MaspDenom i (=0,1,2,3) also has a reward of MaspDenom i. Doing this should allow rewards to be proportional to owned shielded tokens regardless of their denomination.
  • The reference inflation is set to a larger value in order to allow a greater variety/set of fractions of it to be distributed as rewards. More concretely, if ref_inflation == 1, then initially normed_inflation = 1, which means that a new_normed_inflation equal to 1, 2, or 3 represent a limited choice between a 0%, 100%, or 200% inflation. Whereas if ref_inflation were to equal 100, then initially ref_inflation == 100, which means that a new_normed_inflation of say 163 represents a 63% inflation. A higher ref_inflation allows for more accurate inflation distribution.
  • In the case of computing rewards for Namada tokens:
    • Since reward.0 / reward.1 conceptually represents a percentage increase in money supply, this PR gives x*(1+(reward.0 / reward.1)) current-epoch-NAM tokens for every x previous-epoch-NAM tokens. If we only dished out normed_inflation * ((reward.0 / normed_inflation) / reward.1), or (almost) equivalently reward.0 / reward.1 tokens, then it seems that users would lose their principal shielded amount in the process of receiving their rewards for it.
    • The total_reward is now increased by (addr_bal / normed_inflation) * new_normed_inflation in order to support fractional inflation. Were normed_inflation to be assumed to bee I256::from(1), we would lose accuracy in the transparent inflation being able to choose only multiples of 100%.
  • In calculate_masp_rewards, the PD-controller now computes the shielded pool inflation cap using the current supply of NAM instead of the current supply of addr. This seems to be closer to what's in https://specs.namada.net/economics/inflation-system, unless if that document is now outdated.

@murisi murisi force-pushed the murisi/compounding-rewards branch 2 times, most recently from e33446b to 8064a89 Compare October 20, 2023 08:29
@murisi murisi force-pushed the murisi/compounding-rewards branch 3 times, most recently from 0147ea1 to 89aca83 Compare October 20, 2023 09:50
Fraccaman added a commit that referenced this pull request Oct 23, 2023
* origin/murisi/compounding-rewards:
  Added changelog entry.
  Implemented a mul_div operation for Uints and reduced overflow risks in inflation computations.
  make token amounts in `RewardsController` of `Uint` type
  Increased the precision of MASP rewards.
  Integrated PD controller support.
  Implemented compounding inflation for native tokens.
tzemanovic added a commit that referenced this pull request Oct 24, 2023
* origin/murisi/compounding-rewards:
  Added changelog entry.
  Implemented a mul_div operation for Uints and reduced overflow risks in inflation computations.
  make token amounts in `RewardsController` of `Uint` type
  Increased the precision of MASP rewards.
  Integrated PD controller support.
  Implemented compounding inflation for native tokens.
@tzemanovic tzemanovic mentioned this pull request Oct 24, 2023
@tzemanovic tzemanovic merged commit 2e3e640 into main Oct 24, 2023
11 of 14 checks passed
@tzemanovic tzemanovic deleted the murisi/compounding-rewards branch October 24, 2023 11:26
brentstone pushed a commit that referenced this pull request Nov 11, 2023
* origin/murisi/compounding-rewards:
  Added changelog entry.
  Implemented a mul_div operation for Uints and reduced overflow risks in inflation computations.
  make token amounts in `RewardsController` of `Uint` type
  Increased the precision of MASP rewards.
  Integrated PD controller support.
  Implemented compounding inflation for native tokens.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants