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

[zk-sdk] Add error types and zero ciphertext instruction #1453

Merged
merged 7 commits into from
May 23, 2024

Conversation

samkim-crypto
Copy link

@samkim-crypto samkim-crypto commented May 22, 2024

Problem

The proof data and instructions for the new elgamal program in zk-sdk are not added yet.

Summary of Changes

I added the zero-ciphertext instruction to start. Once this PR is merged after some feedback, then I think I can add the rest of the proof data in slightly larger chunk PRs.

  • 7c0e380: I added the ProofGenerationError and ProofVerificationError. These types remain largely the same as in zk-token-sdk and in particular, wraps RangeProofError and SigmaProofError.
  • d701214: I just copied the zero_balance instruction data module from zk-token-sdk verbatim to make the diffs in the next commit easier to view.
  • 8468880: I updated the zero_balance instruction data. The changes are essentially updating ZeroBalance... to ZeroCiphertext... and re-organizing the imports.
  • 25c3c88: Added the VerifyZeroCiphertext instruction in the elgamal proof program.

Fixes #

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

Attention: Patch coverage is 36.69065% with 88 lines in your changes are missing coverage. Please review.

Project coverage is 82.7%. Comparing base (9fc8d17) to head (32a6372).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #1453     +/-   ##
=========================================
- Coverage    82.7%    82.7%   -0.1%     
=========================================
  Files         871      872      +1     
  Lines      370333   370472    +139     
=========================================
+ Hits       306536   306614     +78     
- Misses      63797    63858     +61     

@samkim-crypto samkim-crypto requested a review from joncinque May 22, 2024 10:50
@samkim-crypto samkim-crypto marked this pull request as ready for review May 22, 2024 10:50
Copy link

@joncinque joncinque 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 overall, just the one comment about the error names

Comment on lines 13 to 16
#[error("not enough funds in account")]
NotEnoughFunds,
#[error("transfer fee calculation error")]
FeeCalculation,

Choose a reason for hiding this comment

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

Should these errors be changed or removed?

Copy link
Author

Choose a reason for hiding this comment

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

Oh great point! I removed it. Thanks!

Choose a reason for hiding this comment

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

Ok cool! Can you also get rid of NotEnoughFunds?

Copy link
Author

Choose a reason for hiding this comment

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

Oh oops, removed. Thanks!

Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great!

@samkim-crypto samkim-crypto merged commit 9035690 into anza-xyz:master May 23, 2024
41 checks passed
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.

3 participants