-
Notifications
You must be signed in to change notification settings - Fork 7
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
refactor(RLN:) Remove dependencies and add new APIs #45
Conversation
Allows passing the wasm, zkey and verification key data as buffers, instead of using a path to a folder
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.
Can be rebased on master
(Strictly speaking new_with_params seems like a new feature, but we can have it as part of this commit/PR np)
@oskarth Done! You can have a last glance and eventually merge it
Noted! |
ark-circom = { git = "https://github.com/gakonst/ark-circom", rev = "06eb075", features = ["circom-2"] } | ||
#ark-circom = { git = "https://github.com/vacp2p/ark-circom", branch = "no-ethers-core", features = ["circom-2"] } |
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.
@oskarth The vacp2p fork is a subset of the original ark-circom, which is lighter in terms of dependencies while guaranteeing zerokit the functionalities it needs. I was unsure which dep activate by default (I have pros and cons for both), so I kept the original one. But feel free to change it (the difference is ~60 dependencies less compiled)
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.
Yeah original one is better unless our fork is a must. We should upstream change we make, or at least start discussion.
I feel like there should be some form of dead code compilation that can be done here? So it doesn't matter how many dependencies are in ark-circom for zerokit binary size.
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.
You can get a warning for unused functions/code (dead_code
). To detect unused dependencies you need special tools like udeps, so I suspect the compiler doesn't figure it out automatically which one are unused and skips their compilation. But I might be wrong, I tried to search something like what you said but with no luck.
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.
What matters here is the final compilation size, not if it is part of compilation process (CI isn't that slow). I could be wrong, but I think this gets remove with e.g. https://doc.rust-lang.org/rustc/codegen-options/index.html#link-dead-code
For wasm there are tools like https://rustwasm.github.io/docs/book/reference/code-size.html#the-twiggy-code-size-profiler
Can continue this discussion in relevant issue though
ark-circom = { git = "https://github.com/gakonst/ark-circom", features = ["circom-2"] } | ||
wasmer = { version = "2.0" } | ||
ark-circom = { git = "https://github.com/gakonst/ark-circom", rev = "06eb075", features = ["circom-2"] } | ||
#ark-circom = { git = "https://github.com/vacp2p/ark-circom", branch = "no-ethers-core", features = ["circom-2"] } |
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 think we might want ethers in future anyway, so we can deal with ZK smart contracts etc from Zerokit too
This PR:
fullmerkletree
in order to easily select this MT implementation;new_with_params
that allows to create an RLN object by passing the witness generator, the proving and verification key serialized as raw byte data;This PR is currently based on the branch
poseidon-integration
(currently under review) and will be automatically rebased onmaster
once the first is merged.