-
Notifications
You must be signed in to change notification settings - Fork 18
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
Support ark-bls12-381
#16
base: main
Are you sure you want to change the base?
Conversation
736852d
to
b9c684b
Compare
I'm so excited to see this and demo it! Great work so far! |
@piotr-roslaniec please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
This is great! Will review in greater detail soon. Please let us know if you need any specific inputs. Also, if we switch to arkworks for curves, do we still need dependency on halo2curves? |
Hi @srinathsetty, thank you for looking into this. At this point, I'm trying to fix a broken test which throws an In the meantime, here is a list of things that may benefit from your input:
On |
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.
Copilot reviewed 14 out of 29 changed files in this pull request and generated no comments.
Files not reviewed (15)
- benches/bench.rs: Evaluated as low risk
- src/provider/mod.rs: Evaluated as low risk
- src/bellpepper/solver.rs: Evaluated as low risk
- src/bellpepper/r1cs.rs: Evaluated as low risk
- src/bellpepper/shape_cs.rs: Evaluated as low risk
- src/bellpepper/test_shape_cs.rs: Evaluated as low risk
- src/provider/bn256_grumpkin.rs: Evaluated as low risk
- src/provider/pedersen.rs: Evaluated as low risk
- src/digest.rs: Evaluated as low risk
- src/provider/keccak.rs: Evaluated as low risk
- src/provider/secp_secq.rs: Evaluated as low risk
- src/bellpepper/mod.rs: Evaluated as low risk
- src/provider/ipa_pc.rs: Evaluated as low risk
- src/lib.rs: Evaluated as low risk
- examples/less_than.rs: Evaluated as low risk
Comments suppressed due to low confidence (4)
src/r1cs.rs:361
- The word 'accomodate' is misspelled. It should be 'accommodate'.
/// Renumbers variables to accomodate padded variables
src/r1cs.rs:435
- The word 'consitituent' is misspelled. It should be 'constituent'.
/// A method to create an instance object using consitituent elements
src/r1cs.rs:362
- The
pad
method should be reviewed to ensure it correctly handles the padding logic for R1CSShape.
pub fn pad(&self) -> Self {
src/provider/ark.rs:139
- Handle potential errors more gracefully when reading bytes from the XOF.
reader.read_exact(&mut uniform_bytes).expect("Failed to read bytes from XOF");
The bellpepper module we had turns bellpepper circuits into R1CS matrices. Doesn't arkworks provide similar functionality to extract R1CS matrices from arkworks circuit code?
I didn't follow the point about fork. When this PR is merged, we would be using arkworks for all curves. Can you clarify what you mean by fork? |
Exploratory integration with Arkworks crates as described in #15, and similar to previous https://github.com/arkworks-rs/spartan integration.
This is a pretty rough, initial draft that broke several things (traits, conventions,
crate::providers
, tests), to enable prototyping with Sonobe.I decided to make a draft PR to gain some feedback and pointers from maintainers, and eventually contribute if it is well-received and accepted.