Skip to content

Conversation

@murisi
Copy link
Collaborator

@murisi murisi commented Oct 28, 2024

Describe your changes

An attempt to fix the MASP so that rewards can always be withdrawn without restrictions and to also enable the unshielding of untimestamped assets in all circumstances (including those where an asset is subsequently removed from the reward set). More specifically, the following changes have been made:

  • Removed the normed asset computations because they were harder to reason about and encouraged mistakes in their maintenance
  • Where possible, replaced computations over sums of AssetTypes with computations over sums of Addresses to reduce the chances that mismatched epochs cause parts of a balance to be hidden
  • Adjusted the MASP fee UX so that users always have to specify which single MASP key they want to use to pay the fees. This is done instead of trying to construct this value from left over change
  • Removed the fee computation logic and subsumed it into the logic for constructing ordinary transaction inputs and outputs. The purpose was to simplify code maintenance.
  • Added integration tests to verify that MASP rewards can be withdrawn without withdrawing the principle and to also verify that the principal amount can be withdrawn though it is untimestamped

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:

@codecov
Copy link

codecov bot commented Oct 28, 2024

Codecov Report

Attention: Patch coverage is 89.47368% with 24 lines in your changes missing coverage. Please review.

Project coverage is 73.94%. Comparing base (2107072) to head (b28dbc5).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/shielded_token/src/masp/shielded_wallet.rs 93.61% 12 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    #3954      +/-   ##
==========================================
- Coverage   74.00%   73.94%   -0.07%     
==========================================
  Files         341      341              
  Lines      106615   106383     -232     
==========================================
- Hits        78901    78660     -241     
- Misses      27714    27723       +9     

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

@grarco grarco requested review from batconjurer, grarco and sug0 and removed request for grarco and sug0 October 28, 2024 13:52
@brentstone brentstone marked this pull request as ready for review October 28, 2024 16:48
});
assert!(captured.result.is_ok());
assert!(captured.contains("nam: 0"));
assert!(captured.contains("nam: 1000"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these changes to due to the new way we handle the changes without the Changes type?

@brentstone brentstone added this to the v0.45.0 milestone Nov 2, 2024
@sug0 sug0 marked this pull request as draft November 2, 2024 11:03
@sug0 sug0 closed this Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants