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

Masp epoch transition proof fix #2222

Merged
merged 5 commits into from
Dec 29, 2023
Merged

Conversation

grarco
Copy link
Collaborator

@grarco grarco commented Nov 28, 2023

Describe your changes

Addresses #2094.
Closes #307 (@murisi not completely sure about this).
Closes #1880.

Allows to unshield assets from the MASP that belong to older epochs.

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

v0.27.0

Checklist before merging to draft

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

@grarco grarco marked this pull request as ready for review November 28, 2023 16:14
@grarco grarco requested a review from murisi November 28, 2023 16:14
@grarco
Copy link
Collaborator Author

grarco commented Nov 28, 2023

Note: the integration tests are actually passing for me locally, they fail here because of a time limit on the CI step

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.

Looks good to me, thanks! Can I assume that the integration tests don't take significantly more time to execute on your machine than before your changes?

// Test that a masp unshield transaction can be succesfully executed even across
// an epoch boundary.
#[test]
fn cross_epoch_tx() -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can I assume that assume that CI integration test failure is due solely to the additional time taken by this test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I can confirm, all the masp integration tests pass locally, seems to be just due to the timeout on the CI step

@grarco grarco mentioned this pull request Dec 4, 2023
@tzemanovic tzemanovic mentioned this pull request Dec 7, 2023
tzemanovic added a commit that referenced this pull request Dec 8, 2023
* origin/grarco/masp-cross-epoch-proof-fix:
  Changelog #2222
  Updates masp fixtures for tests
  Refactors `convert_amount`
  Tests masp cross-epoch unshield
  Allows unshielding assets from previous epochs
@grarco grarco mentioned this pull request Dec 15, 2023
2 tasks
brentstone added a commit that referenced this pull request Dec 29, 2023
* origin/grarco/masp-cross-epoch-proof-fix:
  Changelog #2222
  Updates masp fixtures for tests
  Refactors `convert_amount`
  Tests masp cross-epoch unshield
  Allows unshielding assets from previous epochs
@brentstone brentstone merged commit 58d6576 into main Dec 29, 2023
13 of 14 checks passed
@brentstone brentstone deleted the grarco/masp-cross-epoch-proof-fix branch December 29, 2023 19:37
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.

Retry transactions on fee unshielding epoch error Handle MASP Transactions Across Epoch Boundaries
3 participants