chore: add prod param on deployment#1515
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds BFV parameter presets and ABI-encodes/registers two param sets in the deploy script; introduces an SDK type for BFV param sets; raises multiple collection/network timeouts and payload limits; sets JSON body limits in servers; updates dashboard event filtering/UI and template deployment metadata and test timing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DeployScript
participant Provider as Blockchain/Provider
participant EnclaveContract as Enclave (on-chain)
DeployScript->>Provider: deploy or attach Enclave contract
DeployScript->>DeployScript: encodeBfvParams(insecure512) using ethers.AbiCoder
DeployScript->>EnclaveContract: setParamSet(0, encodedInsecure)
Provider-->>DeployScript: tx confirmation
DeployScript->>DeployScript: encodeBfvParams(secure8192) using ethers.AbiCoder
DeployScript->>EnclaveContract: setParamSet(1, encodedSecure)
Provider-->>DeployScript: tx confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 66-92: Replace the direct ethers.AbiCoder encoding for
encodedInsecure and encodedSecure with the SDK utility encodeBfvParams (use
encodeBfvParams(paramsInsecure) and encodeBfvParams(paramsSecure)) so the
built-in guard handles optional error1Variance; also add the import for
encodeBfvParams from packages/enclave-sdk/src/utils.ts and remove the direct
AbiCoder usage for these two variables to avoid encoding an undefined string
into a required Solidity string.
In `@templates/default/tests/integration.spec.ts`:
- Line 174: The test is using a secure BFV preset
(ThresholdBfvParamsPresetNames[1]) but the suite must remain on the insecure
preset; update the configuration so thresholdBfvParamsPresetName uses the
insecure preset value (e.g., ThresholdBfvParamsPresetNames[0]) wherever it's set
(refer to the thresholdBfvParamsPresetName key and any other occurrences of
ThresholdBfvParamsPresetNames[1] in the test file, including the second
occurrence mentioned) to avoid mixing presets across the test suite.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b145ff2-c5ba-4d58-a07c-cf4097467a63
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/package.jsonpackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-sdk/src/crypto/types.tstemplates/default/tests/integration.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/dashboard/src/dashboard.html`:
- Around line 229-234: The severity "warning" predicate currently includes both
WARNING_TYPES and ERROR_TYPES; update the filtering logic where severityFilter
is checked (the code referencing severityFilter, WARNING_TYPES and ERROR_TYPES)
so that when severityFilter === 'warning' it only matches WARNING_TYPES (and
explicitly excludes ERROR_TYPES). Concretely, change the conditional that
returns events for 'warning' to test membership in WARNING_TYPES only (not union
with ERROR_TYPES) and ensure the analogous fix is applied to the duplicate block
around the other occurrence (lines near where severityFilter is used again).
In `@crates/net/src/net_interface.rs`:
- Around line 58-60: The current MAX_KADEMLIA_PAYLOAD_MB is being reused to
configure both the transport via set_max_packet_size(...) and the in-memory
Kademlia store via MemoryStoreConfig.max_value_bytes, which allows the store to
accept huge records (combined with DHT_MAX_RECORDS) and risks OOM; introduce a
separate, much smaller constant (e.g., MEMORY_STORE_MAX_VALUE_BYTES or
MAX_KADEMLIA_STORE_BYTES) and use that for MemoryStoreConfig.max_value_bytes
while keeping MAX_KADEMLIA_PAYLOAD_MB for set_max_packet_size(...); choose the
store cap to match the largest expected record size (not 100MB) and leave
DHT_MAX_RECORDS unchanged.
In `@crates/zk-prover/src/actors/commitment_links/mod.rs`:
- Around line 117-120: The C2→C4 share-commitment links
(C2aToC4aShareCommitmentLink and C2bToC4bShareCommitmentLink) were commented out
from default_links(), creating a verifier-integrity gap; re-enable those
Box::new(...) entries in default_links() so C4 expected_commitments remain
validated against C2 outputs, and instead implement an explicit non-production
bypass: add a clearly named feature/flag (e.g., allow_c2c4_bypass or
debug_allow_commitment_bypass) checked when registering these links, emit strong
warning logs including reason and an expiration timestamp/issue ID whenever the
bypass is used, and ensure the flag is only enabled in dev/test builds or via a
controlled config so production default keeps the links active.
In `@templates/default/enclave.config.yaml`:
- Around line 7-22: slashing_manager.deploy_block is out of sync with the
recorded deployment metadata (SlashingManager.blockNumber); update the value of
slashing_manager.deploy_block from 13 to 11 so it matches
SlashingManager.blockNumber in deployed_contracts.json and keeps per-contract
start-block logic consistent.
In `@templates/default/server/index.ts`:
- Line 293: The current express.json() middleware limit is too low and can
reject valid webhook payloads before handleWebhookRequest runs; update the JSON
body parser configuration used in app.use(express.json(...)) to a larger limit
(e.g., increase to at least "20mb" or an appropriate value for prod serialized
proof+ciphertext sizes) so the webhook POSTs carrying hex-encoded proof and
ciphertext aren't blocked, and ensure this change is made where
app.use(express.json(...)) is registered so handleWebhookRequest receives the
full body.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf56520e-2ff2-43e2-b810-ca491ceba81a
📒 Files selected for processing (10)
crates/dashboard/src/dashboard.htmlcrates/keyshare/src/decryption_key_shared_collector.rscrates/keyshare/src/encryption_key_collector.rscrates/keyshare/src/threshold_share_collector.rscrates/net/src/net_interface.rscrates/program-server/src/lib.rscrates/zk-prover/src/actors/commitment_links/mod.rstemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/server/index.ts
✅ Files skipped from review due to trivial changes (1)
- crates/keyshare/src/decryption_key_shared_collector.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
templates/default/tests/integration.spec.ts (1)
174-174:⚠️ Potential issue | 🟠 MajorUse the insecure preset index here to match
paramSet: 0.Line 174 currently selects
ThresholdBfvParamsPresetNames[1](secure), but Line 207 requestsparamSet: 0(insecure). This preset mismatch can break the integration flow.🔧 Suggested fix
- thresholdBfvParamsPresetName: ThresholdBfvParamsPresetNames[1], + thresholdBfvParamsPresetName: ThresholdBfvParamsPresetNames[0],#!/bin/bash set -euo pipefail echo "== Check preset selection and paramSet pairing in integration test ==" rg -n -C2 --type ts 'thresholdBfvParamsPresetName|paramSet:' templates/default/tests/integration.spec.ts echo echo "== Check SDK default threshold preset ==" rg -n -C2 --type ts 'DEFAULT_THRESHOLD_BFV_PARAMS_PRESET_NAME' packages/enclave-sdk/src/constants.tsBased on learnings, tests in this repository intentionally remain on insecure BFV presets during the transition and should not mix in production presets yet.
Also applies to: 207-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/tests/integration.spec.ts` at line 174, The test uses a secure BFV preset but requests paramSet: 0 (insecure), causing a mismatch; change the selected preset so thresholdBfvParamsPresetName uses the insecure entry ThresholdBfvParamsPresetNames[0] to match paramSet: 0 and the repository's test policy. Update the value referenced by thresholdBfvParamsPresetName (not paramSet) so both the preset and paramSet align (ensure ThresholdBfvParamsPresetNames[0] is used).
🧹 Nitpick comments (1)
packages/enclave-contracts/scripts/deployEnclave.ts (1)
270-271: Replace magic param-set indexes with named constants.Using raw
0/1works, but named constants would make accidental mapping drift less likely.♻️ Small readability refactor
+const PARAM_SET = { + Insecure512: 0, + Secure8192: 1, +} as const; + // Register BFV param sets console.log("Registering BFV param sets..."); -await enclave.setParamSet(0, encodedInsecure); // ParamSet.Insecure512 -await enclave.setParamSet(1, encodedSecure); // ParamSet.Secure8192 +await enclave.setParamSet(PARAM_SET.Insecure512, encodedInsecure); +await enclave.setParamSet(PARAM_SET.Secure8192, encodedSecure); console.log("ParamSet.Insecure512 registered"); console.log("ParamSet.Secure8192 registered");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/scripts/deployEnclave.ts` around lines 270 - 271, Replace the magic indices passed to enclave.setParamSet with named constants to avoid mapping drift: define or use a ParamSet enum/const (e.g., ParamSet.Insecure512 and ParamSet.Secure8192) and call enclave.setParamSet(ParamSet.Insecure512, encodedInsecure) and enclave.setParamSet(ParamSet.Secure8192, encodedSecure) instead of using 0 and 1; update any nearby references in deployEnclave.ts to import or declare the ParamSet identifiers so the calls are self-documenting and resilient to index changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/dashboard/src/dashboard.html`:
- Around line 470-473: The rendering code treats numeric IDs of 0 as missing
because it uses a falsy check on eid and on eid.chain_id; update the logic that
returns '-' to only treat null/undefined (e.g., use eid == null or eid === null
|| eid === undefined) and when composing the returned string in the block that
checks typeof eid === 'object' && eid.id !== undefined, include chain_id even
when it is 0 by explicitly checking chain_id !== undefined (not truthiness)
before prefixing chain_id + ':' to eid.id so zero values are preserved.
- Line 358: The default client-side fetch for events is using a high static
limit (const text = await api('/api/events?since=' + s + '&limit=500')) which
causes heavy periodic loads; change the hardcoded limit to a much smaller
default (e.g., 50 or 100) in that api call and/or make it configurable via a
nearby refresh/auto-refresh setting, and where possible add a server-side clamp
to enforce a maximum; update the string passed to api('/api/events?...') in the
same place so the default periodic reloads use the reduced limit.
---
Duplicate comments:
In `@templates/default/tests/integration.spec.ts`:
- Line 174: The test uses a secure BFV preset but requests paramSet: 0
(insecure), causing a mismatch; change the selected preset so
thresholdBfvParamsPresetName uses the insecure entry
ThresholdBfvParamsPresetNames[0] to match paramSet: 0 and the repository's test
policy. Update the value referenced by thresholdBfvParamsPresetName (not
paramSet) so both the preset and paramSet align (ensure
ThresholdBfvParamsPresetNames[0] is used).
---
Nitpick comments:
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 270-271: Replace the magic indices passed to enclave.setParamSet
with named constants to avoid mapping drift: define or use a ParamSet enum/const
(e.g., ParamSet.Insecure512 and ParamSet.Secure8192) and call
enclave.setParamSet(ParamSet.Insecure512, encodedInsecure) and
enclave.setParamSet(ParamSet.Secure8192, encodedSecure) instead of using 0 and
1; update any nearby references in deployEnclave.ts to import or declare the
ParamSet identifiers so the calls are self-documenting and resilient to index
changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6f5bd95-3e3f-4507-be78-7b4af85739ff
📒 Files selected for processing (6)
crates/dashboard/src/dashboard.htmlcrates/net/src/net_interface.rspackages/enclave-contracts/scripts/deployEnclave.tstemplates/default/enclave.config.yamltemplates/default/server/index.tstemplates/default/tests/integration.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- templates/default/enclave.config.yaml
- templates/default/server/index.ts
- crates/net/src/net_interface.rs
ctrlc03
force-pushed
the
chore/prod-params
branch
from
043f017 to
d5fba10
Compare
ctrlc03
force-pushed
the
chore/prod-params
branch
from
d5fba10 to
761914a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/enclave-contracts/scripts/deployEnclave.ts (2)
24-67: Single-source these BFV preset values.This adds a second copy of the authoritative BFV constants inside the deploy script. A later change in
crates/fhe-paramscan silently desync deploy-time registry data from the SDK/WASM expectations. Consider generating this object from a shared artifact or codegen step instead of maintaining the values here by hand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/scripts/deployEnclave.ts` around lines 24 - 67, The BFV_PARAMS object in this script duplicates the authoritative constants from crates/fhe-params and risks desync; replace the hardcoded BFV_PARAMS with a single-source import (e.g., require/import a generated JSON or JS module produced by crates/fhe-params codegen) and update encodeBfvParams to consume that imported structure (keeping the same shape: degree, plaintextModulus, moduli, error1Variance) so ABI encoding remains unchanged; add a small build/codegen step that exports the constants from crates/fhe-params into a shared artifact consumed by this script and update any references to BFV_PARAMS to use that imported artifact.
270-271: Consider updating template/example client code to use secure parameters, or clearly document the transition period.Multiple test files and template code (e.g.,
templates/default/client/src/pages/steps/RequestComputation.tsx:110) hardcodeparamSet: 0(Insecure512). While tests intentionally remain on insecure BFV presets during the transition period, template code that developers copy may inadvertently propagate insecure defaults into production deployments. Either update template examples to useparamSet: 1(Secure8192) or add prominent warnings that the transition period is temporary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/scripts/deployEnclave.ts` around lines 270 - 271, Update template/example client usage that currently hardcodes paramSet: 0 (ParamSet.Insecure512) to use paramSet: 1 (ParamSet.Secure8192) or add a prominent deprecation/security warning in the template; locate instances where code sets paramSet: 0 (e.g., the RequestComputation.tsx example and any test stubs) and change them to paramSet: 1 or insert a clear comment/README note explaining this is a temporary transition and insecure presets should not be used in production, and ensure any server-side deployment examples calling enclave.setParamSet(0, ...) are updated to enclave.setParamSet(1, ...) or documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/CRISP/packages/crisp-contracts/deployed_contracts.json`:
- Around line 304-310: The manifest records identical blockNumber because the
code calls ethers.provider.getBlockNumber() after sequential deployments;
replace that with the actual mined block number from each deployment receipt
(use tx.wait() and read receipt.blockNumber) and pass that value into
storeDeploymentArgs(...) for both HonkVerifier and CRISPProgram so each stored
deployment uses the receipt.blockNumber rather than a later
provider.getBlockNumber() call.
---
Nitpick comments:
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 24-67: The BFV_PARAMS object in this script duplicates the
authoritative constants from crates/fhe-params and risks desync; replace the
hardcoded BFV_PARAMS with a single-source import (e.g., require/import a
generated JSON or JS module produced by crates/fhe-params codegen) and update
encodeBfvParams to consume that imported structure (keeping the same shape:
degree, plaintextModulus, moduli, error1Variance) so ABI encoding remains
unchanged; add a small build/codegen step that exports the constants from
crates/fhe-params into a shared artifact consumed by this script and update any
references to BFV_PARAMS to use that imported artifact.
- Around line 270-271: Update template/example client usage that currently
hardcodes paramSet: 0 (ParamSet.Insecure512) to use paramSet: 1
(ParamSet.Secure8192) or add a prominent deprecation/security warning in the
template; locate instances where code sets paramSet: 0 (e.g., the
RequestComputation.tsx example and any test stubs) and change them to paramSet:
1 or insert a clear comment/README note explaining this is a temporary
transition and insecure presets should not be used in production, and ensure any
server-side deployment examples calling enclave.setParamSet(0, ...) are updated
to enclave.setParamSet(1, ...) or documented.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33f6245d-618b-4b7a-942c-f14425269889
📒 Files selected for processing (8)
crates/dashboard/src/dashboard.htmlcrates/net/src/net_interface.rsexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonpackages/enclave-contracts/scripts/deployEnclave.tstemplates/default/enclave.config.yamltemplates/default/server/index.tstemplates/default/tests/integration.spec.ts
✅ Files skipped from review due to trivial changes (1)
- examples/CRISP/enclave.config.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- templates/default/server/index.ts
- templates/default/enclave.config.yaml
- crates/dashboard/src/dashboard.html
- templates/default/tests/integration.spec.ts
cedoor
reviewed
Apr 10, 2026
ctrlc03
force-pushed
the
chore/prod-params
branch
from
0d2fe01 to
10406a2
Compare
0xjei
approved these changes
Apr 13, 2026
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
3 participants
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
Tests
Chores
Behavioral Changes