chore: set target web/nodejs for crisp zk-inputs#1006
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds explicit WASM init entrypoints (node + web), embeds web WASM as base64, updates package exports and build scripts, calls the new async initializer from CRISP SDK vote functions, expands Vite optimizeDeps exclusion, and removes a top-level build:wasm script. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant SDK as CRISP SDK
participant Init as `@crisp-e3/zk-inputs/init`
participant Bindgen as WASM Bindgen
Dev->>SDK: call encryptVote()
activate SDK
SDK->>Init: await init()
activate Init
alt first web init
Init->>Init: import base64 module
Init->>Init: decode -> Uint8Array
Init->>Bindgen: bindgen.initSync(bytes)
Bindgen-->>Init: initialized
Init->>Init: cache Promise
else node or cached
Init-->>Init: no-op or return cached
end
Init-->>SDK: resolved
deactivate Init
SDK->>SDK: proceed with WASM-dependent crypto
SDK-->>Dev: encrypted vote
deactivate SDK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧬 Code graph analysis (1)examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)
⏰ 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). (8)
🔇 Additional comments (6)
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)
examples/CRISP/packages/crisp-zk-inputs/init_node.cjs (1)
7-9: Consider accepting the parameter for signature consistency.While the no-op implementation is correct for Node.js, consider accepting the
initParamsparameter (even if unused) to match the signature ofinit_web.js. This makes the API more consistent across environments.Apply this diff:
-module.exports = async function initializeWasm() { +module.exports = async function initializeWasm(initParams) { // Node does not need to be loaded async }examples/CRISP/packages/crisp-zk-inputs/init_web.js (2)
11-11: Remove unused parameter.The
initParamsparameter is accepted but never used in the function body. Either utilize it (if it's meant for wasm-bindgen's init options) or remove it for clarity.If not needed:
-export default async function initializeWasm(initParams) { +export default async function initializeWasm() {
12-26: Consider adding error handling for WASM initialization.The memoization pattern is well-implemented, ensuring single initialization even with concurrent calls. However, if the WASM initialization fails, the rejected promise is cached, and subsequent calls will receive the same rejection.
Consider resetting
promiseon failure to allow retry:export default async function initializeWasm(initParams) { - promise ??= (async () => { + promise ??= (async () => { + try { const { default: base64 } = await import("./dist/web/index_base64.js"); 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); } bindgen.initSync(bytes); return bindgen; + } catch (error) { + promise = undefined; // Reset to allow retry + throw error; + } })(); return promise; }examples/CRISP/packages/crisp-zk-inputs/README.md (1)
11-11: Adjust heading levels for proper hierarchy.Using h5 (
#####) skips heading levels. Markdown best practices recommend incrementing by only one level at a time.Apply this diff:
-##### ❌ DONT USE THE DEFAULT INIT +### ❌ DONT USE THE DEFAULT INIT ```ts // Bad! Because this uses the raw loader which doesn't exist in node contexts import init, { bfvEncryptNumber } from "@crisp-e3/zk-inputs";-##### ✅ DO USE THE EXPORTED SUBMODULE
+### ✅ DO USE THE EXPORTED SUBMODULEAlso applies to: 18-18 </blockquote></details> <details> <summary>examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)</summary><blockquote> `7-9`: **No-op initialization for Node.js looks appropriate.** The empty implementation is correct since Node.js can load WebAssembly synchronously, unlike the browser environment. However, for API consistency with the web version (which uses memoization), consider explicitly returning a resolved promise or adding a JSDoc comment explaining why no initialization is needed. Optional improvement for clarity: ```diff +/** + * Initialize WASM for Node.js environment. + * No-op since Node.js loads WASM synchronously. + */ export default async function initializeWasm() { // Node does not need to be loaded async + return Promise.resolve(); }examples/CRISP/packages/crisp-zk-inputs/scripts/build.js (1)
17-32: Consider parallelizing wasm-pack builds for faster execution.Both builds output to different directories and can run concurrently.
Optional improvement:
- await execa("wasm-pack", [ - "build", - "../../crates/zk-inputs-wasm", - "--target=web", - `--out-dir=${distWeb}`, - "--no-pack", - "--out-name=index", - ]); - await execa("wasm-pack", [ - "build", - "../../crates/zk-inputs-wasm", - "--target=nodejs", - `--out-dir=${distNode}`, - "--no-pack", - "--out-name=index", - ]); + await Promise.all([ + execa("wasm-pack", [ + "build", + "../../crates/zk-inputs-wasm", + "--target=web", + `--out-dir=${distWeb}`, + "--no-pack", + "--out-name=index", + ]), + execa("wasm-pack", [ + "build", + "../../crates/zk-inputs-wasm", + "--target=nodejs", + `--out-dir=${distNode}`, + "--no-pack", + "--out-name=index", + ]), + ]);
📜 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 (11)
examples/CRISP/client/vite.config.ts(1 hunks)examples/CRISP/package.json(0 hunks)examples/CRISP/packages/crisp-sdk/package.json(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(4 hunks)examples/CRISP/packages/crisp-zk-inputs/README.md(1 hunks)examples/CRISP/packages/crisp-zk-inputs/init.d.ts(1 hunks)examples/CRISP/packages/crisp-zk-inputs/init_node.cjs(1 hunks)examples/CRISP/packages/crisp-zk-inputs/init_node.js(1 hunks)examples/CRISP/packages/crisp-zk-inputs/init_web.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 (1)
- examples/CRISP/package.json
🧰 Additional context used
🧬 Code graph analysis (5)
examples/CRISP/packages/crisp-zk-inputs/init_node.cjs (2)
examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)
initializeWasm(7-9)examples/CRISP/packages/crisp-zk-inputs/init_web.js (1)
initializeWasm(11-29)
examples/CRISP/packages/crisp-zk-inputs/init.d.ts (1)
packages/enclave-sdk/src/greco.ts (1)
Params(59-75)
examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)
examples/CRISP/packages/crisp-zk-inputs/init_web.js (1)
initializeWasm(11-29)
examples/CRISP/packages/crisp-zk-inputs/init_web.js (1)
examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)
initializeWasm(7-9)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)
initializeWasm(7-9)examples/CRISP/packages/crisp-zk-inputs/init_web.js (1)
initializeWasm(11-29)
🪛 LanguageTool
examples/CRISP/packages/crisp-zk-inputs/README.md
[grammar] ~3-~3: Ensure spelling is correct
Context: ...for consumption in typescript to enable use to share code between Rust and Typescri...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
examples/CRISP/packages/crisp-zk-inputs/README.md
11-11: Heading levels should only increment by one level at a time
Expected: h3; Actual: h5
(MD001, heading-increment)
⏰ 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). (7)
- GitHub Check: build_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: test_contracts
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: test_net
🔇 Additional comments (5)
examples/CRISP/client/vite.config.ts (1)
26-34: LGTM! Appropriate exclusion of initialization module.Excluding
@crisp-e3/zk-inputs/initfrom Vite's optimization is correct since this module handles runtime WASM initialization that shouldn't be pre-bundled.examples/CRISP/packages/crisp-zk-inputs/package.json (1)
48-53: Verify the "default" condition in "./init" exports.The
"default"condition includes bothimport(Node) anddefault(web) paths, which seems unusual. Typically, the"default"condition should specify a single fallback path, not multiple conditions.Review whether this configuration is intentional. The typical pattern would be:
"default": { "types": "./init.d.ts", "default": "./init_web.js" }Or if you want to support both import/require in the default case:
"default": { "types": "./init.d.ts", "import": "./init_node.js", "require": "./init_node.cjs" }The current setup with
"import","require", and"default"all at the same level in the"default"condition may not resolve as expected. Can you verify this works correctly in both Node and browser environments?examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
8-8: LGTM! Proper WASM initialization pattern.The initialization calls ensure WASM is ready before using
ZKInputsGenerator. The memoization ininit_web.jsensures efficient handling of multiple calls - the first call initializes, and subsequent calls reuse the cached promise.Also applies to: 157-157, 196-196, 245-245
examples/CRISP/packages/crisp-sdk/package.json (1)
23-25: LGTM! Simplified and clearer build flow.The streamlined build scripts properly ensure WASM compilation precedes SDK building and testing.
examples/CRISP/packages/crisp-zk-inputs/scripts/build.js (1)
34-36: LGTM!The base64 conversion logic is correct and aligns with the loading approach in
init_web.js.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/CRISP/packages/crisp-zk-inputs/README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
examples/CRISP/packages/crisp-zk-inputs/README.md
11-11: Heading levels should only increment by one level at a time
Expected: h3; Actual: h5
(MD001, heading-increment)
⏰ 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). (7)
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: test_contracts
- GitHub Check: rust_integration
- GitHub Check: rust_unit
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/CRISP/packages/crisp-zk-inputs/init.d.ts(1 hunks)examples/CRISP/packages/crisp-zk-inputs/init_node.cjs(1 hunks)examples/CRISP/packages/crisp-zk-inputs/init_node.js(1 hunks)examples/CRISP/packages/crisp-zk-inputs/init_web.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
examples/CRISP/packages/crisp-zk-inputs/init_node.cjs (2)
examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)
init(7-9)examples/CRISP/packages/crisp-zk-inputs/init_web.js (1)
init(11-29)
examples/CRISP/packages/crisp-zk-inputs/init_web.js (1)
examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)
init(7-9)
examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)
examples/CRISP/packages/crisp-zk-inputs/init_web.js (1)
init(11-29)
examples/CRISP/packages/crisp-zk-inputs/init.d.ts (2)
examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)
init(7-9)examples/CRISP/packages/crisp-zk-inputs/init_web.js (1)
init(11-29)
⏰ 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). (7)
- GitHub Check: build_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
- GitHub Check: test_net
🔇 Additional comments (3)
examples/CRISP/packages/crisp-zk-inputs/init_node.cjs (1)
7-9: LGTM! CommonJS variant maintains API consistency.The empty implementation is correct for Node.js, which can load WASM synchronously. This module provides CommonJS compatibility while maintaining the same async API signature as the ES module variants.
examples/CRISP/packages/crisp-zk-inputs/init_web.js (1)
7-9: LGTM! Proper setup for lazy initialization.The bindgen import and memoization variable are correctly structured to support the single-initialization pattern.
examples/CRISP/packages/crisp-zk-inputs/init_node.js (1)
7-9: LGTM! Node.js initialization correctly simplified.The empty implementation is appropriate since Node.js can load WASM synchronously. The async signature maintains API consistency with the web variant, allowing consumers to use the same initialization pattern across platforms.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ctrlc03
force-pushed
the
chore/crisp-wasm
branch
from
9cee417 to
1bce676
Compare
2 participants
Closes #1007
Summary by CodeRabbit
New Features
Documentation
Chores