[zk-token-sdk] Implement `FromStr` for `ElGamalPubkey`, `ElGamalCiphertext`, and `AeCiphertext` #130

samkim-crypto · 2024-03-07T08:43:07Z

Problem

Currently, ElGamalPubkey, ElGamalCiphertext and AeCiphertext types implement the Display trait, but not FromStr. It would be nice to implement FromStr for these types since clap-v3 can automatically validate and parse string encodings of any type that implements FromStr. Also, FromStr makes life much easier when exposing these types in wasm binaries.

Summary of Changes

Implement FromStr for types ElGamalPubkey, ElGamalCiphertext, and AeCiphertext.

Fixes #

joncinque

Looks good for the most part! mostly nits.

Since the implementations look pretty much the same in every case, how about creating a macro in the crate to derive FromStr for all of them? It looks like you just need the type, base64 length, and byte length

zk-token-sdk/src/zk_token_elgamal/pod/auth_encryption.rs

joncinque · 2024-03-12T11:43:55Z

zk-token-sdk/src/zk_token_elgamal/pod/auth_encryption.rs

+        if s.len() > AE_CIPHERTEXT_MAX_BASE64_LEN {
+            return Err(ParseError::WrongSize);
+        }
+        let ciphertext_vec = BASE64_STANDARD.decode(s).map_err(|_| ParseError::Invalid)?;


How about decoding directly into an array of [u8; AE_CIPHERTEXT_LEN] with decode_slice https://docs.rs/base64/latest/base64/engine/trait.Engine.html#method.decode_slice? Then you can check that the length is correct and return that directly, no allocations required!

This is a good idea, but one issue that I had is that something like this does not work

let mut array = [0u8; AE_CIPHERTEXT_LEN]; // 64 byte array let decoded_length = BASE64_STANDARD.decode_slice(s, &mut array).unwrap();

because it seems like for the base64 encoding, you need extra space for the additional zeroes during decoding. So decoded_length is still 64, but array must be at least of size 66 for this to not panic. This seems to make directly decoding into an array pointless since we would need to eventually allocate to an array length 64. What do you think?

Sorry, I think I'm missing something, do you have an example that shows this? I just tried this with a few examples of valid of 64-byte encoded strings, ie:

let s = "VGhlIHF1aWNrIGJyb3duIGZveCBqdW1wcyBvdmVyIDFUaGUgcXVpY2sgYnJvd24gZm94IGp1bXBzIG92ZXIgMQ=="; let mut array = [0u8; 64]; let decoded_length = BASE64_STANDARD.decode_slice(&s, &mut array).unwrap(); println!("decoded: {decoded_length}, array: {array:?}");

And it worked as expected with a decoded length of 64.

Hm, weird... I just copy pasted the exact lines into a temporary test and it seems to give the following output for me:

thread 'zk_token_elgamal::pod::elgamal::tests::temp_test' panicked at zk-token-sdk/src/zk_token_elgamal/pod/elgamal.rs:186:75: called `Result::unwrap()` on an `Err` value: OutputSliceTooSmall

If you add the following test

#[test] fn temp_test() { let s = "VGhlIHF1aWNrIGJyb3duIGZveCBqdW1wcyBvdmVyIDFUaGUgcXVpY2sgYnJvd24gZm94IGp1bXBzIG92ZXIgMQ=="; let mut array = [0u8; 64]; let decoded_length = BASE64_STANDARD.decode_slice(&s, &mut array).unwrap(); println!("decoded: {decoded_length}, array: {array:?}"); }

does it pass in your local machine? Both my local and dev server seems to produce OutputSliceTooSmall.

Oh! Looks like this was fixed in version 0.22, which is the version I was testing with

https://github.com/marshallpierce/rust-base64/blob/master/RELEASE-NOTES.md#0220 literally the exact situation we're discussing 😅

Ah makes sense! This was released two weeks ago. I guess then it would make sense to update the base64 crate version for the monorepo in a separate PR and then rebase this PR after that?

#225 on it!

joncinque · 2024-03-12T11:44:50Z

zk-token-sdk/src/zk_token_elgamal/pod/elgamal.rs

+        if s.len() > ELGAMAL_CIPHERTEXT_MAX_BASE64_LEN {
+            return Err(ParseError::WrongSize);
+        }
+        let ciphertext_vec = BASE64_STANDARD.decode(s).map_err(|_| ParseError::Invalid)?;


Same note here: decode into an array?

zk-token-sdk/src/zk_token_elgamal/pod/elgamal.rs

zk-token-sdk/src/zk_token_elgamal/pod/mod.rs

joncinque · 2024-03-13T12:49:09Z

zk-token-sdk/src/zk_token_elgamal/pod/mod.rs

