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] Change FeeSigmaProof to PercentageWithCapProof #1084

Merged
merged 10 commits into from
Apr 30, 2024

Conversation

samkim-crypto
Copy link

@samkim-crypto samkim-crypto commented Apr 27, 2024

Problem

Given two Pedersen commitments, the FeeSigmaProof certifies that one encodes a percentage (with cap) of the number encodes in the other commitment. It does have applications to private transfers, but the name is unnecessarily specific to this application only. It makes sense to rename the type to something more general as proposed in #671.

Summary of Changes

Rename FeeSigmaProof to PercentageWithCapProof.

I made some changes to docs/comments, in the second commit e059946, but I ended up re-writing some of these in 6d72470 since the existing set of comments were a little confusing.

Another notable change is 72ac6ee, which flattens out the set of parameter variables to functions. I initially grouped parameters that were related to each other to make things neater, but I think it actually makes the syntax much more messy. I updated the parameters so that it is always [commitment] --> [opening] --> [amount] order. I wanted to make this change long time ago, but I did not want to break any downstream.

Fixes #

@samkim-crypto samkim-crypto force-pushed the zk-sdk-percentage-proof branch from 57c46a0 to 65ac74f Compare April 27, 2024 08:22
@samkim-crypto samkim-crypto force-pushed the zk-sdk-percentage-proof branch from 65ac74f to 5c14383 Compare April 27, 2024 08:26
@samkim-crypto samkim-crypto changed the title [zk-sdk] Change FeeSigmaProof to PercentageWithCap proof [zk-sdk] Change FeeSigmaProof to PercentageWithCapProof Apr 27, 2024
@samkim-crypto samkim-crypto force-pushed the zk-sdk-percentage-proof branch from 5c14383 to f2bc2f6 Compare April 30, 2024 02:10
@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 82.2%. Comparing base (e4ec48f) to head (de0db31).
Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #1084     +/-   ##
=========================================
  Coverage    82.2%    82.2%             
=========================================
  Files         868      880     +12     
  Lines      234388   235573   +1185     
=========================================
+ Hits       192668   193670   +1002     
- Misses      41720    41903    +183     

@samkim-crypto samkim-crypto marked this pull request as ready for review April 30, 2024 07:41
@samkim-crypto samkim-crypto requested a review from joncinque April 30, 2024 07:41
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! Confirming that the word "fee" no longer appears

//! The protocol guarantees computational soundness (by the hardness of discrete log) and perfect
//! zero-knowledge in the random oracle model.
//!
//! [`ZK Token proof program`]: https://docs.solanalabs.com/runtime/zk-token-proof

Choose a reason for hiding this comment

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

I guess you'll go back through in future work to rename zk-token-proof to zk-proof, correct?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, those have to be updated. Will do on a follow-up!

@samkim-crypto samkim-crypto merged commit 5dd8519 into anza-xyz:master Apr 30, 2024
38 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