Conversation
WalkthroughReplaces a removed Rust app with a new Noir-based proof circuit under apps/proof_circuit, adds associated scripts and README, updates root package.json with JS deps for hashing, and updates .gitignore. The new circuit verifies ECDSA recoveries and a Poseidon2-based nullifier derived from a selected signature. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Bash as scripts/generate-inputs.sh
participant TS as scripts/hash.ts (Poseidon2)
participant Noir as apps/proof_circuit (Noir)
User->>Bash: Run generate-inputs.sh
Bash->>Bash: Read scripts/inputs.txt
Bash->>TS: hash(sigSelected) via CLI
TS->>TS: reduceRS(rs) mod P
TS-->>Bash: Poseidon2Hash([Fr(rsReduced)])
Bash->>Bash: Build Prover.toml (hex->dec arrays, flags)
Bash-->>User: Prover.toml written
User->>Noir: nargo execute (Prover.toml)
Noir->>Noir: Poseidon2([sig_field]) == nullifier_hash
Noir->>Noir: split_uncompressed_pub_key(...)
Noir->>Noir: ecrecover(ad_creator) == ad_creator
Noir->>Noir: ecrecover(bridger) == bridger
Noir-->>User: witness / proof artifacts
sequenceDiagram
autonumber
participant Caller as Public Inputs
participant Circuit as main.nr
participant Utils as utils.nr
participant EC as dep::ecrecover
participant Hash as poseidon2
Caller->>Circuit: nullifier_hash, ad_creator, bridger, msg_hash, ad_contract,<br/>ad_creator_pub_key, ad_creator_sig, bridger_pub_key, bridger_sig
Circuit->>Circuit: Select sig_field by ad_contract
Circuit->>Hash: Poseidon2([sig_field])
Hash-->>Circuit: expected_nullifier_hash
Circuit->>Circuit: assert expected_nullifier_hash == nullifier_hash
Circuit->>Utils: split_uncompressed_pub_key(ad_creator_pub_key)
Utils-->>Circuit: (x1, y1)
Circuit->>EC: ecrecover(x1, y1, ad_creator_sig, msg_hash)
EC-->>Circuit: expected_ad_creator
Circuit->>Circuit: assert ad_creator == expected_ad_creator
Circuit->>Utils: split_uncompressed_pub_key(bridger_pub_key)
Utils-->>Circuit: (x2, y2)
Circuit->>EC: ecrecover(x2, y2, bridger_sig, msg_hash)
EC-->>Circuit: expected_bridger
Circuit->>Circuit: assert bridger == expected_bridger
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
.gitignore (1)
3-3: Ignore all build artifacts under target (keep Verifier in a proper source dir instead).Only ignoring
/target/vkstill commits large compiled blobs. Prefer ignoring the whole app target dir and track Verifier.sol in a stablecontracts/path if you need it versioned.node_modules - -/target/vk +apps/proof_circuit/target/ +# If you want to keep a placeholder: +!apps/proof_circuit/target/.gitkeepapps/proof_circuit/Nargo.toml (1)
4-4: Fill or remove empty authors.Empty author metadata is noise. Either drop the field or populate it.
[package] name = "proof_circuit" type = "bin" -authors = [""] +# authors = ["Your Name <you@example.com>"]apps/proof_circuit/target/proof_circuit.json (1)
1-1: Don’t commit heavy compiled artifacts and local absolute paths.This JSON includes debug_symbols and absolute local paths (e.g.,
/home/blockyj/...) which add bloat and leak environment info. Consider excludingapps/proof_circuit/target/*.jsonfrom VCS; regenerate during CI instead.I can add a minimal build step to CI that compiles the circuit and uploads artifacts if needed.
apps/proof_circuit/target/Verifier.sol (3)
770-786: Pairing precompile comments are misleading; verify G2 limb ordering.The constants here are G2 Fp2 coordinates, but comments say “Fixed G1 point” / “G1 point from VK”. Please correct comments and double-check the Fp2 limb order (bn128 pairing expects [x_im, x_re, y_im, y_re]).
If these constants came from the barretenberg generator, link or paste the source to confirm limb order.
3-3: Two pragma directives — keep one.Having both
>=0.8.21and^0.8.27is redundant; the intersection is^0.8.27. Keep a single pragma at the top.-pragma solidity >=0.8.21; +pragma solidity ^0.8.27; ... -pragma solidity ^0.8.27;Also applies to: 127-127
1633-1635: Gas: batch-invert denominators.You already note “batch invert later?” — doing a Montgomery batch inversion here saves many modexp calls.
I can wire a small batch inversion helper on Fr using the modexp precompile to reduce calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks)apps/proof-circuit/Cargo.toml(0 hunks)apps/proof-circuit/package.json(0 hunks)apps/proof-circuit/src/main.rs(0 hunks)apps/proof_circuit/Nargo.toml(1 hunks)apps/proof_circuit/src/main.nr(1 hunks)apps/proof_circuit/target/Verifier.sol(1 hunks)apps/proof_circuit/target/proof_circuit.json(1 hunks)
💤 Files with no reviewable changes (3)
- apps/proof-circuit/package.json
- apps/proof-circuit/src/main.rs
- apps/proof-circuit/Cargo.toml
🔇 Additional comments (2)
apps/proof_circuit/src/main.nr (1)
3-22: LGTM: concise dual-sig verification against public addresses and msg_hash.Inputs and assertions line up with dep::ecrecover’s semantics (verify with provided pubkey, derive ETH address).
Please confirm the
msg_hashyou pass in is the exact 32-byte digest expected bystd::ecdsa_secp256k1::verify_signature(no Ethereum signed message prefix applied unless your off-chain signer used it). If you want, I can add a helper that hashes raw bytes inside the circuit to avoid mismatch.apps/proof_circuit/target/Verifier.sol (1)
1567-1598: Add CI checks for PROOF_SIZE and publicInputsSize
PROOF_SIZE is hard-coded to 456 and vk.publicInputsSize to 50 in Verifier.sol—add CI assertions to ensure these match your ZK circuit JSON (e.g. proof_circuit.json). For example:# verify proof element count jq '.proof | length' apps/proof_circuit/target/proof_circuit.json | grep -qx '456' # verify public-inputs count jq '.public_inputs | length' apps/proof_circuit/target/proof_circuit.json | grep -qx '50'
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
apps/proof_circuit/src/utils.nr (1)
1-11: Assert uncompressed pubkey prefix to guard misuse.Add a simple constraint so invalid keys fail early instead of propagating bad coordinates.
pub fn split_uncompressed_pub_key(pub_key: [u8; 65]) -> ([u8; 32], [u8; 32]) { let mut pub_key_x: [u8; 32] = [0; 32]; let mut pub_key_y: [u8; 32] = [0; 32]; + // Expect uncompressed secp256k1 pubkey (0x04 || X[32] || Y[32]) + assert(pub_key[0] == 4); + for i in 0..32 { pub_key_x[i] = pub_key[i + 1]; pub_key_y[i] = pub_key[i + 32 + 1]; } (pub_key_x, pub_key_y) }package.json (1)
12-19: Ensure tooling can run TypeScript scripts and versions are compatible.
- You added a TS script (hash.ts) but no runner. Add
tsx(orts-node) sogenerate-inputs.shcan execute it reliably.- Verify
@aztec/bb.js@^0.87.0is compatible with@noir-lang/noir_js@1.0.0-beta.9for Poseidon2 APIs.Proposed additions:
"packageManager": "pnpm@10.10.0", "dependencies": { "@aztec/bb.js": "^0.87.0", "@noir-lang/noir_js": "1.0.0-beta.9" }, "devDependencies": { - "@types/node": "^24.3.1" + "@types/node": "^24.3.1", + "tsx": "^4.16.0", + "typescript": "^5.5.0" }Optionally expose a script:
"scripts": { - "test": "echo \"Error: no test specified\" && exit 1" + "test": "echo \"Error: no test specified\" && exit 1", + "hash:poseidon": "tsx apps/proof_circuit/scripts/hash.ts" }apps/proof_circuit/scripts/hash.ts (1)
21-31: Minor: avoid abrupt process.exit in library-ish code.Since this file both exports a function and runs as a CLI, prefer setting
process.exitCodeand returning to keep it friendlier for programmatic use. Not blocking.- process.stdout.write(result); - process.exit(0); + process.stdout.write(result); + process.exitCode = 0; ... - process.exit(1); + process.exitCode = 1;apps/proof_circuit/scripts/generate-inputs.sh (2)
7-12: Harden key parsing and avoid multiple matches.Quote the grep pattern, allow leading spaces, and take the first match to avoid unexpected multiple-line captures.
Apply this diff:
-extract_value() { +extract_value() { local key=$1 - # Extract the part inside quotes after the equals sign - grep "^$key\s*=" "$input_file" | sed -E 's/^[^=]+= *"(.*)"/\1/' + # Extract the part inside quotes after the equals sign (first match only) + grep -E "^[[:space:]]*${key}[[:space:]]*=" "$input_file" | head -n1 | sed -E 's/^[^=]+= *"(.*)".*$/\1/' }
70-76: Outdated comment and missing normalization note.Comment mentions “expected_address” (not present). Update to reflect actual vars and that signatures are normalized before hashing.
Apply this diff:
-# Strip 0x from everything except expected_address +# Strip 0x from hex blob inputs (addresses remain as-is; converted to Field later)apps/proof_circuit/src/main.nr (1)
17-26: Ensure the nullifier hashing matches preimage semantics.You reduce 64 bytes to a single Field via
Field::from_be_bytes(...)then hash one element. If the goal is “hash the exact 64 signature bytes,” consider hashing two field limbs (first 32, next 32) to avoid reduction ambiguity. Also confirmfrom_be_bytesaccepts 64 bytes in this Noir version.Example refactor (conceptual):
let (a, b) = if ad_contract { (Field::from_be_bytes(bridger_sig[0..32]), Field::from_be_bytes(bridger_sig[32..64])) } else { (Field::from_be_bytes(ad_creator_sig[0..32]), Field::from_be_bytes(ad_creator_sig[32..64])) }; let expected_nullifier_hash = poseidon2::Poseidon2::hash([a, b], 2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.gitignore(1 hunks)apps/proof_circuit/Nargo.toml(1 hunks)apps/proof_circuit/README.md(1 hunks)apps/proof_circuit/scripts/generate-inputs.sh(1 hunks)apps/proof_circuit/scripts/hash.ts(1 hunks)apps/proof_circuit/scripts/inputs.txt(1 hunks)apps/proof_circuit/src/main.nr(1 hunks)apps/proof_circuit/src/utils.nr(1 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/proof_circuit/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/proof_circuit/Nargo.toml
- .gitignore
🧰 Additional context used
🪛 Shellcheck (0.10.0)
apps/proof_circuit/scripts/generate-inputs.sh
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
[warning] 27-27: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (4)
apps/proof_circuit/scripts/inputs.txt (1)
7-7: Confirm boolean parsing ofad_contract.If
generate-inputs.shsources this file in bash, spaces around=and quoting booleans will not parse. If it’s parsed manually, you’re fine. Please confirm.If bash-sourced, prefer:
-ad_contract = "false" +ad_contract=falseapps/proof_circuit/scripts/hash.ts (1)
9-10: No changes needed for hex formatting
Fr.fromString accepts both 0x-prefixed hex and decimal strings, and nullifierHash.toString() returns a 0x-prefixed hex string, which aligns with Noir’s expected input format.apps/proof_circuit/scripts/generate-inputs.sh (1)
78-80: Confirm signature format assumptions.We assume 65-byte ECDSA (r||s||v) with v last and 1 byte. If EIP-2098 (64-byte compact) appears, this truncation breaks inputs. Verify inputs.txt format or gate with a length check.
Do you want me to add a length guard that errors unless signatures are 130 hex chars (after 0x), and optionally support EIP-2098?
apps/proof_circuit/src/main.nr (1)
28-37: Confirm ecrecover API expectations (no v parameter).You pass (pubkey_x, pubkey_y, sigRS, msg_hash). If the library expects (sigRSV, msg_hash) to recover pubkey, this will mis-verify. Double-check dep version/API.
Would you like me to look up the exact ecrecover Noir crate API you’re pinning and adjust accordingly?
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores