[token-2022] Upgrade to `zk-sdk` #7148

samkim-crypto · 2024-08-14T10:06:48Z

Problem

The zk-token-sdk has been deprecated and replaced with zk-sdk in the monorepo, but the token22 program still depends on zk-token-sdk.

Summary of Changes

Updated token22 program to zk-sdk.

1ec4dc8: I first updated the spl-pod crate to zk-sdk. Unfortunately, for the ElGamal Pod types in zk-sdk, there is no proper constructor functions to instantiate them from bytes (will be added in the next major release!). However, these types do implement the FromStr trait, so I used them to initialize these Pod types as a workaround.

23f21dc: Upgraded to use zk-sdk. The changes are mostly just naming substitutions in this PR.

The Pod prefix has been added to pod types (e.g. ElGamalPubkey --> PodElGamalPubkey)
PubkeyValidityData --> PubkeyValidityProofData
ZeroBalanceProofData --> ZeroCiphertextProofData
FeeSigmaProofData --> PercentageWithCapProofData

14669ed: Added dependencies to the spl-token-confidential-transfer-* crates:

The proof generation, verification, and ciphertext arithmetic logic have been refactored into their separate crates, so I added dependencies to these crates.
The spli_proof_generation and ciphertext_extraction crates are now obsolete, so I removed them in confidential_transfer/mod.rs (I forgot to delete the actual files in this commit so I delete them in a later commit). I think it makes sense to refactor out account_info into a separate crate (and removed dependency on spl-token-confidential-transfer-proof-generation, but I will do it in a follow-up PR.
The refactored crates have their own error types TokenProofGenerationError and TokenProofExtractionError. I wasn't sure what is the cleanest way to handle these errors, but I ended up instantiating the From trait to convert them into TokenError. Please let me know if you have other suggestions 🙏 .

9fb3f74: The WithdrawProofData is removed in zk-sdk to be replaced with the combination of equality and range proof. I updated the Withdraw instruction accordingly.

cf9eb6c: This is a follow-up to #7127 (comment).

0594847: Made the upgrade to zk-sdk in the token-client. These are mostly naming substitutions and also updating the withdraw instruction for the split proofs.

aad9f2e: I was about to add the create_equality_proof_context_state_for_withdraw and create_range_proof_context_state_for_withdraw functions and then realized that all these create proof context state functions can be unified into a single generic function just like how it is done for the record account functions. I ended up cleaning out the specific constructor functions for the context state accounts.

a0d12ea: Update the program-2022-tests. Should be mostly straightforward. There is a block of code that I meant to be remove, which I just commented out. I removed this block of code in a later commit 🙏 .

9cc5ede: I forgot to update the serialization logic to zk-sdk, so I made the changes. Again, unfortunately, there are no proper constructor functions for the Pod types, so I used the FromStr workaround. Regarding the decryption handles, they are not included in the instruction data any more, so I removed the related logic.

joncinque

Looks great overall! just tiny things

joncinque · 2024-08-15T13:31:58Z

libraries/pod/src/optional_keys.rs

+    // Unfortunately, the `solana-zk-sdk` does not exporse a constructor interface
+    // to construct `PodRistrettoPoint` from bytes. As a work-around, encode the
+    // bytes as base64 string and then convert the string to a
+    // `PodElGamalCiphertext`.
+    fn elgamal_pubkey_from_bytes(bytes: &[u8]) -> PodElGamalPubkey {
+        let string = BASE64_STANDARD.encode(bytes);
+        std::str::FromStr::from_str(&string).unwrap()
+    }


Can you add to the comment saying that this will be possible in the next version and what function to use? Is that 2.1?

Added! I will also create an issue regarding this very soon.

joncinque · 2024-08-15T16:05:33Z

token/program-2022/src/error.rs

+#[cfg(not(target_os = "solana"))]
+impl From<TokenProofGenerationError> for TokenError {
+    fn from(e: TokenProofGenerationError) -> Self {
+        match e {
+            TokenProofGenerationError::ProofGeneration(_) => TokenError::ProofGeneration,
+            TokenProofGenerationError::NotEnoughFunds => TokenError::InsufficientFunds,
+            TokenProofGenerationError::IllegalAmountBitLength => TokenError::IllegalBitLength,
+            TokenProofGenerationError::FeeCalculation => TokenError::FeeCalculation,
+        }
+    }
+}


For the errors, this is probably fine. You could do ProofGeneration(#[from] TokenProofGenerationError), like in https://github.com/anza-xyz/agave/blob/ecb44d7bd7bcfcbf1f342eb7eaf2728e7b6eefc5/rpc-client-api/src/client_error.rs#L14, but that might end up being more confusing in the end

Yeah makes sense. ProofGeneration(#[from] TokenProofGenerationError) seems more elegant to me, but this seems to need some restructuring of the existing TokenError unfortunately. It could also end up being more confusing as you suggested.

joncinque · 2024-08-15T16:28:31Z

token/client/src/token.rs

-        ciphertext_validity_proof_data: &BatchedGroupedCiphertext3HandlesValidityProofData,
-        ciphertext_validity_proof_signer: &S,
+        proof_data: &ZK,
+        split_instruction: bool,


nit: this parameter name is a little confusing -- can we call it split_account_creation_and_proof_verification? Or maybe you might have something better?

Agreed. I think split_account_creation_and_proof_verification is also the best I can come up with, so I'll use it. Thanks!

joncinque · 2024-08-15T16:33:07Z

token/program-2022-test/tests/confidential_transfer.rs

+// #[cfg(feature = "zk-ops")]
+// #[tokio::test]
+// async fn confidential_transfer_withdraw_with_record_account() {


did you also want to remove this?

Oh oops! Removed. Thanks!

…d_proof_verification`

joncinque

Looks great, thanks!

ryoqun · 2024-08-16T05:59:28Z

@samkim-crypto @joncinque cc: @yihau

seems merging this pr into spl repo caused relevant agave's master downstream ci job to fail: https://github.com/anza-xyz/agave/actions/runs/10415342439/job/28845769665#step:5:1467

#### Problem With solana-labs/solana-program-library#7148, spl-token-2022 has moved from using solana-zk-token-sdk to solana-zk-sdk, which is a major breaking change. Certain agave and anchor crates are depending on the re-export solana-zk-token-sdk in spl-token-2022, which is no longer present. This change is causing the downstream Anchor job to fail, since the patched version of spl-token-2022 is no longer compatible. #### Summary of changes Until new major versions of the SPL crates are available used by the Agave monorepo, disable the downstream anchor job.

joncinque · 2024-08-16T13:56:32Z

Thanks for pointing that out @ryoqun -- I've temporarily disabled the downstream job in anza-xyz/agave#2629

#### Problem With solana-labs/solana-program-library#7148, spl-token-2022 has moved from using solana-zk-token-sdk to solana-zk-sdk, which is a major breaking change. Certain agave and anchor crates are depending on the re-export solana-zk-token-sdk in spl-token-2022, which is no longer present. This change is causing the downstream Anchor job to fail, since the patched version of spl-token-2022 is no longer compatible. #### Summary of changes Until new major versions of the SPL crates are available used by the Agave monorepo, disable the downstream anchor job. (cherry picked from commit 3b9e7a3) # Conflicts: # .github/workflows/downstream-project-anchor.yml

* CI: Disable downstream anchor builds (#2629) #### Problem With solana-labs/solana-program-library#7148, spl-token-2022 has moved from using solana-zk-token-sdk to solana-zk-sdk, which is a major breaking change. Certain agave and anchor crates are depending on the re-export solana-zk-token-sdk in spl-token-2022, which is no longer present. This change is causing the downstream Anchor job to fail, since the patched version of spl-token-2022 is no longer compatible. #### Summary of changes Until new major versions of the SPL crates are available used by the Agave monorepo, disable the downstream anchor job. (cherry picked from commit 3b9e7a3) # Conflicts: # .github/workflows/downstream-project-anchor.yml * Fix merge conflicts --------- Co-authored-by: Jon C <[email protected]>

@samkim-crypto

#### Problem The publish step is great, but there's still some manual work to add the changelog and correct title for the created release. #### Summary of changes Use git-cliff to generate the release notes. This comes with a very simple cliff.toml which will do minimal processing, since we don't follow conventional commits in the repo. Here's a test run for the given input: ``` $ git-cliff -c ci/cliff.toml "pod-v0.3.0"..master --include-path "libraries/pod/**" --github-repo "solana-labs/solana-program-library" ``` Output: ``` ## What's new - Publish pod v0.3.2 by @github-actions[bot] - [token-2022] Upgrade to `zk-sdk` (solana-labs#7148) by @samkim-crypto - Implement `Default` for `PodOption` (solana-labs#7083) by @joncinque - Bump to 0.3.1 (solana-labs#7075) by @febo - Improve `PodOption` type (solana-labs#7076) by @febo - Add `PodU128` type (solana-labs#7012) by @febo - Add `PodOption` type (solana-labs#6886) by @febo - Use `bytemuck_derive` explicitly (solana-labs#6928) by @joncinque ```

@samkim-crypto

* CI: Add git-cliff step during publish #### Problem The publish step is great, but there's still some manual work to add the changelog and correct title for the created release. #### Summary of changes Use git-cliff to generate the release notes. This comes with a very simple cliff.toml which will do minimal processing, since we don't follow conventional commits in the repo. Here's a test run for the given input: ``` $ git-cliff -c ci/cliff.toml "pod-v0.3.0"..master --include-path "libraries/pod/**" --github-repo "solana-labs/solana-program-library" ``` Output: ``` ## What's new - Publish pod v0.3.2 by @github-actions[bot] - [token-2022] Upgrade to `zk-sdk` (#7148) by @samkim-crypto - Implement `Default` for `PodOption` (#7083) by @joncinque - Bump to 0.3.1 (#7075) by @febo - Improve `PodOption` type (#7076) by @febo - Add `PodU128` type (#7012) by @febo - Add `PodOption` type (#6886) by @febo - Use `bytemuck_derive` explicitly (#6928) by @joncinque ``` * Don't always run the step * Make the YAML valid * Trim each commit line to just the first line

#### Problem With solana-labs/solana-program-library#7148, spl-token-2022 has moved from using solana-zk-token-sdk to solana-zk-sdk, which is a major breaking change. Certain agave and anchor crates are depending on the re-export solana-zk-token-sdk in spl-token-2022, which is no longer present. This change is causing the downstream Anchor job to fail, since the patched version of spl-token-2022 is no longer compatible. #### Summary of changes Until new major versions of the SPL crates are available used by the Agave monorepo, disable the downstream anchor job.

samkim-crypto force-pushed the token-2022/zk-sdk branch 5 times, most recently from 70f0372 to 9cc5ede Compare August 15, 2024 01:46

samkim-crypto marked this pull request as ready for review August 15, 2024 02:13

samkim-crypto requested a review from joncinque August 15, 2024 04:35

samkim-crypto added 13 commits August 15, 2024 13:36

update spl-pod to use zk-sdk

92b05b7

update program-2022 to use zk-sdk

9daa22c

add dpendency to spl-token-confidential-transfer-* crates

639e016

update withdraw instruction

8566c56

fix instruction offset check in instruction constructor

0f42964

upgrade to zk-sdk in the client

082e33b

clean up context state constructor functions

0580f5b

update program-2022 tests

3d61b57

update token-cli

a502871

cargo fmt

bf7b09a

cargo clippy

cf0a473

remove unnecessary files

fa2c02f

update serialization

7285da5

samkim-crypto force-pushed the token-2022/zk-sdk branch from b09147f to 2dc870a Compare August 15, 2024 04:36

joncinque reviewed Aug 15, 2024

View reviewed changes

samkim-crypto added 3 commits August 16, 2024 09:55

remove unnecessary block of code

8c85c6d

rename split_instruction variable name to `spli_account_creation_an…

a7aeb97

…d_proof_verification`

add comment on the Pod ElGamal type constructors

27d98a8

samkim-crypto force-pushed the token-2022/zk-sdk branch from 2dc870a to 27d98a8 Compare August 16, 2024 01:05

joncinque approved these changes Aug 16, 2024

View reviewed changes

samkim-crypto merged commit 3766c63 into solana-labs:master Aug 16, 2024
37 checks passed

joncinque mentioned this pull request Aug 16, 2024

CI: Disable downstream anchor builds anza-xyz/agave#2629

Merged

mergify bot mentioned this pull request Aug 17, 2024

v2.0: CI: Disable downstream anchor builds (backport of #2629) anza-xyz/agave#2647

Merged

samkim-crypto mentioned this pull request Aug 19, 2024

[token-2022] Refactor tests to use ConfidentialTransferOption #7165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[token-2022] Upgrade to `zk-sdk` #7148

[token-2022] Upgrade to `zk-sdk` #7148

samkim-crypto commented Aug 14, 2024 •

edited

Loading

joncinque left a comment

joncinque Aug 15, 2024

samkim-crypto Aug 16, 2024 •

edited

Loading

joncinque Aug 15, 2024

samkim-crypto Aug 16, 2024

joncinque Aug 15, 2024

samkim-crypto Aug 16, 2024

joncinque Aug 15, 2024

samkim-crypto Aug 16, 2024

joncinque left a comment

ryoqun commented Aug 16, 2024

joncinque commented Aug 16, 2024

[token-2022] Upgrade to zk-sdk #7148

[token-2022] Upgrade to zk-sdk #7148

Conversation

samkim-crypto commented Aug 14, 2024 • edited Loading

Problem

Summary of Changes

joncinque left a comment

Choose a reason for hiding this comment

joncinque Aug 15, 2024

Choose a reason for hiding this comment

samkim-crypto Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

joncinque Aug 15, 2024

Choose a reason for hiding this comment

samkim-crypto Aug 16, 2024

Choose a reason for hiding this comment

joncinque Aug 15, 2024

Choose a reason for hiding this comment

samkim-crypto Aug 16, 2024

Choose a reason for hiding this comment

joncinque Aug 15, 2024

Choose a reason for hiding this comment

samkim-crypto Aug 16, 2024

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

ryoqun commented Aug 16, 2024

joncinque commented Aug 16, 2024

[token-2022] Upgrade to `zk-sdk` #7148

[token-2022] Upgrade to `zk-sdk` #7148

samkim-crypto commented Aug 14, 2024 •

edited

Loading

samkim-crypto Aug 16, 2024 •

edited

Loading