feat: expose params via wasm#993
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughRefactors BFV parameter handling (adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SDK as EnclaveSDK
participant WASM as WASM Module
User->>SDK: encryptNumber(...)
SDK->>SDK: getProtocolParams()
SDK->>WASM: get_bfv_params(name)
WASM-->>SDK: BfvParamSet (serialized)
SDK->>WASM: bfv_encrypt(data, params)
WASM-->>SDK: encrypted
SDK-->>User: encrypted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus on:
Possibly related PRs
Suggested labels
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 |
ryardley
changed the title
ryardley
force-pushed
the
ry/990-crisp-e2e
branch
from
dddbd41 to
b43ba65
Compare
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
circuits/crates/libs/greco/src/lib.nr (1)
213-232: Update documentation to include thee0isparameter.The constructor documentation lists parameters
e0ande1but does not document the newe0isparameter, which represents per-CRT basis error polynomials forct0computation.Apply this documentation update:
/// * `u` - Secret polynomial from ternary distribution /// * `e0` - Error polynomial sampled from discrete Gaussian distribution /// * `e1` - Error polynomial sampled from discrete Gaussian distribution + /// * `e0is` - Per-CRT basis error polynomials for ct0 computation /// * `k1` - Scaled message polynomial /// * `r1is` - Randomness polynomials for ct0 computation
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yamltemplates/default/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
Cargo.toml(2 hunks)circuits/crates/libs/greco/src/lib.nr(5 hunks)crates/bfv-helpers/Cargo.toml(1 hunks)crates/bfv-helpers/src/client.rs(5 hunks)crates/bfv-helpers/src/lib.rs(24 hunks)crates/support-scripts/dev/Cargo.toml(1 hunks)crates/test-helpers/src/bin/fake_encrypt.rs(2 hunks)crates/test-helpers/src/bin/pack_e3_params.rs(2 hunks)crates/test-helpers/src/lib.rs(2 hunks)crates/wasm/Cargo.toml(1 hunks)crates/wasm/src/lib.rs(2 hunks)examples/CRISP/Cargo.toml(1 hunks)examples/CRISP/circuits/src/main.nr(2 hunks)examples/CRISP/crates/zk-inputs/src/serialization.rs(3 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)examples/CRISP/program/Cargo.toml(1 hunks)examples/CRISP/server/Cargo.toml(1 hunks)examples/CRISP/server/src/cli/commands.rs(3 hunks)examples/CRISP/server/src/server/routes/rounds.rs(2 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json(1 hunks)packages/enclave-contracts/package.json(1 hunks)packages/enclave-sdk/src/enclave-sdk.ts(9 hunks)packages/enclave-sdk/src/types.ts(4 hunks)packages/enclave-sdk/src/utils.ts(5 hunks)packages/enclave-sdk/tests/sdk.test.ts(3 hunks)templates/default/Cargo.toml(1 hunks)templates/default/deployed_contracts.json(1 hunks)templates/default/program/Cargo.toml(1 hunks)templates/default/program/src/lib.rs(2 hunks)templates/default/scripts/dev_program.sh(1 hunks)templates/default/tests/integration.spec.ts(3 hunks)templates/default/vitest.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
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.
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 345
File: packages/ciphernode/fhe/src/fhe.rs:47-91
Timestamp: 2025-04-18T09:26:41.728Z
Learning: The team wants to follow DRY principles and avoid duplicate implementation of functions like `decode_bfv_parameters` across different files in the codebase, preferring to centralize such utility functions.
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
crates/support-scripts/dev/Cargo.tomlexamples/CRISP/server/Cargo.tomlcrates/wasm/Cargo.tomltemplates/default/program/Cargo.tomlexamples/CRISP/program/Cargo.tomltemplates/default/Cargo.tomlCargo.tomlcrates/bfv-helpers/Cargo.tomlexamples/CRISP/Cargo.toml
📚 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:
crates/support-scripts/dev/Cargo.tomlexamples/CRISP/server/Cargo.tomltemplates/default/program/Cargo.tomlexamples/CRISP/program/Cargo.tomltemplates/default/Cargo.tomlCargo.tomlcrates/bfv-helpers/Cargo.tomlexamples/CRISP/Cargo.toml
📚 Learning: 2025-10-04T14:18:45.636Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 660
File: crates/support-scripts/src/program_dev.rs:17-18
Timestamp: 2025-10-04T14:18:45.636Z
Learning: In crates/support-scripts/src/program_dev.rs, the #[allow(dead_code)] attribute on ProgramSupportDev is intentionally kept even though the struct appears to be referenced elsewhere, as confirmed by the maintainer.
Applied to files:
crates/support-scripts/dev/Cargo.toml
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
examples/CRISP/server/Cargo.tomlexamples/CRISP/server/src/cli/commands.rstemplates/default/program/src/lib.rscrates/test-helpers/src/lib.rscrates/bfv-helpers/Cargo.toml
📚 Learning: 2025-11-07T16:17:58.942Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 972
File: circuits/crates/libs/greco/src/lib.nr:192-192
Timestamp: 2025-11-07T16:17:58.942Z
Learning: In the Greco library (circuits/crates/libs/greco/src/lib.nr), the e1is array should satisfy the constraint `e1 mod qi == e1is[i]` for each CRT basis i, where qi are the CRT moduli. This constraint ensures the relationship between the global error polynomial e1 and its per-basis representations e1is[i].
Applied to files:
examples/CRISP/circuits/src/main.nrcircuits/crates/libs/greco/src/lib.nr
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
examples/CRISP/circuits/src/main.nrcircuits/crates/libs/greco/src/lib.nr
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/main.nr
📚 Learning: 2024-10-23T01:59:42.967Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274
Timestamp: 2024-10-23T01:59:42.967Z
Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
Applied to files:
packages/enclave-sdk/tests/sdk.test.tstemplates/default/program/src/lib.rscrates/bfv-helpers/src/client.rscrates/test-helpers/src/bin/fake_encrypt.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
packages/enclave-sdk/tests/sdk.test.tstemplates/default/program/src/lib.rscrates/bfv-helpers/src/client.rscrates/test-helpers/src/bin/fake_encrypt.rs
📚 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:
packages/enclave-sdk/tests/sdk.test.tscrates/wasm/src/lib.rstemplates/default/program/src/lib.rscrates/test-helpers/src/lib.rscrates/bfv-helpers/src/client.rscrates/test-helpers/src/bin/fake_encrypt.rscrates/bfv-helpers/src/lib.rspackages/enclave-sdk/src/utils.tsexamples/CRISP/server/src/server/routes/rounds.rspackages/enclave-sdk/src/enclave-sdk.tspackages/enclave-sdk/src/types.ts
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
packages/enclave-sdk/tests/sdk.test.tstemplates/default/program/src/lib.rscrates/test-helpers/src/lib.rscrates/test-helpers/src/bin/fake_encrypt.rs
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/test-helpers/src/bin/pack_e3_params.rscrates/wasm/src/lib.rsexamples/CRISP/server/src/cli/commands.rstemplates/default/program/src/lib.rscrates/test-helpers/src/lib.rscrates/bfv-helpers/src/client.rscrates/test-helpers/src/bin/fake_encrypt.rscrates/bfv-helpers/src/lib.rsexamples/CRISP/server/src/server/routes/rounds.rs
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
examples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/routes/rounds.rs
📚 Learning: 2024-09-26T04:12:09.345Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: tests/basic_integration/test.sh:103-114
Timestamp: 2024-09-26T04:12:09.345Z
Learning: In `tests/basic_integration/test.sh`, the user prefers not to refactor the ciphernode addition section to reduce duplication.
Applied to files:
templates/default/scripts/dev_program.sh
📚 Learning: 2024-10-28T12:04:26.578Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave_node/src/ciphernode.rs:26-28
Timestamp: 2024-10-28T12:04:26.578Z
Learning: In the `setup_ciphernode` function in `packages/ciphernode/enclave_node/src/ciphernode.rs`, panicking on errors during setup is acceptable.
Applied to files:
templates/default/scripts/dev_program.sh
📚 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/program/Cargo.tomltemplates/default/Cargo.toml
📚 Learning: 2024-10-23T02:03:02.008Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/encryption.rs:45-45
Timestamp: 2024-10-23T02:03:02.008Z
Learning: In the `packages/ciphernode/keyshare/src/encryption.rs` file, the environment variable `CIPHERNODE_SECRET` is used for the encryption password. A secure secret management solution is not currently available, but may be considered in future iterations.
Applied to files:
crates/test-helpers/src/bin/fake_encrypt.rs
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 752
File: packages/enclave-contracts/contracts/Enclave.sol:15-17
Timestamp: 2025-09-19T11:16:53.825Z
Learning: The Enclave contract in the gnosisguild/enclave repository has not been deployed yet as of September 2025, so storage layout considerations for upgradeable contracts don't apply to current changes.
Applied to files:
templates/default/deployed_contracts.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json
📚 Learning: 2025-04-30T06:25:14.721Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 372
File: packages/ciphernode/events/src/eventbus.rs:15-15
Timestamp: 2025-04-30T06:25:14.721Z
Learning: EnclaveEvent implements Display in packages/ciphernode/events/src/enclave_event/mod.rs, which satisfies the Event trait requirement. Static analysis tools may incorrectly flag implementations as missing when they do exist.
Applied to files:
packages/enclave-sdk/src/types.ts
🧬 Code graph analysis (12)
crates/test-helpers/src/bin/pack_e3_params.rs (1)
crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_arc(266-288)
crates/wasm/src/lib.rs (1)
crates/bfv-helpers/src/lib.rs (3)
get_params_by_str(82-86)from(100-142)get_params_list(89-96)
examples/CRISP/server/src/cli/commands.rs (1)
crates/bfv-helpers/src/lib.rs (2)
build_bfv_params_from_set_arc(194-201)encode_bfv_params(302-316)
templates/default/tests/integration.spec.ts (3)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (1)
event(19-19)packages/enclave-sdk/src/index.ts (1)
RegistryEventType(46-46)packages/enclave-react/src/index.ts (1)
RegistryEventType(32-32)
templates/default/scripts/dev_program.sh (4)
crates/support-scripts/src/program_dev.rs (1)
start(29-35)crates/support-scripts/src/program.rs (1)
start(45-50)crates/support-scripts/src/program_risc0.rs (1)
start(28-48)crates/support-scripts/src/traits.rs (1)
start(13-13)
templates/default/program/src/lib.rs (2)
crates/bfv-helpers/src/lib.rs (2)
build_bfv_params_arc(266-288)encode_bfv_params(302-316)packages/enclave-sdk/src/utils.ts (1)
INSECURE_SET_2048_1032193_1(65-70)
crates/test-helpers/src/bin/fake_encrypt.rs (1)
crates/bfv-helpers/src/lib.rs (2)
build_bfv_params_from_set_arc(194-201)decode_bfv_params(335-399)
crates/bfv-helpers/src/lib.rs (3)
crates/wasm/src/lib.rs (1)
from(196-203)crates/trbfv/src/trbfv_config.rs (1)
params(34-36)templates/default/program/src/lib.rs (1)
test(39-74)
packages/enclave-sdk/src/utils.ts (1)
packages/enclave-sdk/src/index.ts (1)
BFV_PARAMS_SET(64-64)
examples/CRISP/server/src/server/routes/rounds.rs (1)
crates/bfv-helpers/src/lib.rs (2)
build_bfv_params_from_set_arc(194-201)encode_bfv_params(302-316)
packages/enclave-sdk/src/enclave-sdk.ts (4)
packages/enclave-sdk/src/types.ts (2)
ProtocolParams(307-324)ProtocolParamsName(326-331)crates/wasm/init_web.js (1)
initializeWasm(11-29)crates/wasm/init_node.js (1)
initializeWasm(7-9)crates/wasm/src/lib.rs (5)
get_bfv_params(165-174)bfv_encrypt_number(32-42)bfv_encrypt_vector(62-72)bfv_verifiable_encrypt_number(93-108)bfv_verifiable_encrypt_vector(129-144)
packages/enclave-sdk/src/types.ts (2)
packages/enclave-sdk/src/index.ts (6)
EnclaveEventData(36-36)RegistryEventType(46-46)RegistryEventData(37-37)EventCallback(21-21)AllEventTypes(23-23)EnclaveEvent(24-24)packages/enclave-react/src/index.ts (4)
RegistryEventType(32-32)EventCallback(19-19)AllEventTypes(18-18)EnclaveEvent(20-20)
⏰ 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). (6)
- GitHub Check: ciphernode_integration_test (base)
- GitHub Check: ciphernode_integration_test (persist)
- GitHub Check: template_integration
- GitHub Check: crisp_e2e
- GitHub Check: rust_integration
- GitHub Check: Build & Push Image
🔇 Additional comments (29)
packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json (1)
854-854: Artifact rebuild is expected; ABI and contract interface unchanged.The
buildInfoIdchanged due to recompilation, which is normal when build environment or dependencies change. The contract ABI, bytecode, and all functional definitions remain identical, so this has no impact on contract behavior or SDK integration.Optionally verify that this artifact rebuild is an expected side effect of the upstream integration and parameter refactoring changes in this PR.
packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json (1)
980-980: Artifact regeneration is expected.The
buildInfoIdchanged as a result of recompilation, which is normal when dependencies are updated or source contracts are rebuilt. The ABI and contract interface remain unchanged, confirming backward compatibility.packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json (1)
538-538: Auto-generated artifact with expected buildInfoId update.This is a Solidity compiler artifact file. The
buildInfoIdhash changed due to recompilation, which is expected and benign. All functional content (ABI, bytecode, function signatures) remains identical and unchanged.templates/default/scripts/dev_program.sh (1)
6-6: Streamlined startup flow with correct bash chaining.The refactor replaces the multi-step startup logic with a cleaner pattern that waits for the ciphernodes readiness sentinel, then immediately starts the enclave program. The
&&operator correctly sequences the commands—ifpnpm wait-onfails, the script exits due toset -e(line 3); if it succeeds, the program starts.Verify that
pnpm wait-on file:/tmp/enclave_ciphernodes_readycorrectly monitors file creation in your environment and that the wait-on timeout behavior (if any) is acceptable for the dev workflow.templates/default/deployed_contracts.json (3)
70-82: Verify thesubmissionWindowchange from 3 to 10 aligns with test expectations.Line 78 shows
submissionWindowchanged from "3" to "10"—a 3x increase that affects contract behavior. While this may relate to solving template/default test issues mentioned in the PR objectives, please confirm this is intentional and not a deployment configuration error.
91-93: Verifyparamsschema change is compatible with contract and WASM exposure.The
paramsfield changed from a single hex string (sepolia, line 5) to an array containing one hex string (localhost, lines 91–93). This schema change relates to the PR's WASM parameter exposure feature, but please confirm:
- The Enclave contract constructor accepts an array (not a single string)
- The WASM binding correctly handles the array format
- This change doesn't break existing contract initialization logic
119-125: Resolve inconsistentblockNumberpresence across contracts.Contracts
MockRISC0Verifier(line 119–120),ImageID(line 122), andInputValidator(line 125) lackblockNumberfields, while most other contracts have them. This inconsistency could cause parsing errors if code expects all contracts to have ablockNumber.Please clarify:
- Is this intentional (e.g., these are not freshly deployed)?
- Should missing
blockNumbervalues be added for consistency?- If intentional, is the parsing logic defensive against missing fields?
packages/enclave-contracts/package.json (1)
161-161:--discflag is valid and has no negative impact.The
--discflag is a documented, standard option in solhint v5.0.5 that disables version update checks. Adding this flag to CI/build scripts is a common best practice as it prevents network calls during linting, improving performance and determinism. No issues found.packages/enclave-sdk/src/utils.ts (2)
116-144: Parameter rename does not break any callers.All references to
encodeBfvParamsin the codebase call it with no arguments (relying on defaults):
templates/default/tests/integration.spec.ts:191:encodeBfvParams()templates/default/client/src/pages/steps/RequestComputation.tsx:109:encodeBfvParams()Neither caller uses named parameters, so the
error2_variance→error1_variancerename poses no breaking change risk.
64-70: Verify the field name breaking change is intentional; constant rename has no public API impact.The constant rename from
SET_2048_1032193_1toINSECURE_SET_2048_1032193_1does not break the public API since individual constants are not exported. Users access parameter sets only through the exportedBFV_PARAMS_SETobject.However, the field rename from
error2_variancetoerror1_varianceis a breaking change to the structure of exportedBFV_PARAMS_SET. All internal references have been consistently updated across the codebase. This appears to be an intentional refactoring, likely aligning with upstream changes from fhe.rs.Confirm this field rename is intentional and document it as a breaking change in release notes if applicable.
templates/default/vitest.config.ts (1)
9-19: Configuration is appropriate for integration tests with cryptographic/WASM operations.The Vitest configuration with
singleFork: trueand 2-minute timeout is suitable for the integration tests (tests/integration.spec.ts) that run complex E3/enclave scenarios. The forked pool isolates the test environment, and the extended timeout accommodates multi-service coordination (EVM, ciphernodes, server, program). Verify thatsingleFork: truedoesn't hide inter-test dependencies by confirming tests execute successfully via:pnpm test:integrationtemplates/default/program/src/lib.rs (2)
29-31: LGTM: Clear security-conscious naming.The update to
INSECURE_SET_2048_1032193_1makes it explicit that these parameters are not suitable for production use, which is good security practice for test/template code.
42-43: LGTM: Correct API usage with sensible defaults.The
build_bfv_params_arccall correctly uses the new constant and relies on the defaulterror1_varianceof "10" when None is passed, as confirmed by the implementation incrates/bfv-helpers/src/lib.rs.templates/default/tests/integration.spec.ts (1)
221-224: LGTM: Enhanced test observability.The added console.log statements and improved assertion messages provide better visibility into the test execution flow, making it easier to debug integration test failures.
Also applies to: 230-230, 242-242, 246-247, 251-251, 279-279
Cargo.toml (2)
36-42: LGTM: Proper workspace organization.The exclude list updates correctly prevent double workspace membership for self-contained examples and templates, which is the proper approach for Cargo workspace management.
153-153: LGTM: Dependencies support enum serialization for WASM.Adding
serde-wasm-bindgenandstrum(with derive feature) provides the necessary serialization infrastructure for exposing the BFV parameter enum to TypeScript via WASM bindings.Also applies to: 160-160
packages/enclave-sdk/src/types.ts (3)
320-323: LGTM: TypeScript API aligns with Rust parameter structure.Adding
error1VariancetoProtocolParamscorrectly exposes the error variance parameter that was previously internal, maintaining consistency with the Rust BFV parameter API.
326-331: LGTM: Type-safe parameter set naming.The
ProtocolParamsNameunion type provides compile-time safety for parameter set selection and aligns with the new enum-based parameter system introduced in the Rust codebase.
347-347: LGTM: Consistent default error variance.The
error1Variance: "10"default matches the Rust implementation's default value (as seen inbuild_bfv_params_arcwhen None is passed), ensuring consistency between TypeScript and Rust parameter definitions.Also applies to: 365-365
examples/CRISP/server/Cargo.toml (1)
45-46: LGTM: Consolidated workspace dependency management.Switching to workspace-based dependency resolution for
e3-compute-providerande3-sdkimproves consistency and simplifies version management across the codebase.packages/enclave-sdk/tests/sdk.test.ts (1)
35-35: LGTM: Formatting consistency.The trailing comma additions follow modern TypeScript conventions and improve diff clarity for future changes.
Also applies to: 44-44, 50-50, 60-60, 64-64, 72-72, 78-78
crates/wasm/Cargo.toml (1)
14-15: LGTM: WASM serialization infrastructure.Adding
serdeandserde-wasm-bindgenprovides the necessary serialization support for exposing BFV parameters to JavaScript, directly supporting the PR's goal of parameter retrieval via WASM bindings.crates/bfv-helpers/src/client.rs (2)
128-128: LGTM: Import aligns with enum-based parameter API.Updating the import from
crate::paramstocrate::BfvParamscorrectly reflects the refactoring to an enum-based parameter system.
138-138: LGTM: Tests migrated to enum-based parameter selection.The test updates consistently use
BfvParams::InsecureSet2048_1032193_1.into()to construct parameter sets, demonstrating proper adoption of the new enum API while maintaining the same test semantics.Also applies to: 163-163, 195-195, 226-226
examples/CRISP/crates/zk-inputs/src/serialization.rs (1)
39-39: LGTM! Consistent field rename from e1is to e0is.The field rename is applied consistently across the struct definition, construction logic, and test fixtures.
Also applies to: 218-226, 319-322
examples/CRISP/circuits/src/main.nr (1)
36-36: LGTM! Parameter rename aligns with Greco library changes.The parameter name change from
e1istoe0isis consistent with the updated Greco constructor signature.Also applies to: 84-84
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
156-156: LGTM! Interface field updated consistently.The interface field rename from
e1istoe0isaligns with the backend serialization changes.circuits/crates/libs/greco/src/lib.nr (2)
190-192: Clarify the relationship betweene0ande0ispolynomials.The struct contains both
e0(single polynomial) ande0is(per-CRT array). Currently:
e0is range-checked (line 321) and included in the payload but NOT used in verificatione0isis used in verification (lines 412-415) but NOT range-checked or in the payloadBased on learnings, per-CRT representations should satisfy
e0 mod qi == e0is[i]for each CRT basis. This constraint is not explicitly enforced.Please verify:
- Should
e0ishave its own range checks, or does thee0range check sufficiently constraine0is?- Should there be an explicit constraint relating
e0ande0is[i](e.g.,e0 mod qi == e0is[i])?- Is
e0needed in the struct if it's not used in verification, or is there missing logic?Also applies to: 321-321, 412-415
251-273: Verify whethere0isshould be included in the Fiat-Shamir payload and clarify the design relationship betweene0/e0isande1/e1is.The review comment raises a reasonable concern:
e0isis used in constraint verification (lines 412, 415) but excluded frompayload(). The payload function's documentation states "The sponge absorbs all witness values" (line 361), yete0isis missing while all other per-basis witness arrays (r1is,r2is,p1is,p2is) are included.However, verification reveals inconsistencies that prevent a definitive determination:
e0isis not documented in the constructor parameter documentation (onlye0ande1are mentioned)- The constraint documentation shows
e0(gamma)but the code evaluatese0is[i]per basise0isappears to be a witness parameter but its relationship toe0is not clearly explained (CRT decomposition?)- Notably, the learnings reference an
e1isarray that should exist per CRT constraints, but it does not exist in the codebaseRecommended action: Manually verify that the inclusion/exclusion of
e0isis intentional and that the implementation matches the intended Fiat-Shamir protocol for this circuit. Clarify the relationship between error polynomials and their per-basis representations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/test-helpers/src/bin/pack_e3_params.rs (2)
36-36: Use.is_empty()for idiomatic Rust.Consider using
.is_empty()instead oflen() == 0for better readability and idiomatic Rust code.Apply this diff:
- if args.moduli.len() == 0 { + if args.moduli.is_empty() {
41-55: Eliminate code duplication in the if-else block.The
build_bfv_params_arccall is duplicated in both branches with only the last parameter differing. This violates DRY principles and can be simplified.Apply this diff to consolidate the logic:
- if let Some(error1_variance) = args.error1_variance { - params = build_bfv_params_arc( - args.degree as usize, - args.plaintext_modulus, - &args.moduli, - Some(&error1_variance), - ); - } else { - params = build_bfv_params_arc( - args.degree as usize, - args.plaintext_modulus, - &args.moduli, - None, - ); - } + params = build_bfv_params_arc( + args.degree as usize, + args.plaintext_modulus, + &args.moduli, + args.error1_variance.as_deref(), + );Based on learnings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/test-helpers/src/bin/pack_e3_params.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
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.
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 345
File: packages/ciphernode/fhe/src/fhe.rs:47-91
Timestamp: 2025-04-18T09:26:41.728Z
Learning: The team wants to follow DRY principles and avoid duplicate implementation of functions like `decode_bfv_parameters` across different files in the codebase, preferring to centralize such utility functions.
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/test-helpers/src/bin/pack_e3_params.rs
🧬 Code graph analysis (1)
crates/test-helpers/src/bin/pack_e3_params.rs (2)
crates/trbfv/src/trbfv_config.rs (1)
params(34-36)crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_arc(266-288)
⏰ 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). (10)
- GitHub Check: build_sdk
- GitHub Check: build_e3_support_dev
- GitHub Check: test_contracts
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (1)
crates/test-helpers/src/bin/pack_e3_params.rs (1)
28-29: Previous issue resolved: CLI flag now matches field name.The CLI flag has been correctly updated to
--error1-varianceto match the internal field nameerror1_variance. This resolves the inconsistency flagged in the previous review.
3 participants
fix enclave init scriptdifferent PREnclave SDK was really quickly thrown together for the hackathon and really needs to be rewritten at this point so I am not really concerned with having a good design there - I just want to try and consolidate parameters so they are configured in a single place. This PR hacks bfv params in for now. I will add a ticket for rewriting the sdk.
Summary by CodeRabbit
New Features
API Changes
Chores