-
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 range_proof
module
#1091
[zk-sdk] Add range_proof
module
#1091
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1091 +/- ##
========================================
Coverage 82.1% 82.1%
========================================
Files 881 886 +5
Lines 235742 236383 +641
========================================
+ Hits 193677 194244 +567
- Misses 42065 42139 +74 |
a74de22
to
dec8702
Compare
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.
Just a couple of comments, but the rest looks great!
zk-sdk/src/range_proof/generators.rs
Outdated
#[cfg(not(target_os = "solana"))] | ||
const MAX_GENERATOR_LENGTH: usize = u32::MAX as usize; |
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.
Is this meant to have the #[cfg(...)]
here? It's used later in code that's not gated with the same cfg
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 yes, the entire generators module is included in #[cfg(...)]`, so this is actually not needed. It seems like this is a relic of the past before some of the refactoring was done... I removed it. Thanks!
zk-sdk/src/range_proof/generators.rs
Outdated
|
||
#[allow(non_snake_case)] | ||
#[derive(Clone)] | ||
pub struct BulletproofGens { |
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.
I thought Bulletproof
would get changed to RangeProof
, is that not the case?
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 wait, this is the bulletproofs generators. Yes, those should be changed 🙏
43abd8d
to
b032a38
Compare
Summary of Changes
This is a follow-up to #1065, which migrates the
range_proof
module from thezk-token-sdk
intozk-sdk
.Everything from the range proof module seemed to be fine and so it is copied verbatim, with the exception of
allow(dead_code)
to prevent ci from complaining, and a minor update to comments.