Open
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds support for a new STWO (stow) ZK proof engine alongside existing GNARK and SNARKJS engines. Changes include proto enum extension, TypeScript code generation updates, a new browser-based STWO operator implementation with WASM initialization and witness serialization, build system configuration updates, and integration across client and utility code to enable engine selection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/zk.ts (1)
689-697:⚠️ Potential issue | 🔴 CriticalAdd OPRF operator support for STWO engine.
STWO is selectable as a ZK engine (e.g.,
src/scripts/generate-receipt.ts), butOPRF_OPERATOR_MAKERSonly includes a maker for 'gnark'. If STWO is selected as thezkEngineand TOPRF operations are required,makeDefaultOPRFOperatorwill throw"No OPRF operator maker for stwo"at runtime.Either add
makeStwoOPRFOperatortoOPRF_OPERATOR_MAKERS, or prevent STWO from being selected when OPRF is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/zk.ts` around lines 689 - 697, OPRF_OPERATOR_MAKERS currently only maps 'gnark' and lacks a maker for the 'stwo' ZK engine, causing makeDefaultOPRFOperator to throw when zkEngine == 'stwo'; fix by adding the STWO OPRF maker to the mapping (add an entry 'stwo': makeStwoOPRFOperator) or, if STWO cannot support OPRF, add a guard where makeDefaultOPRFOperator is invoked to prevent selecting 'stwo' when OPRF is required and surface a clear error; update the OPRF_OPERATOR_MAKERS constant or the selection logic referencing OPRF_OPERATOR_MAKERS and makeDefaultOPRFOperator accordingly, using the makeStwoOPRFOperator symbol if available.
🧹 Nitpick comments (2)
src/scripts/build-browser.sh (1)
8-14: Consider failing the build when STWO resources are missing.The script emits warnings but continues when
s2circuits.jsors2circuits_bg.wasmare absent. Given thatgenerate-receipt.tsnow defaults to STWO (instead of SnarkJS), a build missing these resources will fail at runtime. Consider making this a fatal error:♻️ Proposed fix to fail on missing resources
# ensure stwo resources exist (s2circuits.js + s2circuits_bg.wasm) +STWO_MISSING=0 if [ ! -f ./browser/resources/stwo/s2circuits.js ]; then - echo "Warning: stwo/s2circuits.js not found in resources" + echo "Error: stwo/s2circuits.js not found in resources" + STWO_MISSING=1 fi if [ ! -f ./browser/resources/stwo/s2circuits_bg.wasm ]; then - echo "Warning: stwo/s2circuits_bg.wasm not found in resources" + echo "Error: stwo/s2circuits_bg.wasm not found in resources" + STWO_MISSING=1 fi +if [ $STWO_MISSING -eq 1 ]; then + echo "STWO resources missing. Please ensure `@reclaimprotocol/zk-symmetric-crypto` includes stwo resources." + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/build-browser.sh` around lines 8 - 14, The build script currently only warns when STWO resources s2circuits.js and s2circuits_bg.wasm are missing; change it to fail the build by printing an error and exiting non‑zero. Update the checks in build-browser.sh that look for ./browser/resources/stwo/s2circuits.js and ./browser/resources/stwo/s2circuits_bg.wasm to emit an error (to stderr) and call exit 1 when either file is absent (or set a missing flag and exit 1 after both checks); this ensures generate-receipt.ts's default STWO path won't break at runtime.src/scripts/fallbacks/stwo.ts (1)
16-34: Consider using more efficient string building for large arrays.The
fromUint8Arrayfunction uses string concatenation in a loop, which has O(n²) complexity due to string immutability. For large ciphertext blocks, this could impact performance.♻️ Optional performance improvement
const Base64 = { fromUint8Array(arr: Uint8Array): string { - let binary = '' - for(const element of arr) { - binary += String.fromCharCode(element) - } - - return btoa(binary) + // Use apply with chunks to avoid call stack limits while maintaining O(n) complexity + const CHUNK_SIZE = 0x8000 + const chunks: string[] = [] + for(let i = 0; i < arr.length; i += CHUNK_SIZE) { + chunks.push(String.fromCharCode.apply(null, arr.subarray(i, i + CHUNK_SIZE) as unknown as number[])) + } + return btoa(chunks.join('')) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/fallbacks/stwo.ts` around lines 16 - 34, Base64.fromUint8Array currently concatenates characters in a loop causing O(n²) behavior for large arrays; replace the per-byte string concatenation with chunked batch conversion using String.fromCharCode on slices (e.g., 32k–64k sized chunks) and push each chunk into an array then join once and call btoa on the result to avoid quadratic concatenation and argument-length limits; keep Base64.toUint8Array unchanged. Ensure you update the Base64.fromUint8Array function accordingly and reference it by name when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scripts/fallbacks/stwo.ts`:
- Around line 96-106: The code currently uses non-null assertions (e.g.,
s2.initSync!, s2.generate_chacha20_proof!) which can throw cryptic runtime
errors if the WASM glue (getS2Circuits) doesn't expose those methods; update
initPromise and groth16Prove to perform explicit runtime checks: call
getS2Circuits(), verify that required functions (s2.initSync,
s2.generate_chacha20_proof, and any other methods used later) exist and typeof
=== 'function' before invoking them, and if any are missing throw a clear,
descriptive Error (e.g., "s2circuits missing initSync") so the failure is
explicit and avoid using non-null assertions anywhere in initPromise,
groth16Prove, and the code paths around lines 156-168.
- Around line 84-109: The module-level WASM state (wasmInitialized, initPromise)
and the release() behavior cause shared global state across all
makeStwoZkOperator instances; change ensureWasmInitialized and release handling
to use instance-aware reference counting or scoped initialization: implement a
shared counter incremented when makeStwoZkOperator (or the function that calls
ensureWasmInitialized) initializes WASM and decremented by each instance's
release(), and only actually reset wasmInitialized/initPromise and free WASM
when the counter reaches zero; update ensureWasmInitialized to increment the
refcount on successful init and ensure release() decrements the refcount and
only clears module-level flags when refcount == 0, keeping function names
ensureWasmInitialized, release, wasmInitialized, and initPromise as the
integration points.
- Around line 182-220: In groth16Verify, wrap the verification flow in a
try/catch and return false on any thrown error; specifically, first safely guard
access to publicSignals.noncesAndCounters (avoid direct [0] when
noncesAndCounters can be undefined), then call ensureWasmInitialized and
getS2Circuits as before but wrap assertU32Counter, TextDecoder.decode, the calls
to s2.verify_chacha20_proof! / s2.verify_aes_ctr_proof!, and JSON.parse in the
try block so any RangeError, decoding error, WASM error, or parse error is
caught; in catch log the error via logger?.warn and return false to ensure the
function never throws.
- Around line 145-180: In groth16Prove (function in
src/scripts/fallbacks/stwo.ts) change the returned object to include proofData
and plaintext to match downstream expectations: return the generated proof as
proofData (use result.proof) and include the plaintext buffer/value you created
earlier (plaintext derived from data.plaintext), so the function returns {
proofData: result.proof, plaintext: plaintext } instead of { proof: result.proof
}; ensure the return type aligns with ProveResult/consumer code.
In `@src/scripts/generate-receipt.ts`:
- Line 60: The current zk engine selection (const zkEngine =
getCliArgument('zk') === 'gnark' ? 'gnark' : 'stwo') causes unintended STWO
selection and will crash in Node because fallbacks/stwo.ts's getS2Circuits()
accesses window directly; update selection logic to explicitly support
'snarkjs', 'gnark', and 'stwo' (e.g., use the CLI arg value and default to
'snarkjs' in Node), and modify getS2Circuits() in fallbacks/stwo.ts to guard
browser globals (check typeof window !== 'undefined' and return a safe
failure/throw or fallback) so the CLI doesn't hit window.s2circuits in Node.
Ensure you reference getCliArgument, zkEngine, and getS2Circuits when making the
changes.
---
Outside diff comments:
In `@src/utils/zk.ts`:
- Around line 689-697: OPRF_OPERATOR_MAKERS currently only maps 'gnark' and
lacks a maker for the 'stwo' ZK engine, causing makeDefaultOPRFOperator to throw
when zkEngine == 'stwo'; fix by adding the STWO OPRF maker to the mapping (add
an entry 'stwo': makeStwoOPRFOperator) or, if STWO cannot support OPRF, add a
guard where makeDefaultOPRFOperator is invoked to prevent selecting 'stwo' when
OPRF is required and surface a clear error; update the OPRF_OPERATOR_MAKERS
constant or the selection logic referencing OPRF_OPERATOR_MAKERS and
makeDefaultOPRFOperator accordingly, using the makeStwoOPRFOperator symbol if
available.
---
Nitpick comments:
In `@src/scripts/build-browser.sh`:
- Around line 8-14: The build script currently only warns when STWO resources
s2circuits.js and s2circuits_bg.wasm are missing; change it to fail the build by
printing an error and exiting non‑zero. Update the checks in build-browser.sh
that look for ./browser/resources/stwo/s2circuits.js and
./browser/resources/stwo/s2circuits_bg.wasm to emit an error (to stderr) and
call exit 1 when either file is absent (or set a missing flag and exit 1 after
both checks); this ensures generate-receipt.ts's default STWO path won't break
at runtime.
In `@src/scripts/fallbacks/stwo.ts`:
- Around line 16-34: Base64.fromUint8Array currently concatenates characters in
a loop causing O(n²) behavior for large arrays; replace the per-byte string
concatenation with chunked batch conversion using String.fromCharCode on slices
(e.g., 32k–64k sized chunks) and push each chunk into an array then join once
and call btoa on the result to avoid quadratic concatenation and argument-length
limits; keep Base64.toUint8Array unchanged. Ensure you update the
Base64.fromUint8Array function accordingly and reference it by name when making
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11e65824-59f1-4768-84ba-2ed59fd70275
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
browser/index.htmlpackage.jsonproto/api.protosrc/client/create-claim.tssrc/proto/api.tssrc/scripts/build-browser.shsrc/scripts/build-browser.tssrc/scripts/build-jsc.tssrc/scripts/fallbacks/stwo.tssrc/scripts/generate-receipt.tssrc/server/utils/assert-valid-claim-request.tssrc/utils/zk.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Updates