Skip to content

[zk-token-sdk] Refactor AuthenticatedEncryptionError and ElGamalError to errors module#589

Merged
samkim-crypto merged 3 commits intoanza-xyz:masterfrom
samkim-crypto:zk-token-sdk/re-organize-error-types
Apr 12, 2024
Merged

[zk-token-sdk] Refactor AuthenticatedEncryptionError and ElGamalError to errors module#589
samkim-crypto merged 3 commits intoanza-xyz:masterfrom
samkim-crypto:zk-token-sdk/re-organize-error-types

Conversation

@samkim-crypto
Copy link
Copy Markdown

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

Problem

Currently, error types AuthenticatedEncryptionError and ElGamalError lives in the encryption module, which is excluded from sbf target. However, it would be nice to use these types to handle errors in the zk_token_elgamal module as well (e.g. #588), which should be included for sbf target.

Summary of Changes

Refactor these error types into the errors module so that both encryption and zk_token_elgamal submodules can access it.

Fixes #

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

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

Project coverage is 81.8%. Comparing base (dcc195e) to head (5807079).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #589     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         850      851      +1     
  Lines      229369   229481    +112     
=========================================
+ Hits       187717   187751     +34     
- Misses      41652    41730     +78     

@samkim-crypto samkim-crypto marked this pull request as ready for review April 11, 2024 07:19
@samkim-crypto samkim-crypto requested a review from joncinque April 11, 2024 07:19
Copy link
Copy Markdown

@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 good to me!

Comment on lines +25 to +32
#[cfg(not(target_os = "solana"))]
use {
aes_gcm_siv::{
aead::{Aead, NewAead},
Aes128GcmSiv,
},
rand::{rngs::OsRng, Rng},
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: did cargo fmt move this down? it's up to you, but it can be more consistent to keep this block at the top, same with the other files

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hm, yeah it seems like cargo fmt always favors the larger chunk to be on top of the smaller chunk of imports. Alternatively, I can put an extra space between the two import chunks, but it seems like this is less clean. I'll leave it for now and maybe I can update it on a follow-up.

@samkim-crypto samkim-crypto merged commit b046c12 into anza-xyz:master Apr 12, 2024
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