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

Murisi/simplify namada masp rewards #2699

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

murisi
Copy link
Contributor

@murisi murisi commented Feb 22, 2024

Describe your changes

Experimented with modifying the reward distribution algorithm so that native token rewards are distributed in the same way as non-native tokens. Doing this causes a slight variation in the shielded rewards distributed for the default set of parameters and hence necessitated slight changes in the expectations of the integration tests.

At a high level, these changes mean that now all native tokens with an epoch other than 0 receive their rewards as epoch 0 native tokens (just the same as it's done for non-native tokens). The exception to this rule is epoch 0 native tokens, which have a direct conversion to the current epoch (i.e. a conversion term like 100NAM@ep30 - 10NAM@ep0). Hence reward claiming for everything except NAM@ep0 involves claiming a NAM@ep0 reward and then converting that to NAM@epN.

The following list describes the benefits/drawbacks of this approach:

  • Benefit: simpler/shorter reward distribution code (due to native token branch being fused with non-native branch)
  • Benefit: ability to claim rewards even for small amounts of NAM
    • Note: this benefit is small because these small amounts are insiginificant after inflation adjustment
  • Drawback: the rewards become significantly less accurate over time as a "denominator" approaches 0
    • This "denominator" is the negative term of the native token conversion from epoch 0 to the current epoch

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

#2695

Checklist before merging to draft

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

@murisi murisi requested review from cwgoes and grarco February 22, 2024 16:57
@murisi murisi force-pushed the murisi/simplify-namada-masp-rewards branch from 071accd to 901345e Compare February 22, 2024 16:58
Copy link

codecov bot commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 9.18635% with 346 lines in your changes are missing coverage. Please review.

Project coverage is 53.32%. Comparing base (c733be2) to head (5890ef8).
Report is 77 commits behind head on main.

Files Patch % Lines
crates/sdk/src/masp.rs 0.00% 332 Missing ⚠️
crates/shielded_token/src/conversion.rs 71.42% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2699      +/-   ##
==========================================
- Coverage   53.38%   53.32%   -0.06%     
==========================================
  Files         302      302              
  Lines      103403   103431      +28     
==========================================
- Hits        55198    55159      -39     
- Misses      48205    48272      +67     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@murisi murisi force-pushed the murisi/simplify-namada-masp-rewards branch from 901345e to 0d1f7d9 Compare February 22, 2024 17:47
@murisi murisi force-pushed the murisi/simplify-namada-masp-rewards branch from 0d1f7d9 to 5890ef8 Compare February 22, 2024 18:29
@murisi murisi added the MASP label Feb 23, 2024
@brentstone
Copy link
Collaborator

what's the status of this? Is it still desired or necessary?

@murisi
Copy link
Contributor Author

murisi commented Apr 24, 2024

what's the status of this? Is it still desired or necessary?

It may be necessary. I'll be looking at integrating it soon.

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.

2 participants