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

Unify MASP refs events #3821

Merged
merged 9 commits into from
Sep 18, 2024
Merged

Unify MASP refs events #3821

merged 9 commits into from
Sep 18, 2024

Conversation

grarco
Copy link
Contributor

@grarco grarco commented Sep 14, 2024

Describe your changes

Unifies the previous two masp refs events into a single MaspTxBatchRefs event: this ensures that no reordering of masp data can happen on the client side that would lead to the construction of an invalid state.

Updates the IndexedTx type to carry an optional pointer to a specific inner tx inside a batch. This fixes an issue for which the shielded sync command could not properly process multiple masp transactions in a same batch: it was appending the decrypted tx to a map using as an index the same block height and tx index, effectively taking into account only the last masp transaction of a batch

TODOs:

  • See if there's a better way to emit the masp ref event (maybe without the need for ExtendedTxResult) (Reduce masp events #3824 )
  • Update the indexer

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

@grarco grarco marked this pull request as ready for review September 16, 2024 17:27
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 74.17582% with 47 lines in your changes missing coverage. Please review.

Project coverage is 72.80%. Comparing base (e603759) to head (561f604).
Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
crates/sdk/src/masp.rs 42.37% 34 Missing ⚠️
crates/sdk/src/masp/utilities.rs 80.00% 6 Missing ⚠️
crates/core/src/masp.rs 0.00% 3 Missing ⚠️
crates/node/src/protocol.rs 90.00% 1 Missing ⚠️
...hielded_token/src/masp/shielded_sync/dispatcher.rs 97.56% 1 Missing ⚠️
crates/shielded_token/src/masp/shielded_wallet.rs 95.45% 1 Missing ⚠️
crates/tx/src/types.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3821      +/-   ##
==========================================
+ Coverage   72.79%   72.80%   +0.01%     
==========================================
  Files         338      338              
  Lines      103995   103989       -6     
==========================================
+ Hits        75701    75708       +7     
+ Misses      28294    28281      -13     

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

@brentstone brentstone added this to the v0.44.0 milestone Sep 18, 2024
@brentstone
Copy link
Collaborator

Make an issue for better way to emit MASP ref event (in your TODO)?

@grarco
Copy link
Contributor Author

grarco commented Sep 18, 2024

Make an issue for better way to emit MASP ref event (in your TODO)?

Done with #3824

@tzemanovic tzemanovic added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Sep 18, 2024
mergify bot added a commit that referenced this pull request Sep 18, 2024
@mergify mergify bot merged commit 28ebf30 into main Sep 18, 2024
24 checks passed
@mergify mergify bot deleted the grarco/fix-masp-events branch September 18, 2024 15:33
@tzemanovic tzemanovic mentioned this pull request Sep 19, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking:client breaking:SDK breaking-change bug Something isn't working IBC ledger 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.

4 participants