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

Add zk_token_proof_program module to sdk #35339

Closed
wants to merge 1 commit into from

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Feb 28, 2024

Problem

The zk token program id is not accessible within the sdk yet but is needed for #34901

Summary of Changes

Adds a zk_token_proof_program module to sdk so that other core crates have easy access to its ID.

Fixes #

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.7%. Comparing base (da08868) to head (2e1ac4d).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #35339     +/-   ##
=========================================
- Coverage    81.7%    81.7%   -0.1%     
=========================================
  Files         834      835      +1     
  Lines      224244   224300     +56     
=========================================
- Hits       183398   183391      -7     
- Misses      40846    40909     +63     

Copy link
Contributor

@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.

In general, we should avoid putting more in the sdk and rely on the program crates to provide info about them.

I took a quick look at #34901, and it seems like it might be possible to move ReservedAccountKeys a bit higher up than sdk, maybe in solana-program-runtime? That way there's no need to redefine the id in solana-program, and you can simply import from solana-zk-token-sdk in solana-program-runtime.

For all the instances where you pass in ReservedAccountKeys::empty() to sanitize the transaction, you could keep the old constructor and use that directly, and instead create a new creator for SanitizedTransaction that always takes in the set of reserved keys.

What do you think?

@jstarry
Copy link
Member Author

jstarry commented Feb 29, 2024

In general, we should avoid putting more in the sdk and rely on the program crates to provide info about them.

I generally feel the same way but I think that since the zk token program is an enshrined / native program it's convenient to pull in descriptive details like program id and maybe ix interface without pulling in the zk token sdk and its crypto dependencies.

@joncinque
Copy link
Contributor

joncinque commented Feb 29, 2024

The proof program is enshrined at the level of the runtime, but not at the level of the SDK. Maybe I'm missing a use-case, but I can't think of a situation where someone using the SDK would want just the program id and nothing else.

The runtime might need it, which is why we have some redefined program ids like https://github.com/solana-labs/solana/blob/master/accounts-db/src/inline_spl_token.rs -- if we can't use the id defined in the zk token sdk crate, let's put the redefined program id in some higher place like these ids.

@jstarry
Copy link
Member Author

jstarry commented Feb 29, 2024

The proof program is enshrined at the level of the runtime, but not at the level of the SDK. Maybe I'm missing a use-case, but I can't think of a situation where someone using the SDK would want just the program id and nothing else.

Well there is one use-case for client side checking for erroneous write locks

@jstarry
Copy link
Member Author

jstarry commented Feb 29, 2024

if we can't use the id defined in the zk token sdk crate, let's put the redefined program id in some higher place like these ids.

That's fine with me!

@jstarry jstarry closed this Feb 29, 2024
@jstarry jstarry deleted the refactor/zk-module branch February 29, 2024 23:49
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.

2 participants