@@ -82,3 +82,27 @@ impl TryFrom<PodProofType> for ProofType {
 #[derive(Clone, Copy, Pod, Zeroable, PartialEq, Eq)]
 #[repr(transparent)]
 pub struct CompressedRistretto(pub [u8; 32]);
+
+#[macro_export]


nit: since #[macro_export] makes this pub by default, how about exporting as only pub(crate) like here https://stackoverflow.com/a/67140319/16310679 ?

Oh yes, I completely forgot about this. I removed it.

codecov-commenter · 2024-03-14T01:43:46Z

Codecov Report

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

Project coverage is 81.9%. Comparing base (794cb2f) to head (532147a).
Report is 6 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master     #130    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         836      836            
  Lines      226643   226675    +32     
========================================
+ Hits       185636   185744   +108     
+ Misses      41007    40931    -76

joncinque

It's pretty much there! Just two last little nits, then this is all good for me

zk-token-sdk/src/zk_token_elgamal/pod/mod.rs

Co-authored-by: Jon C <[email protected]>

joncinque · 2024-03-14T13:26:41Z

zk-token-sdk/src/zk_token_elgamal/pod/mod.rs

+
+            fn from_str(s: &str) -> Result<Self, Self::Err> {
+                if s.len() > $base64_len {
+                    return Err(ParseError::WrongSize);


Sorry one last bit: it seems like these usages here still require implementations to import ParseError, could you fully qualify these with Self::Err or crate::zk_token_elgamal::pod::ParseError so the import isn't required?

Oh yes I should have caught this before. I removed the need for the dependency!

…rtext`, and `AeCiphertext` (anza-xyz#130) * add `ParseError` in `zk-token-elgamal` * implement `FromStr` for `ElGamalPubkey` and `ElGamalCiphertext` * implement `FromStr` for `AeCiphertext` * fix target * cargo fmt * use constants for byte length check * make `FromStr` functions available on chain * use macros for the `FromStr` implementations * restrict `from_str` macro to `pub(crate)` * decode directly into array * cargo fmt * Apply suggestions from code review Co-authored-by: Jon C <[email protected]> * remove unnecessary imports * remove the need for `ParseError` dependency --------- Co-authored-by: Jon C <[email protected]>

mergify bot added the need:merge-assist label Mar 7, 2024

samkim-crypto requested a review from joncinque March 12, 2024 08:59

joncinque reviewed Mar 12, 2024

View reviewed changes

joncinque reviewed Mar 13, 2024

View reviewed changes

samkim-crypto force-pushed the zk-token-sdk/fromstr branch from 3ecebdc to 1cd90bf Compare March 13, 2024 13:46

joncinque mentioned this pull request Mar 13, 2024

deps: Update base64 to 0.22 #225

Merged

samkim-crypto added 11 commits March 14, 2024 09:27

add ParseError in zk-token-elgamal

f6f2d70

implement FromStr for ElGamalPubkey and ElGamalCiphertext

6511cfa

implement FromStr for AeCiphertext

15b709d

fix target

0623dd1

cargo fmt

fb27ad1

use constants for byte length check

14771f4

make FromStr functions available on chain

a1e8be2

use macros for the FromStr implementations

366bc99

restrict from_str macro to pub(crate)

8e8a3fb

decode directly into array

be4985d

cargo fmt

9c13720

samkim-crypto force-pushed the zk-token-sdk/fromstr branch from 1cd90bf to 9c13720 Compare March 14, 2024 00:31

samkim-crypto requested a review from joncinque March 14, 2024 08:26

joncinque reviewed Mar 14, 2024

View reviewed changes

zk-token-sdk/src/zk_token_elgamal/pod/mod.rs Outdated Show resolved Hide resolved

zk-token-sdk/src/zk_token_elgamal/pod/mod.rs Outdated Show resolved Hide resolved

samkim-crypto and others added 2 commits March 14, 2024 21:30

Apply suggestions from code review

7fb252f

Co-authored-by: Jon C <[email protected]>

remove unnecessary imports

093b323

joncinque reviewed Mar 14, 2024

View reviewed changes

remove the need for ParseError dependency

532147a

joncinque approved these changes Mar 14, 2024

View reviewed changes

samkim-crypto merged commit fba70c8 into anza-xyz:master Mar 15, 2024
44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[zk-token-sdk] Implement `FromStr` for `ElGamalPubkey`, `ElGamalCiphertext`, and `AeCiphertext` #130

[zk-token-sdk] Implement `FromStr` for `ElGamalPubkey`, `ElGamalCiphertext`, and `AeCiphertext` #130

samkim-crypto commented Mar 7, 2024 •

edited

Loading

joncinque left a comment

joncinque Mar 12, 2024

samkim-crypto Mar 13, 2024

joncinque Mar 13, 2024

samkim-crypto Mar 13, 2024

joncinque Mar 13, 2024

joncinque Mar 13, 2024

samkim-crypto Mar 13, 2024

joncinque Mar 13, 2024

joncinque Mar 12, 2024

joncinque Mar 13, 2024

samkim-crypto Mar 13, 2024

codecov-commenter commented Mar 14, 2024 •

edited

Loading

joncinque left a comment

joncinque Mar 14, 2024

samkim-crypto Mar 14, 2024

[zk-token-sdk] Implement FromStr for ElGamalPubkey, ElGamalCiphertext, and AeCiphertext #130

[zk-token-sdk] Implement FromStr for ElGamalPubkey, ElGamalCiphertext, and AeCiphertext #130

Conversation

samkim-crypto commented Mar 7, 2024 • edited Loading

Problem

Summary of Changes

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 14, 2024 • edited Loading

Codecov Report

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[zk-token-sdk] Implement `FromStr` for `ElGamalPubkey`, `ElGamalCiphertext`, and `AeCiphertext` #130

[zk-token-sdk] Implement `FromStr` for `ElGamalPubkey`, `ElGamalCiphertext`, and `AeCiphertext` #130

samkim-crypto commented Mar 7, 2024 •

edited

Loading

codecov-commenter commented Mar 14, 2024 •

edited

Loading