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

Fixes MASP tx expiration in the SDK #3724

Merged
merged 12 commits into from
Sep 6, 2024
Merged

Conversation

grarco
Copy link
Contributor

@grarco grarco commented Aug 29, 2024

Describe your changes

Closes #3586.

  • Fixes a bug by which the SDK would not set the correct expiration to MASP Transactions
  • Adds an integration test for masp expiration
  • Removes some outdated assertions based on the old double-block execution scheme
  • Improves assertions on MockNode
  • Wraps MockNode into a single Arc

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

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 72.49%. Comparing base (ef49b3d) to head (f41e88a).
Report is 34 commits behind head on main.

Files with missing lines Patch % Lines
crates/sdk/src/tx.rs 66.66% 2 Missing ⚠️
crates/shielded_token/src/masp/shielded_wallet.rs 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3724      +/-   ##
==========================================
- Coverage   72.50%   72.49%   -0.01%     
==========================================
  Files         338      338              
  Lines      104190   104191       +1     
==========================================
- Hits        75541    75538       -3     
- Misses      28649    28653       +4     

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

grarco added a commit that referenced this pull request Aug 30, 2024
@grarco grarco marked this pull request as ready for review August 30, 2024 14:48
@grarco grarco requested review from sug0 and yito88 August 30, 2024 15:26
crates/sdk/src/tx.rs Outdated Show resolved Hide resolved
grarco added a commit that referenced this pull request Sep 2, 2024
Copy link
Contributor

@sug0 sug0 left a comment

Choose a reason for hiding this comment

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

couple of suggestions + comments

crates/node/src/shell/testing/node.rs Outdated Show resolved Hide resolved
crates/apps_lib/src/cli.rs Outdated Show resolved Hide resolved
crates/tests/src/integration/masp.rs Show resolved Hide resolved
crates/tests/src/integration/masp.rs Outdated Show resolved Hide resolved
crates/tests/src/integration/masp.rs Show resolved Hide resolved
grarco added a commit that referenced this pull request Sep 4, 2024
@sug0 sug0 self-requested a review September 5, 2024 09:09
grarco added a commit that referenced this pull request Sep 6, 2024
@grarco grarco added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Sep 6, 2024
mergify bot added a commit that referenced this pull request Sep 6, 2024
@mergify mergify bot merged commit ee4649b into main Sep 6, 2024
23 checks passed
@mergify mergify bot deleted the grarco/fix-masp-expiration branch September 6, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

MASP txs do not respect tx expiration date set in args
3 participants