chore(crisp): make sdk compatible with different envs#1010
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request consolidates and simplifies the CRISP example packages by bumping versions to 0.2.3-test, eliminating explicit BFV parameter passing in favor of defaults-based generation, refactoring the CRISPVerifier contract to remove public input and pairing-point handling, and streamlining WASM module initialization into a single entry point with unified build output. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
cedoor
changed the title
- Updated @crisp-e3/sdk to 0.2.1-test - Updated @crisp-e3/contracts to 0.2.1-test - Updated @crisp-e3/zk-inputs to 0.2.1-test - Published to npm
- Updated @crisp-e3/sdk to 0.2.2-test - Updated @crisp-e3/contracts to 0.2.2-test - Updated @crisp-e3/zk-inputs to 0.2.2-test - Published to npm
- Updated @crisp-e3/sdk to 0.2.3-test - Updated @crisp-e3/contracts to 0.2.3-test - Updated @crisp-e3/zk-inputs to 0.2.3-test - Published to npm
cedoor
force-pushed
the
chore/new-crisp-version
branch
from
23b268f to
fd54207
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
examples/CRISP/packages/crisp-zk-inputs/main.js (1)
10-16: Consider using a more concise decoding approach.The manual loop for base64 decoding is correct but verbose. Modern environments support more concise alternatives.
If Node.js compatibility is required, consider:
-const binaryString = atob(base64); -const len = binaryString.length; -const bytes = new Uint8Array(len); - -for (let i = 0; i < len; i++) { - bytes[i] = binaryString.charCodeAt(i); -} +const bytes = Uint8Array.from(atob(base64), c => c.charCodeAt(0));However, the current implementation is more readable and avoids potential compatibility issues with older environments, so this is purely a stylistic suggestion.
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
72-89: Wrap switch clause declarations in a block.The static analyzer correctly identifies that variable declarations in switch clauses can be accessed by other cases. While not causing issues in this specific case (only one case exists), wrapping in a block is a best practice.
switch (votingMode) { case VotingMode.GOVERNANCE: + { const voteArray = [] const length = bfvParams.degree const halfLength = length / 2 const yesBinary = toBinary(vote.yes).split('') const noBinary = toBinary(vote.no).split('') // Fill first half with 'yes' binary representation (pad with leading 0s if needed) for (let i = 0; i < halfLength; i++) { const offset = halfLength - yesBinary.length voteArray.push(i < offset ? '0' : yesBinary[i - offset]) } // Fill second half with 'no' binary representation (pad with leading 0s if needed) for (let i = 0; i < length - halfLength; i++) { const offset = length - halfLength - noBinary.length voteArray.push(i < offset ? '0' : noBinary[i - offset]) } return voteArray + } default: throw new Error('Unsupported voting mode') }examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (2)
489-491: Empty loop when publicInputsSize is zero.This loop iterates over publicInputsSize, which is now 0, so it will never execute. While functionally correct, consider adding an early return for clarity and minor gas savings:
bytes32[] memory round0 = new bytes32[](3 + publicInputsSize + 12); round0[0] = bytes32(circuitSize); round0[1] = bytes32(publicInputsSize); round0[2] = bytes32(pubInputsOffset); - for (uint256 i = 0; i < publicInputsSize; i++) { - round0[3 + i] = bytes32(publicInputs[i]); - } + if (publicInputsSize > 0) { + for (uint256 i = 0; i < publicInputsSize; i++) { + round0[3 + i] = bytes32(publicInputs[i]); + } + }
1594-1602: Empty loop simplification opportunity.When
numPublicInputsis 0, this loop never executes and the function returns the identity value (1/1 = 1). Consider an early return for clarity:function computePublicInputDelta(bytes32[] memory publicInputs, Fr beta, Fr gamma, uint256 offset) internal view returns (Fr publicInputDelta) { + if (numPublicInputs == 0) { + return Fr.wrap(1); + } + Fr numerator = Fr.wrap(1); Fr denominator = Fr.wrap(1);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.github/workflows/ci.yml(3 hunks)circuits/crates/libs/safe/Nargo.toml(1 hunks)examples/CRISP/client/libs/wasm/pkg/crisp_worker.js(0 hunks)examples/CRISP/client/package.json(2 hunks)examples/CRISP/client/vite.config.ts(1 hunks)examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol(9 hunks)examples/CRISP/packages/crisp-contracts/package.json(1 hunks)examples/CRISP/packages/crisp-sdk/package.json(3 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(0 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(0 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(5 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(6 hunks)examples/CRISP/packages/crisp-zk-inputs/init.d.ts(0 hunks)examples/CRISP/packages/crisp-zk-inputs/init_node.cjs(0 hunks)examples/CRISP/packages/crisp-zk-inputs/init_node.js(0 hunks)examples/CRISP/packages/crisp-zk-inputs/init_web.js(0 hunks)examples/CRISP/packages/crisp-zk-inputs/main.js(1 hunks)examples/CRISP/packages/crisp-zk-inputs/package.json(1 hunks)examples/CRISP/packages/crisp-zk-inputs/scripts/build.js(1 hunks)
💤 Files with no reviewable changes (7)
- examples/CRISP/packages/crisp-sdk/src/constants.ts
- examples/CRISP/packages/crisp-zk-inputs/init.d.ts
- examples/CRISP/packages/crisp-zk-inputs/init_web.js
- examples/CRISP/client/libs/wasm/pkg/crisp_worker.js
- examples/CRISP/packages/crisp-sdk/src/types.ts
- examples/CRISP/packages/crisp-zk-inputs/init_node.cjs
- examples/CRISP/packages/crisp-zk-inputs/init_node.js
✅ Files skipped from review due to trivial changes (1)
- examples/CRISP/packages/crisp-contracts/package.json
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 963
File: examples/CRISP/client/package.json:25-25
Timestamp: 2025-11-05T14:12:57.814Z
Learning: In the Enclave/CRISP codebase, `enclave-e3/sdk` and `crisp-e3/sdk` are different packages: `enclave-e3/sdk` is the general Enclave SDK, while `crisp-e3/sdk` is the CRISP-specific SDK. The CRISP client (`examples/CRISP/client`) intentionally depends on `enclave-e3/sdk`, not `crisp-e3/sdk`.
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 996
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:144-169
Timestamp: 2025-11-12T10:08:30.693Z
Learning: In the CRISP program (examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol), there are plans to move the merkle tree from the Enclave contract to the program contract itself in the future, which would allow direct calls to validateInput to be stored in the merkle tree.
📚 Learning: 2025-11-08T01:31:47.484Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 969
File: packages/enclave-sdk/src/enclave-sdk.ts:101-103
Timestamp: 2025-11-08T01:31:47.484Z
Learning: In packages/enclave-sdk/src/enclave-sdk.ts, the TRBFV protocol parameter setup is intentionally incomplete as of PR #969. The constructor sets BFV_THRESHOLD parameters for TRBFV, but the encryption methods (encryptNumber, encryptVector, encryptNumberAndGenInputs, encryptVectorAndGenInputs) intentionally only support BFV and will throw "Protocol not supported" for TRBFV until future implementation is completed.
Applied to files:
examples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/packages/crisp-sdk/tests/vote.test.ts
📚 Learning: 2025-08-27T13:49:03.811Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: .github/workflows/ci.yml:202-206
Timestamp: 2025-08-27T13:49:03.811Z
Learning: In GitHub Actions workflows for Noir circuits, the user prefers to use "stable" toolchain with noir-lang/noirup action rather than pinning to specific versions like 1.0.0-beta.11, and prefers to update the noirup action version (e.g., to 0.1.4) to get tooling improvements.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2025-11-05T14:12:57.814Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 963
File: examples/CRISP/client/package.json:25-25
Timestamp: 2025-11-05T14:12:57.814Z
Learning: In the Enclave/CRISP codebase, `enclave-e3/sdk` and `crisp-e3/sdk` are different packages: `enclave-e3/sdk` is the general Enclave SDK, while `crisp-e3/sdk` is the CRISP-specific SDK. The CRISP client (`examples/CRISP/client`) intentionally depends on `enclave-e3/sdk`, not `crisp-e3/sdk`.
Applied to files:
examples/CRISP/client/package.jsonexamples/CRISP/packages/crisp-sdk/package.jsonexamples/CRISP/packages/crisp-zk-inputs/package.json
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
circuits/crates/libs/safe/Nargo.toml
🧬 Code graph analysis (2)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (2)
vote(26-26)encodedVote(37-37)examples/CRISP/packages/crisp-sdk/src/types.ts (2)
IVote(81-90)BFVParams(179-183)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (4)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
BFVParams(179-183)examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
generateMaskVote(237-277)examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (4)
merkleProof(31-35)vote(26-26)encodedVote(37-37)encryptedVote(38-38)examples/CRISP/packages/crisp-sdk/tests/constants.ts (2)
merkleProof(33-33)testAddress(32-32)
🪛 Biome (2.1.2)
examples/CRISP/packages/crisp-sdk/src/vote.ts
[error] 72-72: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 73-73: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: crisp_e2e
- GitHub Check: build_sdk
- GitHub Check: rust_unit
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
🔇 Additional comments (16)
.github/workflows/ci.yml (1)
245-245: Verify the Nargo toolchain version downgrade across three jobs.The toolchain has been downgraded from v1.0.0-beta.11 to v1.0.0-beta.3 consistently across the
integration_prebuild,crisp_e2e, andtest_enclave_circuitsjobs. While this aligns with the PR title's goal to ensure compatibility across different environments, the rationale for this specific downgrade and whether a later version or the "stable" tag would work instead needs clarification.Can you confirm:
- Is this downgrade necessary for environment compatibility, or was a compatibility issue discovered with beta.11?
- Have you tested whether using the "stable" toolchain tag (as suggested in learnings from PR #648) would work for these environments?
- Are there any known issues with v1.0.0-beta.11 that necessitate v1.0.0-beta.3?
Also applies to: 428-428, 500-500
circuits/crates/libs/safe/Nargo.toml (1)
10-10: No inconsistencies found—sha256 version is consistent across the workspace.The verification confirms that
circuits/crates/libs/safe/Nargo.tomlis the only location in the workspace where the sha256 dependency is defined. The v0.1.0 version is already applied uniformly, so no further updates are needed.examples/CRISP/client/package.json (1)
22-22: LGTM! Version bump aligns with SDK changes.The dependency version update to
0.2.3-testis consistent with the coordinated version bumps across the CRISP package suite.examples/CRISP/packages/crisp-sdk/package.json (2)
19-21: LGTM! Top-level fields improve compatibility.Adding
main,module, andtypesat the top level alongside theexportsfield improves compatibility with older tooling that doesn't fully support the exports field.
46-48: Verify dependency downgrades are intentional.The dependencies have been downgraded to older versions:
@aztec/bb.js: 0.87.0 → 0.82.2@noir-lang/noir_js: 1.0.0-beta.9 → 1.0.0-beta.3Please confirm these downgrades are intentional and necessary for compatibility with the new SDK architecture. Consider verifying that these older versions don't have known security issues or critical bugs.
examples/CRISP/packages/crisp-zk-inputs/package.json (1)
11-24: LGTM! Unified entry point simplifies the public API.The consolidation from environment-specific entry points (node/web) to a single
main.jsloader simplifies the package surface and should improve compatibility across different environments. The new structure with a unifieddist/directory and single entry point aligns well with modern bundler capabilities.examples/CRISP/packages/crisp-zk-inputs/main.js (1)
7-20: LGTM! Clean WASM loader pattern.The loader correctly decodes the base64-encoded WASM binary, initializes the module, and re-exports the public API. This pattern enables bundlers to properly handle the WASM binary without requiring special plugins.
examples/CRISP/packages/crisp-zk-inputs/scripts/build.js (1)
13-39: LGTM! Unified build output simplifies the artifact structure.The consolidation from separate
dist/webanddist/nodedirectories to a singledist/output directory simplifies the build process and aligns with the unified entry point approach inmain.js. All file paths are correctly updated to reference the new location.examples/CRISP/client/vite.config.ts (1)
25-25: LGTM! Simplified optimization configuration.The streamlined
optimizeDeps.excludelist removes CRISP-specific package exclusions, which aligns with the new unified build approach. The remaining exclusions for Noir and Aztec packages are appropriate for their specific requirements.examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (2)
31-32: LGTM! Defaults-based approach simplifies test setup.Using
ZKInputsGenerator.withDefaults()and extracting BFV params only when needed for assertions significantly simplifies the test code. This approach reduces boilerplate while maintaining test coverage.
214-214: Let me search more broadly to understand the full scope of BFV parameter testing and validation.Let me search for other test files and validation logic in the CRISP SDK:Now let me search for more specific information about test file structure and whether custom BFV parameters are validated elsewhere:
Based on the learnings and evidence gathered, I need to generate the final response now. The learnings note that custom parameter setup (like TRBFV) is intentionally incomplete, and the first script showed no custom BFV parameter tests remaining in the test file. Let me generate the rewritten review comment:
Confirm that custom BFV parameter validation is intentionally handled via defaults-only approach or verify alternative coverage exists.
The removal of the "should work with small moduli" test eliminates coverage of custom plaintext modulus edge cases. Based on the codebase search, only default BFV parameters are currently tested in
vote.test.ts(viaZKInputsGenerator.withDefaults()). Confirm whether:
- Custom parameter validation/testing is covered by a different test suite or validation layer
- The defaults-only approach is intentional and sufficient for current needs
- Integration tests or validation code elsewhere verify parameter edge cases
This matters because plaintext modulus is a critical cryptographic parameter affecting noise budget and operational correctness.
examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
64-67: LGTM! Defaults-based encoding simplifies the API.Moving to
ZKInputsGenerator.withDefaults()internally eliminates the need for callers to manage BFV parameters explicitly. This significantly simplifies the public API while maintaining flexibility through the defaults mechanism.
166-169: LGTM! Useful addition to the public API.The new
generatePublicKey()function provides a convenient way to generate public keys without requiring callers to instantiateZKInputsGeneratordirectly, improving the developer experience.
197-199: LGTM! Consistent defaults-based implementation.The refactoring to use
ZKInputsGenerator.withDefaults()throughoutencryptVoteAndGenerateCRISPInputsandgenerateMaskVotecreates a consistent, simplified API surface that reduces the burden on callers while maintaining correctness.Also applies to: 244-244
examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (2)
677-721: LGTM! Proof loading structure is correct.The proof loading has been updated to use fixed offsets without pairing-point data. The offset calculations are correct:
- Free wires and lookup helpers: 8 × 0x80 = 0x400 bytes
- Boundary correctly set to 0x400 for remaining proof elements
The structure aligns with the reduced PROOF_SIZE of 440 field elements.
1545-1545: LGTM! PROOF_SIZE calculation is correct.PROOF_SIZE decreased from 456 to 440. The reduction of 16 field elements aligns with the removal of pairing-point data from the proof structure. The new size correctly accounts for:
- 8 G1ProofPoints (32 FEs)
- Sumcheck data (264 FEs)
- Gemini data (136 FEs)
- Shplonk and KZG (8 FEs)
Total: 440 field elements
2 participants
This PR aligns the Noir and Aztec versions across the Enclave SDK, the CRISP SDK, and the CI configuration. Anyone working with the circuits now needs the following versions to compile and generate the Solidity verifier contracts:
Since the CRISP SDK will be used mainly in web environments, the wasm-pack target for building zk-input has been switched to web. The resulting WASM file is now distributed as base64 in every environment, reducing compatibility issues across different runtimes.
Summary by CodeRabbit
Release Notes v0.2.3-test
New Features
generatePublicKey()function to the SDKRefactor
Chores