Swap-out solana-zk-sdk for solana-zk-elgamal-proof-interface and solana-zk-sdk-pod#1090
Conversation
351682b to
87f0130
Compare
87f0130 to
e814132
Compare
grod220
left a comment
There was a problem hiding this comment.
Thanks for this work! Unraveling dependencies can be tricky. Just have some cleanup comments.
| solana-zk-sdk = "6.0.0" | ||
| solana-zk-sdk-pod = "0.1.1" |
There was a problem hiding this comment.
Can you put these in alphabetical order?
|
|
||
| [dev-dependencies] | ||
| solana-nonce = "3.2.0" | ||
| solana-nonce = "3.1.0" |
There was a problem hiding this comment.
I actually can't get this to work due to wincode.
solana-noncev3.2 requiressolana-hashv4.3, which requireswincode` v0.5.3- agave crates like
solana-ledgerv4.0.0 requireswincodev0.4
This causes dependency conflict in the token-cli.
The crate solana-nonce v3.1 requires solana-hash v4.2, which requires wincode v0.4, so I can make this work.
| // solana_zk_sdk::{ | ||
| // encryption::{ | ||
| // auth_encryption::AeKey, | ||
| // elgamal::{ElGamalCiphertext, ElGamalKeypair, ElGamalPubkey, ElGamalSecretKey}, | ||
| // pod::{ | ||
| // auth_encryption::PodAeCiphertext, | ||
| // elgamal::{PodElGamalCiphertext, PodElGamalPubkey}, | ||
| // }, | ||
| // }, | ||
| // zk_elgamal_proof_program::{ | ||
| // self, | ||
| // instruction::{close_context_state, ContextStateInfo}, | ||
| // proof_data::*, | ||
| // state::ProofContextState, | ||
| // }, | ||
| // }, |
There was a problem hiding this comment.
Sorry, this should have removed prior to asking for review 🙏
| solana-address = "2.6.0" | ||
| solana-banks-client = { version = "3.0.0", optional = true } | ||
| solana-banks-interface = { version = "3.0.0", optional = true } | ||
| solana-address = "2.5.0" |
There was a problem hiding this comment.
I downgrade solana-address for the same reason as #1090 (comment):
solana-addressv2.6.0 requires wincode v0.5solana-addressv2.5.0 requires wincode v0.4
But unlike with solana-hash, it doesn't create conflicts in the token-cli (though we do get two versions of wincode in Cargo.lock), so I think we can keep v2.6.0 here.
Fixed in 91b1fa3.
| solana-cli-output = { version = "3.1.0", features = ["agave-unstable-api"], optional = true } | ||
| solana-hash = "4.3.0" | ||
| solana-cli-output = { version = "4.0.0-beta.7", features = ["agave-unstable-api"], optional = true } | ||
| solana-hash = "4.2.0" |
There was a problem hiding this comment.
Maybe some of these version regressions are rebase artifacts? Perhaps you can do another pass to catch these in the rest of the PR.
There was a problem hiding this comment.
I couldn't get this to work with solana-hash v4.3 due to the same reason as #1090 (comment).
| [package] | ||
| name = "spl-token-confidential-transfer-proof-extraction" | ||
| version = "0.5.1" | ||
| version = "0.6.0" |
There was a problem hiding this comment.
We don't bump these ourselves right? Think the publish github action is responsible for that. See a few examples in this PR of this.
There was a problem hiding this comment.
I did this to actually resolve version conflicts. Some of the agave crates (e.g. solana-account-decoder) depend on spl-token-2022-interface v2.1.0, which depends on the spl-token-confidential-transfer-proof-extraction crate v0.5.1.
So if I leave the local crate version to v0.5.1, then cargo will try to be efficient and just use my local spl-token-confidential-transfer-proof-extraction version, which is not compatible with solana-account-decoder.
If I bump the local crate version to v0.6.0, then cargo has no choice and will maintain two versions of ``spl-token-confidential-transfer-proof-extractionin theCargo.lock` file, the local version with v0.6.0 and the crate.io v5.1, so things will compile.
I think the publish script will not bump Cargo.toml if it is already set to be correctly (or we can also manually specify the version in the CI publish UI), so I think this should be fine, though let me know if I am missing something.
| // test_transfer_with_fee_proof_validity(65535, 65535, 5, 10); | ||
| // test_transfer_with_fee_proof_validity(65535, 65535, 5, 1); | ||
|
|
||
| test_transfer_with_fee_proof_validity(65536, 65536, 5, 10); | ||
| test_transfer_with_fee_proof_validity(65536, 65536, 5, 1); | ||
|
|
||
| test_transfer_with_fee_proof_validity(281474976710655, 281474976710655, 5, 10); // 2^48 - 1 | ||
| test_transfer_with_fee_proof_validity(281474976710655, 281474976710655, 5, 1); | ||
| // test_transfer_with_fee_proof_validity(65536, 65536, 5, 10); | ||
| // test_transfer_with_fee_proof_validity(65536, 65536, 5, 1); | ||
| // | ||
| // test_transfer_with_fee_proof_validity(281474976710655, 281474976710655, 5, 10); // 2^48 - 1 | ||
| // test_transfer_with_fee_proof_validity(281474976710655, 281474976710655, 5, 1); |
There was a problem hiding this comment.
Yeah sorry, I forgot about addressing this before asking for review 🙏 . This requires a small in on the zk-sdk side, so I will address this once solana-program/zk-elgamal-proof#377 is merged.
joncinque
left a comment
There was a problem hiding this comment.
Looks great overall! I agree with all of Gabe's comments
4d3bf65 to
91b1fa3
Compare
…-proof-interface`
cbb7d9a to
e897b95
Compare
|
Thanks for the patience. This PR should be ready for another set of reviews! 🙏 |
grod220
left a comment
There was a problem hiding this comment.
Thanks for those changes! Definitely some tricky dep combinations. Let's get Jon's eyes on this too though.
joncinque
left a comment
There was a problem hiding this comment.
Looks good to me! We'll just need to be careful when doing the publishes on the confidential-transfer crates
Summary of Changes
487e83c: I swapped out
solana-zk-sdkforsolana-zk-elgamal-proof-interfaceandsolana-zk-sdk-podin token-2022interfaceandprogram. Thesolana-zk-sdkis needed in some places like proof-generation. For these places, I bumped the version to v6.0.0. This commit should be pretty straightforward and mechanical, just re-organizing the imports. The exception is replacing theOptionalNonZeroElGamalPubkeywithMaybeNull<PodElGamalPubkey>.0fb0bed: I bumped agave crates like
solana-test-validatorandsolana-program-testto v4-beta while downgrading some of the solana-sdk dependencies to resolve some dependency conflicts. I divided this commit in smaller commits in #1106.e814132: in 487e83c, I made a mistake in comparing
MaybeNullElGamalPubkeys, so I fixed it here.76e9728: Addressed #1116 (comment).