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 the program module #1188

Merged
merged 7 commits into from
May 8, 2024

Conversation

samkim-crypto
Copy link

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

Problem

Each submodules from the zk-token-sdk are in the process of getting migrated to zk-sdk. The submodules related to the zk-token-proof program are yet to be moved over.

Summary of Changes

Added the program module that contains logic related to the original zk-token-proof program.

  • As specified in [zk-token-sdk] Proposal to use application agnostic names for instructions and modules #671, the zk-token-proof program is renamed zk-elgamal-proof program. I kept the original id for the program for now. I think we will need to have a follow-up dedicated to changing this id.
  • In the new zk-sdk, I organized all the logic related to the proof program inside a single program module instead of having it scattered throughout like in the zk-token-sdk.
  • None of the actual proof verification instructions or their instruction data are added yet. I will do so in a follow-up.

Fixes #

@codecov-commenter
Copy link

codecov-commenter commented May 5, 2024

Codecov Report

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

Project coverage is 82.1%. Comparing base (b0cfffb) to head (fa9ab7d).
Report is 34 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #1188     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         886      893      +7     
  Lines      236372   236414     +42     
=========================================
+ Hits       194204   194212      +8     
- Misses      42168    42202     +34     

@samkim-crypto samkim-crypto marked this pull request as ready for review May 7, 2024 01:59
@samkim-crypto samkim-crypto requested a review from joncinque May 7, 2024 02:06
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.

Since it's so nicely factored, I'm wondering if this should actually live in a separate crate under programs. but we can split that out later

Comment on lines 32 to 33
//! [`ZK ElGamal proof`]: https://docs.solanalabs.com/runtime/zk-token-proof
//! [`context-state`]: https://docs.solanalabs.com/runtime/zk-token-proof#context-data

Choose a reason for hiding this comment

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

I guess we'll have to also fixup the docs, but that can happen later

Copy link
Author

Choose a reason for hiding this comment

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

Yep, we'll definitely need to fix the docs!

pub mod state;

// Program Id of the ZK ElGamal Proof program
solana_program::declare_id!("ZkTokenProof1111111111111111111111111111111");

Choose a reason for hiding this comment

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

We may as well just declare the new id, no? We can do ZkE1Gama1Proof11111111111111111111111111111

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Comment on lines 40 to 44
#[derive(Error, Clone, Debug, Eq, PartialEq)]
pub enum ProofVerificationError {
#[error("Invalid proof context")]
ProofContext,
}

Choose a reason for hiding this comment

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

Should this rather be in the program module?

Copy link
Author

Choose a reason for hiding this comment

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

lHm yeah that is a good point. I will move it to the program module.

Choose a reason for hiding this comment

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

Is there a reason you decided on program instead of elgamal-program?

Copy link
Author

Choose a reason for hiding this comment

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

No particular reason. I thought we would just have a single program in the zk-sdk, but I guess it would make more sense to called it elgamal-program. I'll make the change.

@samkim-crypto samkim-crypto requested a review from joncinque May 8, 2024 04:45
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 a2af4ca into anza-xyz:master May 8, 2024
49 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