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

move masp validation from SDK into shielded_token crate #3419

Merged
merged 3 commits into from
Jul 5, 2024

Conversation

tzemanovic
Copy link
Member

@tzemanovic tzemanovic commented Jun 17, 2024

Describe your changes

closes #3101

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

0.39.0

Checklist before merging to draft

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

tzemanovic added a commit that referenced this pull request Jun 17, 2024
@tzemanovic tzemanovic marked this pull request as ready for review June 17, 2024 15:31
@tzemanovic tzemanovic requested a review from grarco June 17, 2024 15:31
grarco
grarco previously approved these changes Jun 17, 2024
Copy link
Contributor

@grarco grarco left a comment

Choose a reason for hiding this comment

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

One minor comment but LGTM!

@grarco grarco dismissed their stale review June 17, 2024 16:02

Ah actually failing unit tests

@tzemanovic tzemanovic mentioned this pull request Jun 17, 2024
15 tasks
grarco
grarco previously approved these changes Jun 18, 2024
@tzemanovic
Copy link
Member Author

tzemanovic commented Jun 24, 2024

The make check-crates was still failing, but is now fixed in b588224 - I had to re-introduce the download-params feature as the MASP VP must always be available even without it for the SDK to compile

@tzemanovic tzemanovic marked this pull request as draft June 24, 2024 15:04
@tzemanovic
Copy link
Member Author

tzemanovic commented Jun 24, 2024

The make check-crates was still failing, but is now fixed in b588224 - I had to re-introduce the download-params feature as the MASP VP must always be available even without it for the SDK to compile

So download-params wasn't enough on its own - I also had to bring back the validation feature too in 89eaa9f to prevent JS wasm bindgen leaking into our wasm build (I think this is due the combination of rand_core which pulls getrandom and masp_proof which enabled feature js in getrandom).

@tzemanovic
Copy link
Member Author

so the problem was indeed in masp_proofs - anoma/masp#85

tzemanovic added a commit that referenced this pull request Jun 25, 2024
@tzemanovic
Copy link
Member Author

I rm'd the "validation" feature again in https://github.com/anoma/namada/compare/89eaa9f610de6361c4bcd95c77f9dcb8a6d97537..2f3abec526148f599a567907d153fd7b9d2e45eb as it's not needed with the masp_proofs fix

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 1.85185% with 159 lines in your changes missing coverage. Please review.

Project coverage is 53.91%. Comparing base (879a326) to head (d6212b9).
Report is 4 commits behind head on main.

Files Patch % Lines
crates/shielded_token/src/validation.rs 0.00% 158 Missing ⚠️
crates/node/src/lib.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3419      +/-   ##
==========================================
- Coverage   53.92%   53.91%   -0.01%     
==========================================
  Files         317      318       +1     
  Lines      107575   107569       -6     
==========================================
- Hits        58011    58001      -10     
- Misses      49564    49568       +4     

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

@tzemanovic tzemanovic marked this pull request as ready for review June 25, 2024 12:26
brentstone added a commit that referenced this pull request Jun 28, 2024
* tomas/move-verify-shielded:
  changelog: add #3419
  shielded_token: feature guard validation to avoid compilation into wasm
  move masp validation from SDK into shielded_token crate
brentstone added a commit that referenced this pull request Jul 4, 2024
* origin/tomas/move-verify-shielded:
  changelog: add #3419
  shielded_token: feature guard validation to avoid compilation into wasm
  move masp validation from SDK into shielded_token crate
@brentstone brentstone merged commit 8975d21 into main Jul 5, 2024
16 of 19 checks passed
@brentstone brentstone deleted the tomas/move-verify-shielded branch July 5, 2024 21:16
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.

Move verify_shielded_tx out of the SDK
3 participants