Skip to content
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-643: add circuits&secrets to shielder_bindings crate #84

Merged
merged 5 commits into from
Jan 14, 2025

Conversation

kroist
Copy link
Collaborator

@kroist kroist commented Jan 13, 2025

This PR completes the code refactor of shielder-wasm bindings into new shielder_bindings crate. Particularly:

  • /circuits module with better error handling
  • secrets.rs module

In next PR, going to delete shielder-wasm crate, change shielder-sdk-crypto-wasm to use shielder_bindings and change CI to compile it.

@kroist kroist requested review from fbielejec and guspiel January 13, 2025 11:10
Copy link

github-actions bot commented Jan 13, 2025

Transaction NameMainCurrentDifference (%)
NewAccountNative20003112000275-0.00180%
DepositNative18279061827882-0.00131%
WithdrawNative18981021898126+0.00126%

@kroist kroist requested review from pmikolajczyk41 and removed request for guspiel January 14, 2025 13:01
Comment on lines 1 to 5
/// This script builds artifacts, which are later
/// embedded into the wasm binary.
/// To speedup the build process, we cache the artifacts after the first build.
///
/// When working locally, the `artifacts/` directory should be cleaned after the circuits are changed.
Copy link
Member

Choose a reason for hiding this comment

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

nit: //! instead of ///

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed

gen_deposit(&full_params);
generate_new_account(&full_params);
generate_withdraw(&full_params);
println!("cargo:rerun-if-changed=../shielder-circuits");
Copy link
Member

Choose a reason for hiding this comment

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

does it matter when we print? in particular, should we print this before heavy work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah it matters, moved to the top

}

impl From<halo2_proofs::plonk::Error> for VerificationError {
fn from(error: halo2_proofs::plonk::Error) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

you might want to ensure that the variant makes sense in this context to avoid confusing error propagation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure how to check the variant, there are plenty of them and they all are low-level

Comment on lines 36 to 75
impl WasmCircuit for Circuit<DepositProverKnowledge<F, RANGE_PROOF_CHUNK_SIZE>> {
fn load_files() -> (Params, ProvingKey, u32) {
let params = unmarshall_params(include_bytes!("../../artifacts/deposit/params.bin"))
.expect("Failed to unmarshall params");

let (k, pk) = unmarshall_pk::<
<DepositProverKnowledge<F, RANGE_PROOF_CHUNK_SIZE> as ProverKnowledge<F>>::Circuit,
>(include_bytes!("../../artifacts/deposit/pk.bin"))
.expect("Failed to unmarshall pk");

(params, pk, k)
}
}

impl WasmCircuit for Circuit<NewAccountProverKnowledge<F>> {
fn load_files() -> (Params, ProvingKey, u32) {
let params = unmarshall_params(include_bytes!("../../artifacts/new_account/params.bin"))
.expect("Failed to unmarshall params");
let (k, pk) =
unmarshall_pk::<<NewAccountProverKnowledge<F> as ProverKnowledge<F>>::Circuit>(
include_bytes!("../../artifacts/new_account/pk.bin"),
)
.expect("Failed to unmarshall pk");

(params, pk, k)
}
}

impl WasmCircuit for Circuit<WithdrawProverKnowledge<F, RANGE_PROOF_CHUNK_SIZE>> {
fn load_files() -> (Params, ProvingKey, u32) {
let params = unmarshall_params(include_bytes!("../../artifacts/withdraw/params.bin"))
.expect("Failed to unmarshall params");
let (k, pk) = unmarshall_pk::<
<WithdrawProverKnowledge<F, RANGE_PROOF_CHUNK_SIZE> as ProverKnowledge<F>>::Circuit,
>(include_bytes!("../../artifacts/withdraw/pk.bin"))
.expect("Failed to unmarshall pk");

(params, pk, k)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I strongly suggest unifying these functions with a single helper

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added macro for that

@kroist kroist merged commit 956d092 into main Jan 14, 2025
9 checks passed
@kroist kroist deleted the complete-bindings-crate branch January 14, 2025 21:19
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