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

Test enabling rewards after shielding and large amounts in the masp #3959

Merged
merged 30 commits into from
Nov 4, 2024

Conversation

sug0
Copy link
Collaborator

@sug0 sug0 commented Oct 29, 2024

Describe your changes

Based on #3954 (DIFF)
Rebases commits from #3954, therefore it essentially supersedes it.

This PR accomplishes the following:

  • Fixes clippy lints
  • Fixes formatting
  • Adds new storage functions to update the masp token map
  • Adds a new masp integration test to check the behavior of enabling rewards on some asset that had already been shielded
  • Adds a new masp integration test to check the behavior of interacting with tokens in the masp whose values span across multiple notes (because to represent them we need more than 64 bits)
  • Adds a new masp integration test to check the behavior of initializing the token map with null rewards, shielding, then updating the rewards to some non-null value
  • Modified change computation to deal with each individual masp denomination separately

To check the rest of the changes, check the description of #3954.

Checklist before merging

  • If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
    • This will require 2 reviewers to approve the changes
  • If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
    • Relevant PR if applies:
  • If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
    • Relevant PR if applies:

Copy link
Collaborator

@murisi murisi left a comment

Choose a reason for hiding this comment

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

This is a review of just 1f05f77 . I think this approach is the way to go, thanks! Ah, I see the new MASP integration test is still failing. Let me see if I can spot the error...

My guess is that this issue is similar to having a shielded balance with the single note 1 [2^64 Wei] and trying to spend/send 1 [2^0 Wei] to another payment address. Though these two asset types concern the same underlying token, they are not convertible, and the MASP cannot use/break down the former amount (as far as I remember) to send the much smaller output. You would first need to unshield the 1 [2^64 Wei] note to get a transparent balance of 2^64 Wei, and then shield (2^64 - 1) [2^0 Wei] back to yourself and the other 1 [2^0 Wei] to your desired destination. So essentially the Insufficient funds error would be correct in a sense. This is my guess based on how this test also broke #3954 ...

If the above guess is correct, then the resolution to this error is to adjust the amounts worked with in the new test to stop straddling the MASP digit boundaries. Or more adventurously, do the unshield-then-shield trick above. But I would lean towards the former solution and make a separate test to demonstrate the above inconvertibility behaviour.

crates/shielded_token/src/masp/shielded_wallet.rs Outdated Show resolved Hide resolved
crates/shielded_token/src/masp/shielded_wallet.rs Outdated Show resolved Hide resolved
crates/shielded_token/src/masp/shielded_wallet.rs Outdated Show resolved Hide resolved
crates/shielded_token/src/masp/shielded_wallet.rs Outdated Show resolved Hide resolved
crates/shielded_token/src/masp/shielded_wallet.rs Outdated Show resolved Hide resolved
crates/shielded_token/src/masp/shielded_wallet.rs Outdated Show resolved Hide resolved
@sug0 sug0 force-pushed the tiago+murisi/fix-masp-change branch from 1b41cbb to c1ef340 Compare October 31, 2024 09:13
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 89.28571% with 33 lines in your changes missing coverage. Please review.

Project coverage is 73.94%. Comparing base (a148ac3) to head (6cd9e94).
Report is 31 commits behind head on main.

Files with missing lines Patch % Lines
crates/shielded_token/src/masp/shielded_wallet.rs 93.69% 15 Missing ⚠️
crates/core/src/token.rs 62.50% 6 Missing ⚠️
crates/sdk/src/lib.rs 0.00% 5 Missing ⚠️
crates/core/src/uint.rs 83.33% 4 Missing ⚠️
crates/sdk/src/args.rs 0.00% 2 Missing ⚠️
crates/sdk/src/tx.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3959      +/-   ##
==========================================
- Coverage   73.99%   73.94%   -0.05%     
==========================================
  Files         341      341              
  Lines      106614   106450     -164     
==========================================
- Hits        78888    78715     -173     
- Misses      27726    27735       +9     

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

@sug0 sug0 requested review from batconjurer and grarco October 31, 2024 17:09
grarco added a commit that referenced this pull request Oct 31, 2024
sug0 added a commit that referenced this pull request Oct 31, 2024
@sug0 sug0 force-pushed the tiago+murisi/fix-masp-change branch from acafd29 to c3ba7f7 Compare October 31, 2024 17:30
@sug0
Copy link
Collaborator Author

sug0 commented Oct 31, 2024

@brentstone I think this PR might require some docs changes, because it changes the behavior of --disposable-gas-payer

@sug0 sug0 added the MASP label Oct 31, 2024
0u8.into(),
)?;

// Give Bertha some test tokens
Copy link
Member

@batconjurer batconjurer Nov 1, 2024

Choose a reason for hiding this comment

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

Understatement of the year. Bertha has all of the tokens! 🤣

@sug0 sug0 changed the title Test enabling rewards after shielding Test enabling rewards after shielding and large amounts in the masp Nov 1, 2024
@brentstone brentstone added this to the v0.45.0 milestone Nov 2, 2024
sug0 added a commit that referenced this pull request Nov 2, 2024
@sug0 sug0 force-pushed the tiago+murisi/fix-masp-change branch from c3ba7f7 to b771524 Compare November 2, 2024 16:55
sug0 added a commit that referenced this pull request Nov 2, 2024
@sug0 sug0 force-pushed the tiago+murisi/fix-masp-change branch from b771524 to d923da9 Compare November 2, 2024 17:31
sug0 added a commit that referenced this pull request Nov 2, 2024
@sug0 sug0 force-pushed the tiago+murisi/fix-masp-change branch from d923da9 to 0a06602 Compare November 2, 2024 17:40
@sug0 sug0 force-pushed the tiago+murisi/fix-masp-change branch from 0a06602 to 6cd9e94 Compare November 4, 2024 11:36
@sug0 sug0 requested a review from batconjurer November 4, 2024 11:36
// NB: the max reward rate needs to be quite big, to allow
// the inflation being computed by the pd controller to
// exceed the amount of test tokens in the masp
max_reward_rate: Dec::from_str("999999999999999.0").unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

dayum

@brentstone brentstone added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Nov 4, 2024
@mergify mergify bot merged commit a11be9b into main Nov 4, 2024
24 checks passed
@mergify mergify bot deleted the tiago+murisi/fix-masp-change branch November 4, 2024 17:12
@sug0 sug0 mentioned this pull request Nov 7, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking:cli breaking:SDK MASP merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants