-
Notifications
You must be signed in to change notification settings - Fork 256
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
[zk-sdk] Add errors
, macros
, and encryption
modules
#1019
[zk-sdk] Add errors
, macros
, and encryption
modules
#1019
Conversation
9b06acd
to
574e673
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1019 +/- ##
=========================================
+ Coverage 81.9% 82.1% +0.1%
=========================================
Files 857 864 +7
Lines 232441 233160 +719
=========================================
+ Hits 190583 191432 +849
+ Misses 41858 41728 -130 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just the one question
I also realized that everything in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last little thing, which you can do separately, looks good otherwise!
@@ -10,6 +10,9 @@ | |||
//! - Basic type-wrapper around the AES-GCM-SIV symmetric authenticated encryption scheme | |||
//! implemented by [aes-gcm-siv](https://docs.rs/aes-gcm-siv/latest/aes_gcm_siv/) crate. | |||
|
|||
#[cfg(not(target_os = "solana"))] | |||
#[macro_use] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, looking through this again, I don't think you need #[macro_use]
either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh oops, yeah I guess this is only required by old rust. Thanks! I will removed on a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, actually it seems like if I removed this macro_use
then the macros seem to be not accessible to other crates. Reading a bit on the macro_use
attribute, it seems like this is required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, I thought the macros were only used internally. no worries then!
Summary of Changes
This is a follow-up to #988, which migrates the
errors
,macros
, andencryption
modules fromzk-token-sdk
intozk-sdk
. The only additional change to these modules is that the deprecated functions are now removed.Since the encryption module is largely the same in
zk-token-sdk
andzk-sdk
, we can consider removing and re-exporting the functions/types inzk-token-sdk
to the ones inzk-sdk
. Sincezk-token-sdk
will not get any updates and then eventually be removed, I thought I would just leave it as is. We can consider re-exporting in a follow-up.Fixes #