refactor: accept params from contracts [skip-line-limit]#1499
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ctrlc03
changed the title
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:
📝 WalkthroughWalkthroughThis PR refactors BFV encryption parameter management across the system. Instead of storing pre-encoded parameter bytes or passing preset objects to constructors, the system now uses numeric param-set identifiers that are resolved from centralized metadata. Extensions now derive parameters from Changes
Sequence DiagramsequenceDiagram
participant EC as Enclave Contract
participant EVM as EVM Reader
participant Meta as E3Meta
participant Event as CiphernodeSelected Event
participant Ext as Extensions<br/>(Aggregator/Keyshare)
Note over EC,Ext: Parameter Set Registration & Flow
EC->>EC: setParamSet(0, encodedParams)
Note over EC: Store in paramSetRegistry[0]
EC->>EVM: E3Requested event<br/>with paramSet=0
EVM->>EVM: Convert paramSet to BfvPreset<br/>via from_on_chain_param_set()
EVM->>Meta: Create E3Meta<br/>with params_preset field
Meta->>Meta: Register as META_KEY<br/>dependency
EC->>Event: Emit CiphernodeSelected<br/>with params_preset from E3Meta
Event->>Ext: on_event triggered
Ext->>Meta: Lookup META_KEY dependency
Ext->>Ext: Derive share_enc_preset from<br/>params_preset.dkg_counterpart()
Ext->>Ext: Build aggregator params<br/>from preset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
ctrlc03
commented
Mar 30, 2026
![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: 3
🧹 Nitpick comments (7)
packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
66-82: Remove deadtrueliteral from reuse condition.The trailing
trueon line 72 is dead code that always evaluates to true and serves no purpose. It appears to be a remnant from removing theareArraysEqual(preDeployedArgs.constructorArgs.params, params)comparison.Additionally, the
timeoutConfigis accepted as a parameter but not compared in the reuse guard, which could lead to silent proxy reuse with mismatched timeout configuration.🧹 Proposed cleanup
preDeployedArgs?.constructorArgs?.bondingRegistry === bondingRegistry && preDeployedArgs?.constructorArgs?.e3RefundManager === e3RefundManager && - preDeployedArgs?.constructorArgs?.feeToken === feeToken && - true) + preDeployedArgs?.constructorArgs?.feeToken === feeToken)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/scripts/deployAndSave/enclave.ts` around lines 66 - 82, Remove the dead "&& true" from the reuse guard and ensure timeoutConfig is also compared before reusing the pre-deployed contract: update the conditional that inspects preDeployedArgs.constructorArgs (used with EnclaveFactory.connect and signer) to explicitly compare preDeployedArgs.constructorArgs.timeoutConfig with the incoming timeoutConfig (use the same deep-equality method you use elsewhere, e.g., JSON.stringify or an existing equality helper), and if params were intended to be checked restore the areArraysEqual(preDeployedArgs.constructorArgs.params, params) comparison instead of the removed fragment; keep the existing preDeployedArgs.address existence check and the subsequent EnclaveFactory.connect flow.packages/enclave-contracts/tasks/enclave.ts (1)
47-52: Thee3ParamsCLI option appears to be dead code.The
e3Paramsoption is defined but no longer used after the migration toparamSet. Consider either:
- Removing it if no longer needed
- Using it to select the
paramSetvalue (e.g., mapping "insecure" → 0, "secure" → 1)Option 1: Remove dead option
- .addOption({ - name: "e3Params", - description: "parameters for the E3 program", - defaultValue: ZeroAddress, - type: ArgumentType.STRING, - })Option 2: Wire to paramSet selection
+ .addOption({ + name: "paramSet", + description: "param set index (0=Insecure512, 1=Secure8192)", + defaultValue: 0, + type: ArgumentType.INT, + }) - .addOption({ - name: "e3Params", - description: "parameters for the E3 program", - defaultValue: ZeroAddress, - type: ArgumentType.STRING, - })Then use
paramSetfrom the options instead of hardcoding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/tasks/enclave.ts` around lines 47 - 52, The CLI option "e3Params" is dead code: either remove the .addOption({ name: "e3Params", ... }) entry or wire it to the existing paramSet logic; to fix, delete the e3Params option and any references to it if you choose removal, or map e3Params values to the paramSet numeric value (e.g., "insecure" → 0, "secure" → 1) and set options.paramSet from the parsed e3Params before the code that reads paramSet; locate the .addOption call for "e3Params" and the code that reads paramSet (the task options/paramSet usage) to apply the change.templates/default/client/src/pages/steps/RequestComputation.tsx (1)
14-14: Unused import:encodeBfvParamsSimilar to the integration test,
encodeBfvParamsis no longer used after migrating to theparamSetapproach.import { - encodeBfvParams, encodeComputeProviderParams, DEFAULT_COMPUTE_PROVIDER_PARAMS, DEFAULT_E3_CONFIG, calculateInputWindow, } from '@enclave-e3/sdk'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/client/src/pages/steps/RequestComputation.tsx` at line 14, The import encodeBfvParams is unused in RequestComputation.tsx; remove encodeBfvParams from the import list (where it's imported alongside other symbols) and delete any leftover references to encodeBfvParams in the file so the module uses the new paramSet approach consistently; ensure the import line only includes currently used symbols.packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (1)
254-268: Dead code:encodedE3ProgramParamscomputation inmakeRequestis unusedThe local encoding of BFV params (lines 254-261) is no longer used since the request now uses
paramSet: 0to reference pre-registered parameters. This dead code should be removed.Suggested cleanup
async function makeRequest( enclave: any, usdcToken: any, mockE3Program: any, mockDecryptionVerifier: any, signer?: Signer, ) { const abiCoder = ethers.AbiCoder.defaultAbiCoder(); - const polynomial_degree = ethers.toBigInt(2048); - const plaintext_modulus = ethers.toBigInt(1032193); - const moduli = [ethers.toBigInt("18014398492704769")]; - const encodedE3ProgramParams = abiCoder.encode( - ["uint256", "uint256", "uint256[]"], - [polynomial_degree, plaintext_modulus, moduli], - ); const currentTime = await networkHelpers.time.latest(); const requestParams = { committeeSize: 0, inputWindow: [currentTime + 100, currentTime + 300] as [number, number], e3Program: await mockE3Program.getAddress(), paramSet: 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts` around lines 254 - 268, Remove the unused BFV encoding setup: delete the local abiCoder, polynomial_degree, plaintext_modulus, moduli and the computed encodedE3ProgramParams that are defined above the request creation in the test (they are dead because the request uses paramSet: 0); keep the rest of makeRequest and requestParams intact so the test references pre-registered parameters instead of computing encodedE3ProgramParams locally.templates/default/tests/integration.spec.ts (1)
11-11: Unused import:encodeBfvParamsThe
encodeBfvParamsimport is no longer used after migrating to theparamSetapproach. Consider removing it to keep imports clean.import { EnclaveSDK, calculateInputWindow, DEFAULT_COMPUTE_PROVIDER_PARAMS, - encodeBfvParams, encodeComputeProviderParams, decodePlaintextOutput, CommitteeSize, } from '@enclave-e3/sdk'🤖 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 11, Remove the unused import encodeBfvParams from the imports in templates/default/tests/integration.spec.ts; locate the import list where encodeBfvParams is referenced and delete that symbol so only used helpers (e.g., paramSet) remain, then run the test suite or linter to confirm there are no remaining references to encodeBfvParams.templates/default/server/index.ts (1)
70-92: Consider using the official ABI instead of inline definition.The inline ABI for
paramSetRegistry(lines 76-84) works correctly, but the SDK already importsEnclave__factoryfrom@enclave-e3/contracts/typeswhich contains the official ABI. Using the official ABI would ensure consistency if the contract interface changes.However, this is a minor concern since the inline ABI matches the contract definition, and the template may intentionally minimize dependencies.
♻️ Optional: Use official ABI from contract types
import { Enclave__factory } from '@enclave-e3/contracts/types' // Then in runProgram: const e3ProgramParams = (await sdk.sdk.getPublicClient().readContract({ address: sdk.sdk.getContractAddresses().enclave, abi: Enclave__factory.abi, functionName: 'paramSetRegistry', args: [paramSetId], })) as string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/server/index.ts` around lines 70 - 92, Replace the inline ABI for paramSetRegistry with the official contract ABI to ensure consistency: import Enclave__factory from '@enclave-e3/contracts/types' (or the named export Enclave__factory) and use Enclave__factory.abi in the readContract call (the code around createPrivateSDK(), e3Details.paramSet, and the readContract invocation for paramSetRegistry), leaving the rest of the logic (ciphertextInputs and callFheRunner) unchanged.packages/enclave-contracts/contracts/Enclave.sol (1)
654-664: Consider adding ability to clear/unregister a param set.The current implementation allows setting/updating param sets but doesn't provide a way to remove/disable one. If a param set needs to be deprecated (e.g., security issue discovered), there's no mechanism to prevent its use in new requests other than deploying an upgrade.
This is a minor operational consideration - you could add a function to set the registry entry to empty bytes, or this could be deferred if the upgrade path is acceptable.
Optional: Add clearParamSet function
/// `@notice` Clears a previously registered param set, preventing its use in new requests. /// `@param` paramSet The ParamSet variant to clear. function clearParamSet(ParamSet paramSet) public onlyOwner { require(paramSetRegistry[paramSet].length > 0, "Param set not registered"); delete paramSetRegistry[paramSet]; // Consider emitting an event like ParamSetCleared(paramSet) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/Enclave.sol` around lines 654 - 664, Add a way to unregister/deprecate param sets by introducing a clearParamSet(ParamSet paramSet) function that is onlyOwner, verifies the entry exists (paramSetRegistry[paramSet].length > 0), deletes the entry (delete paramSetRegistry[paramSet]) and emits a new ParamSetCleared(paramSet) event; update any docs/tests that assume immutability and ensure ParamSetRegistered remains for setParamSet while the new clear operation prevents future uses of that registry entry.
🤖 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/aggregator/src/ext.rs`:
- Around line 104-107: PublicKeyAggregatorExtension::hydrate and
ThresholdPlaintextAggregatorExtension::hydrate use
ctx.get_dependency(META_KEY).map(|meta| meta.params_preset).unwrap_or_default(),
which yields BfvPreset::SecureThreshold8192 and mismatches keyshare's
BfvPreset::InsecureThreshold512; change the fallback to use the same explicit
preset as keyshare (e.g., .unwrap_or(BfvPreset::InsecureThreshold512)) or
otherwise read the keyshare fallback helper so both extensions use the identical
default; update the calls referencing META_KEY and params_preset accordingly so
the default preset aligns with the keyshare extension.
In `@crates/keyshare/src/ext.rs`:
- Around line 104-108: The preset fallback in hydrate uses an explicit
BfvPreset::InsecureThreshold512 which mismatches aggregator's
unwrap_or_default(); change the fallback for share_enc_preset by replacing the
current unwrap_or(BfvPreset::InsecureThreshold512) with unwrap_or_default() so
that the expression derived from ctx.get_dependency(META_KEY).and_then(|meta|
meta.params_preset.dkg_counterpart()) uses the crate's default BfvPreset
(matching the aggregator extension) and prevents rehydration preset
inconsistencies.
In `@examples/CRISP/packages/crisp-contracts/deployed_contracts.json`:
- Around line 130-135: The JSON contains a spurious "undefined" network section
with the contract entry "PoseidonT3" which is likely an artifact of the
deployment tool; remove the entire "undefined" object from
deployed_contracts.json (or replace it with the correct network key) and, if
this recurs, fix the deployment script to ensure it supplies a valid network
name when writing the deployed_contracts.json file (search for logic that writes
the network key and the code that references "PoseidonT3" to locate the writer).
---
Nitpick comments:
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 654-664: Add a way to unregister/deprecate param sets by
introducing a clearParamSet(ParamSet paramSet) function that is onlyOwner,
verifies the entry exists (paramSetRegistry[paramSet].length > 0), deletes the
entry (delete paramSetRegistry[paramSet]) and emits a new
ParamSetCleared(paramSet) event; update any docs/tests that assume immutability
and ensure ParamSetRegistered remains for setParamSet while the new clear
operation prevents future uses of that registry entry.
In `@packages/enclave-contracts/scripts/deployAndSave/enclave.ts`:
- Around line 66-82: Remove the dead "&& true" from the reuse guard and ensure
timeoutConfig is also compared before reusing the pre-deployed contract: update
the conditional that inspects preDeployedArgs.constructorArgs (used with
EnclaveFactory.connect and signer) to explicitly compare
preDeployedArgs.constructorArgs.timeoutConfig with the incoming timeoutConfig
(use the same deep-equality method you use elsewhere, e.g., JSON.stringify or an
existing equality helper), and if params were intended to be checked restore the
areArraysEqual(preDeployedArgs.constructorArgs.params, params) comparison
instead of the removed fragment; keep the existing preDeployedArgs.address
existence check and the subsequent EnclaveFactory.connect flow.
In `@packages/enclave-contracts/tasks/enclave.ts`:
- Around line 47-52: The CLI option "e3Params" is dead code: either remove the
.addOption({ name: "e3Params", ... }) entry or wire it to the existing paramSet
logic; to fix, delete the e3Params option and any references to it if you choose
removal, or map e3Params values to the paramSet numeric value (e.g., "insecure"
→ 0, "secure" → 1) and set options.paramSet from the parsed e3Params before the
code that reads paramSet; locate the .addOption call for "e3Params" and the code
that reads paramSet (the task options/paramSet usage) to apply the change.
In `@packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts`:
- Around line 254-268: Remove the unused BFV encoding setup: delete the local
abiCoder, polynomial_degree, plaintext_modulus, moduli and the computed
encodedE3ProgramParams that are defined above the request creation in the test
(they are dead because the request uses paramSet: 0); keep the rest of
makeRequest and requestParams intact so the test references pre-registered
parameters instead of computing encodedE3ProgramParams locally.
In `@templates/default/client/src/pages/steps/RequestComputation.tsx`:
- Line 14: The import encodeBfvParams is unused in RequestComputation.tsx;
remove encodeBfvParams from the import list (where it's imported alongside other
symbols) and delete any leftover references to encodeBfvParams in the file so
the module uses the new paramSet approach consistently; ensure the import line
only includes currently used symbols.
In `@templates/default/server/index.ts`:
- Around line 70-92: Replace the inline ABI for paramSetRegistry with the
official contract ABI to ensure consistency: import Enclave__factory from
'@enclave-e3/contracts/types' (or the named export Enclave__factory) and use
Enclave__factory.abi in the readContract call (the code around
createPrivateSDK(), e3Details.paramSet, and the readContract invocation for
paramSetRegistry), leaving the rest of the logic (ciphertextInputs and
callFheRunner) unchanged.
In `@templates/default/tests/integration.spec.ts`:
- Line 11: Remove the unused import encodeBfvParams from the imports in
templates/default/tests/integration.spec.ts; locate the import list where
encodeBfvParams is referenced and delete that symbol so only used helpers (e.g.,
paramSet) remain, then run the test suite or linter to confirm there are no
remaining references to encodeBfvParams.
🪄 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: 0dcae474-3002-45a8-aa06-52d09dbba1ad
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
crates/aggregator/src/ext.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/events/src/enclave_event/ciphernode_selected.rscrates/events/src/enclave_event/e3_requested.rscrates/evm/src/enclave_sol_reader.rscrates/fhe-params/src/presets.rscrates/keyshare/src/ext.rscrates/request/Cargo.tomlcrates/request/src/meta.rscrates/sortition/src/ciphernode_selector.rscrates/tests/tests/integration.rsexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.solexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonexamples/CRISP/server/.env.examplepackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IE3.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/ignition/modules/enclave.tspackages/enclave-contracts/scripts/deployAndSave/enclave.tspackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Pricing/Pricing.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tspackages/enclave-sdk/src/contracts/contract-client.tspackages/enclave-sdk/src/contracts/types.tspackages/enclave-sdk/src/enclave-sdk.tspackages/enclave-sdk/src/events/types.tstemplates/default/client/src/pages/steps/RequestComputation.tsxtemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/server/index.tstemplates/default/tests/integration.spec.ts
💤 Files with no reviewable changes (1)
- packages/enclave-contracts/ignition/modules/enclave.ts
ctrlc03
force-pushed
the
refactor/param-enums
branch
from
200ff28 to
296d0c2
Compare
Contributor
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)
packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json (1)
565-572:⚠️ Potential issue | 🔴 CriticalABI mismatch:
getActiveCommitteeNodesis missing thescoresoutput in the artifact.The regenerated artifact at lines 565-572 now exposes only
nodes, butpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.sol:428andpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol:556both define the function to return(address[] memory nodes, uint256[] memory scores). All existing callers destructure both values:
packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts:578packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts:650, 676packages/enclave-contracts/tasks/enclave.ts:434Regenerate the artifact to match the contract surface, or land the source/caller updates in the same PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json` around lines 565 - 572, The artifact ABI for getActiveCommitteeNodes is missing the second return value (scores); update the artifact so getActiveCommitteeNodes in ICiphernodeRegistry.json matches the Solidity signature (address[] nodes, uint256[] scores) used in ICiphernodeRegistry and CiphernodeRegistryOwnable and expected by callers. Regenerate the contract artifact or edit the outputs array for the getActiveCommitteeNodes entry to include a second output with "internalType": "uint256[]", "name": "scores", "type": "uint256[]", then run the build/tests to ensure callers (e.g., CiphernodeRegistryOwnable.spec.ts, CommitteeExpulsion.spec.ts, enclave.ts) compile against the corrected ABI.
🧹 Nitpick comments (5)
fhe.md (1)
1-932: Consider Markdown heading hierarchy for navigability.Using
#/##headings (and optionally fenced blocks for large diagrams/tables) would improve TOC generation and skimmability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fhe.md` around lines 1 - 932, The document lacks consistent Markdown heading hierarchy which hampers TOC generation and skimmability; convert top-level title to "# Fully Homomorphic Encryption — From Zero to Hero", make each "Lecture X: ..." line a "## Lecture X: ..." heading (or "###" for sub-lectures), and ensure subordinate sections like "Why Is This Hard?", "Encoding 1 bit", etc. use descending heading levels; additionally wrap large ASCII diagrams/tables in fenced code blocks (``` or ```text) to preserve formatting and consider adding a TOC at the top using a Markdown list linking to lecture headings.packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
66-72: Remove redundant&& trueplaceholder.The
&& trueis a leftover from removing theareArraysEqual(preDeployedArgs.constructorArgs.params, params)check. It's functionally harmless but makes the condition unnecessarily verbose.♻️ Proposed cleanup
(preDeployedArgs?.constructorArgs?.owner === owner && preDeployedArgs?.constructorArgs?.maxDuration === maxDuration && preDeployedArgs?.constructorArgs?.registry === registry && preDeployedArgs?.constructorArgs?.bondingRegistry === bondingRegistry && preDeployedArgs?.constructorArgs?.e3RefundManager === e3RefundManager && - preDeployedArgs?.constructorArgs?.feeToken === feeToken && - true) + preDeployedArgs?.constructorArgs?.feeToken === feeToken)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/scripts/deployAndSave/enclave.ts` around lines 66 - 72, The boolean expression that checks preDeployedArgs.constructorArgs includes a redundant "&& true" at the end; remove the "&& true" token so the conditional simply compares preDeployedArgs?.constructorArgs?.owner, .maxDuration, .registry, .bondingRegistry, .e3RefundManager, and .feeToken against owner, maxDuration, registry, bondingRegistry, e3RefundManager, and feeToken respectively (i.e., update the condition in the block using preDeployedArgs and constructorArgs to drop the trailing && true).packages/enclave-contracts/tasks/enclave.ts (1)
106-111: Thee3ParamsCLI option appears unused after this change.The
e3Paramsoption is still defined (lines 106-111) and destructured in the action (line 137), but it's no longer used sinceparamSetreplacese3ProgramParams. Consider removing the dead option or repurposing it to let users specify aparamSetvalue.♻️ Suggested change: Remove or repurpose the unused option
Option 1 - Remove the dead option:
- .addOption({ - name: "e3Params", - description: "parameters for the E3 program", - defaultValue: ZeroAddress, - type: ArgumentType.STRING, - })Option 2 - Repurpose as paramSet selector:
.addOption({ - name: "e3Params", - description: "parameters for the E3 program", - defaultValue: ZeroAddress, - type: ArgumentType.STRING, + name: "paramSet", + description: "BFV param set (0=Insecure512, 1=Secure8192)", + defaultValue: 0, + type: ArgumentType.INT, })Also applies to: 228-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/tasks/enclave.ts` around lines 106 - 111, The CLI option e3Params is now dead: it’s defined in the command options and destructured in the action but no longer used after switching to paramSet/e3ProgramParams; either remove the e3Params option and its destructuring usage entirely (clean up its definition and any references), or repurpose it to map to paramSet by using e3Params as an alias for paramSet (update the option name/description and the action to set paramSet = e3Params when provided). Locate references to e3Params, paramSet, and e3ProgramParams in the command definition and action handler and make the corresponding removal or aliasing change consistently.templates/default/server/index.ts (1)
70-87: Consider importing ABI from SDK to avoid divergence.The inline ABI for
paramSetRegistryduplicates the contract definition. If the contract interface changes, this hardcoded ABI could become stale. The SDK'sEnclave__factory.abialready contains this function signature.Additionally, there's no error handling if
paramSetRegistryreturns empty bytes for an unregisteredparamSet. Consider adding a guard:if (!e3ProgramParams || e3ProgramParams === '0x') { console.error(`No params registered for paramSet ${paramSetId}`) session.isProcessing = false return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/server/index.ts` around lines 70 - 87, The inline ABI for paramSetRegistry should be replaced with the SDK-provided ABI to avoid duplication and drift: use the Enclave__factory.abi from the SDK when calling sdk.sdk.getPublicClient().readContract (keep the call site around sdk.getE3 and the variable e3ProgramParams), and add a guard after the read to handle empty results—if e3ProgramParams is falsy or equals '0x' log an error like "No params registered for paramSet <paramSetId>", set session.isProcessing = false and return to stop processing. Ensure you reference createPrivateSDK, sdk.getE3, paramSetRegistry (via Enclave__factory.abi), e3ProgramParams and session.isProcessing when making the changes.tests/integration/restart.sh (1)
18-18: Guarantee cleanup on failure with an EXIT trap.With
set -e, failures before Line 234 skip shutdown and can leave processes running. Add an EXIT trap to make cleanup deterministic.Suggested fix
set -eu THIS_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" source "$THIS_DIR/fns.sh" source "$THIS_DIR/lib/utils.sh" +trap 'gracefull_shutdown' EXIT @@ -gracefull_shutdown +trap - EXIT +gracefull_shutdownAlso applies to: 234-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/restart.sh` at line 18, The script currently uses set -eu which can exit early and skip teardown; add an EXIT trap immediately after set -eu that always runs your cleanup routine so processes are stopped on any failure. Implement or reuse a single handler (e.g., on_exit or cleanup / shutdown_all) that performs the existing shutdown logic and register it with trap 'on_exit EXIT' (or equivalent) so cleanup runs on normal exit, error, or interrupt; ensure the handler is idempotent so repeated calls are safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fhe.md`:
- Around line 81-82: The example for fixed-point encoding is truncated; complete
the second case by showing the multiplication and stored integer: for k = 8
bits, t = 0.75 → 0.75 × 256 = 192 → stored as 192 (optionally note rounding
behavior if fractional result occurs). Update the line containing the examples
of t to include the full calculation and final stored value for t = 0.75.
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 85-86: Update the devdoc on the enum/params comment to reflect the
new registration flow: replace the reference to the old initialize() with the
current setter setParamSet() and clarify that Ciphernodes decode the enum
directly while the encoded bytes are stored on-chain, so the comment should
mention setParamSet() as the population point and remove any implication that
initialize() performs this action; update the text near the enum/params
declaration in Enclave.sol (where initialize() is currently referenced) to
reference setParamSet() and the on-chain transparency of the encoded bytes.
- Around line 658-664: The setParamSet function currently overwrites existing
entries in paramSetRegistry[paramSet], allowing historical ParamSet enum values
to change; modify setParamSet (and the ParamSet registry logic) to prevent
overwrites by rejecting calls when a paramSet is already registered (e.g.,
require(paramSetRegistry[paramSet].length == 0, "ParamSet already set")) or
alternatively implement explicit versionedParamSets (e.g., mapping(ParamSet =>
mapping(uint256 => bytes)) plus a registerVersion method) so registrations are
immutable after first set; update emit behavior accordingly and keep the
function names paramSetRegistry and setParamSet as the touch points.
In `@tests/integration/restart.sh`:
- Around line 130-138: Before running Scenario 1, remove any stale output files
so waiton("$SCRIPT_DIR/output/pubkey.bin") cannot immediately succeed; modify
the script around enclave_nodes_up/request_committee/waiton to delete or
truncate "$SCRIPT_DIR/output/pubkey.bin" (and any other leftover artifacts
produced by previous runs) before invoking request_committee and waiton,
ensuring waiton only returns after the fresh publish event.
- Around line 50-61: The script uses unquoted variable expansions (e.g.,
ENCODED_PARAMS assignment invoking $SCRIPT_DIR/lib/pack_e3_params.sh and the
CIPHERNODE_ADDRESS_* variables used in pnpm ciphernode:add) which can cause
word-splitting/globbing; fix by quoting all expansions: wrap "$SCRIPT_DIR",
"$ENCODED_PARAMS" (where used), and each ciphernode address variable
("$CIPHERNODE_ADDRESS_1" ... "$CIPHERNODE_ADDRESS_5") in double quotes and also
quote any other unquoted expansions noted (lines referenced in the comment) so
commands and parameter substitutions are treated as single arguments. Ensure the
pack_e3_params.sh call uses "$SCRIPT_DIR/lib/pack_e3_params.sh" and assign
ENCODED_PARAMS using a quoted command substitution if applicable.
---
Outside diff comments:
In
`@packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json`:
- Around line 565-572: The artifact ABI for getActiveCommitteeNodes is missing
the second return value (scores); update the artifact so getActiveCommitteeNodes
in ICiphernodeRegistry.json matches the Solidity signature (address[] nodes,
uint256[] scores) used in ICiphernodeRegistry and CiphernodeRegistryOwnable and
expected by callers. Regenerate the contract artifact or edit the outputs array
for the getActiveCommitteeNodes entry to include a second output with
"internalType": "uint256[]", "name": "scores", "type": "uint256[]", then run the
build/tests to ensure callers (e.g., CiphernodeRegistryOwnable.spec.ts,
CommitteeExpulsion.spec.ts, enclave.ts) compile against the corrected ABI.
---
Nitpick comments:
In `@fhe.md`:
- Around line 1-932: The document lacks consistent Markdown heading hierarchy
which hampers TOC generation and skimmability; convert top-level title to "#
Fully Homomorphic Encryption — From Zero to Hero", make each "Lecture X: ..."
line a "## Lecture X: ..." heading (or "###" for sub-lectures), and ensure
subordinate sections like "Why Is This Hard?", "Encoding 1 bit", etc. use
descending heading levels; additionally wrap large ASCII diagrams/tables in
fenced code blocks (``` or ```text) to preserve formatting and consider adding a
TOC at the top using a Markdown list linking to lecture headings.
In `@packages/enclave-contracts/scripts/deployAndSave/enclave.ts`:
- Around line 66-72: The boolean expression that checks
preDeployedArgs.constructorArgs includes a redundant "&& true" at the end;
remove the "&& true" token so the conditional simply compares
preDeployedArgs?.constructorArgs?.owner, .maxDuration, .registry,
.bondingRegistry, .e3RefundManager, and .feeToken against owner, maxDuration,
registry, bondingRegistry, e3RefundManager, and feeToken respectively (i.e.,
update the condition in the block using preDeployedArgs and constructorArgs to
drop the trailing && true).
In `@packages/enclave-contracts/tasks/enclave.ts`:
- Around line 106-111: The CLI option e3Params is now dead: it’s defined in the
command options and destructured in the action but no longer used after
switching to paramSet/e3ProgramParams; either remove the e3Params option and its
destructuring usage entirely (clean up its definition and any references), or
repurpose it to map to paramSet by using e3Params as an alias for paramSet
(update the option name/description and the action to set paramSet = e3Params
when provided). Locate references to e3Params, paramSet, and e3ProgramParams in
the command definition and action handler and make the corresponding removal or
aliasing change consistently.
In `@templates/default/server/index.ts`:
- Around line 70-87: The inline ABI for paramSetRegistry should be replaced with
the SDK-provided ABI to avoid duplication and drift: use the
Enclave__factory.abi from the SDK when calling
sdk.sdk.getPublicClient().readContract (keep the call site around sdk.getE3 and
the variable e3ProgramParams), and add a guard after the read to handle empty
results—if e3ProgramParams is falsy or equals '0x' log an error like "No params
registered for paramSet <paramSetId>", set session.isProcessing = false and
return to stop processing. Ensure you reference createPrivateSDK, sdk.getE3,
paramSetRegistry (via Enclave__factory.abi), e3ProgramParams and
session.isProcessing when making the changes.
In `@tests/integration/restart.sh`:
- Line 18: The script currently uses set -eu which can exit early and skip
teardown; add an EXIT trap immediately after set -eu that always runs your
cleanup routine so processes are stopped on any failure. Implement or reuse a
single handler (e.g., on_exit or cleanup / shutdown_all) that performs the
existing shutdown logic and register it with trap 'on_exit EXIT' (or equivalent)
so cleanup runs on normal exit, error, or interrupt; ensure the handler is
idempotent so repeated calls are safe.
🪄 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: f55ad455-adaf-4378-8047-154b15343772
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
agent/flow-trace/07_CIPHERNODE_E3_DEEP_DIVE.mdcrates/aggregator/src/ext.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/events/src/enclave_event/ciphernode_selected.rscrates/events/src/enclave_event/e3_requested.rscrates/evm/src/enclave_sol_reader.rscrates/fhe-params/src/presets.rscrates/keyshare/src/ext.rscrates/request/Cargo.tomlcrates/request/src/meta.rscrates/sortition/src/ciphernode_selector.rscrates/tests/tests/integration.rsexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.solexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonexamples/CRISP/server/.env.examplefhe.mdlogs.txtlogs_c0c3_debug.txtpackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IE3.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/ignition/modules/enclave.tspackages/enclave-contracts/scripts/deployAndSave/enclave.tspackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Pricing/Pricing.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tspackages/enclave-sdk/src/contracts/contract-client.tspackages/enclave-sdk/src/contracts/types.tspackages/enclave-sdk/src/enclave-sdk.tspackages/enclave-sdk/src/events/types.tstemplates/default/client/src/pages/steps/RequestComputation.tsxtemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/logs.txttemplates/default/server/index.tstemplates/default/tests/integration.spec.tstests/integration/restart.sh
💤 Files with no reviewable changes (1)
- packages/enclave-contracts/ignition/modules/enclave.ts
✅ Files skipped from review due to trivial changes (9)
- crates/request/Cargo.toml
- templates/default/enclave.config.yaml
- examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol
- packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json
- packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json
- templates/default/tests/integration.spec.ts
- packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts
- packages/enclave-contracts/test/Pricing/Pricing.spec.ts
- packages/enclave-sdk/src/enclave-sdk.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/enclave-contracts/scripts/deployEnclave.ts
- crates/tests/tests/integration.rs
- packages/enclave-contracts/contracts/interfaces/IE3.sol
- templates/default/client/src/pages/steps/RequestComputation.tsx
- crates/events/src/enclave_event/ciphernode_selected.rs
- crates/request/src/meta.rs
- examples/CRISP/enclave.config.yaml
- crates/fhe-params/src/presets.rs
- packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts
- packages/enclave-contracts/deployed_contracts.json
- templates/default/deployed_contracts.json
- packages/enclave-sdk/src/contracts/types.ts
- crates/ciphernode-builder/src/ciphernode_builder.rs
- packages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.ts
- packages/enclave-sdk/src/events/types.ts
- crates/keyshare/src/ext.rs
- packages/enclave-contracts/test/Enclave.spec.ts
- examples/CRISP/packages/crisp-contracts/deployed_contracts.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
examples/CRISP/server/src/server/routes/rounds.rs (1)
201-202: Unused variable after refactor.The
paramsvariable is computed viaencode_bfv_params(...)at line 202 but is no longer used anywhere in the function after the migration toparam_set: u8. This appears to be dead code.Suggested removal
- info!("Generating parameters..."); - let params = encode_bfv_params(&build_bfv_params_from_set_arc(default_param_set())); - let token_address: Address = token_address.parse()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/server/src/server/routes/rounds.rs` around lines 201 - 202, Remove the dead computation of params: the call to encode_bfv_params(&build_bfv_params_from_set_arc(default_param_set())) is no longer used after migrating to param_set: u8, so delete the variable assignment (the params binding) from the function in rounds.rs; also scan for and remove any now-unused imports or helpers related to encode_bfv_params, build_bfv_params_from_set_arc, or default_param_set() in that scope to avoid unused-import warnings, and ensure no side effects are relied upon before deleting.templates/default/server/index.ts (1)
70-94: Consider caching the fetchede3ProgramParamsto avoid redundant RPC calls.The current implementation fetches
e3ProgramParamsfromparamSetRegistryon everyrunPrograminvocation. Since param sets are expected to be immutable once registered (or rarely changed), consider storing the fetched params in the session to avoid repeated RPC calls.Optional: Cache fetched params on session
+ // Check if we already have the encoded params cached + let e3ProgramParams = session.e3ProgramParams + if (!e3ProgramParams) { // Look up the encoded params from the on-chain paramSetRegistry const sdk = await createPrivateSDK() const e3Details = await sdk.getE3(e3Id) const paramSetId = e3Details.paramSet const publicClient = sdk.getPublicClient() const { ENCLAVE_CONTRACT } = getCheckedEnvVars() - const e3ProgramParams = (await publicClient.readContract({ + e3ProgramParams = (await publicClient.readContract({ address: ENCLAVE_CONTRACT as `0x${string}`, abi: [ { name: 'paramSetRegistry', type: 'function', stateMutability: 'view', inputs: [{ name: '', type: 'uint8' }], outputs: [{ name: '', type: 'bytes' }], }, ], functionName: 'paramSetRegistry', args: [paramSetId], })) as string + session.e3ProgramParams = e3ProgramParams + }And update the interface:
interface E3Session { e3Id: bigint expiration: bigint paramSet?: number e3ProgramParams?: string // cached encoded params inputs: Array<{ data: string; index: bigint }> isProcessing: boolean isCompleted: boolean }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/server/index.ts` around lines 70 - 94, The code currently always calls the on-chain paramSetRegistry to get e3ProgramParams inside runProgram; add caching by storing the fetched encoded params on the session object and reusing them if present. Update the E3Session interface to include an optional e3ProgramParams?: string (and paramSet?: number if needed), then in the run flow check session.e3ProgramParams first and only call publicClient.readContract (paramSetRegistry) when it's undefined, and after a successful fetch assign session.e3ProgramParams = e3ProgramParams before calling callFheRunner (keep using symbols e3ProgramParams, paramSetRegistry, session, E3Session, and callFheRunner to locate 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 `@packages/enclave-contracts/contracts/interfaces/IEnclave.sol`:
- Around line 30-32: Fix the truncated NatSpec by completing the interrupted
sentence and separating the two comments: finish the sentence that starts "The
DKG counterpart" to describe its relationship to the BFV encryption parameter
sets (e.g., which DKG presets correspond or how they're used), and move or
adjust the following `@notice` for E3Stage so it is not embedded inside the BFV
comment; ensure the NatSpec tags (e.g., the BFV description and the `@notice` for
E3Stage) each have a full, self-contained sentence and correct placement near
the related enum/type declarations (reference BFV encryption parameter sets and
E3Stage identifiers to locate the comments).
---
Nitpick comments:
In `@examples/CRISP/server/src/server/routes/rounds.rs`:
- Around line 201-202: Remove the dead computation of params: the call to
encode_bfv_params(&build_bfv_params_from_set_arc(default_param_set())) is no
longer used after migrating to param_set: u8, so delete the variable assignment
(the params binding) from the function in rounds.rs; also scan for and remove
any now-unused imports or helpers related to encode_bfv_params,
build_bfv_params_from_set_arc, or default_param_set() in that scope to avoid
unused-import warnings, and ensure no side effects are relied upon before
deleting.
In `@templates/default/server/index.ts`:
- Around line 70-94: The code currently always calls the on-chain
paramSetRegistry to get e3ProgramParams inside runProgram; add caching by
storing the fetched encoded params on the session object and reusing them if
present. Update the E3Session interface to include an optional e3ProgramParams?:
string (and paramSet?: number if needed), then in the run flow check
session.e3ProgramParams first and only call publicClient.readContract
(paramSetRegistry) when it's undefined, and after a successful fetch assign
session.e3ProgramParams = e3ProgramParams before calling callFheRunner (keep
using symbols e3ProgramParams, paramSetRegistry, session, E3Session, and
callFheRunner to locate 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: d27c2022-58a0-4913-9010-d5fc1097b0a5
📒 Files selected for processing (14)
crates/evm-helpers/src/contracts.rscrates/indexer/src/indexer.rsexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/routes/rounds.rspackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IE3.solpackages/enclave-contracts/contracts/interfaces/IEnclave.soltemplates/default/server/index.ts
✅ Files skipped from review due to trivial changes (2)
- packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json
- packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json
- packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json
ctrlc03
force-pushed
the
refactor/param-enums
branch
from
7300dd2 to
4416396
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
examples/CRISP/packages/crisp-contracts/deployed_contracts.json (1)
130-149:⚠️ Potential issue | 🟡 MinorRemove the stray fallback network sections.
"undefined"and"default"do not look like real deployment targets here, and the"default"block is internally inconsistent becauseMockUSDCandEnclaveTokenboth point at0x5FbDB…. Keeping these sections in the committed artifact makes downstream network resolution ambiguous and preserves deployment-writer garbage as source-of-truth.🧹 Suggested cleanup
- "undefined": { - "PoseidonT3": { - "blockNumber": 3, - "address": "0x3333333C0A88F9BE4fd23ed0536F9B6c427e3B93" - } - }, - "default": { - "MockUSDC": { - "constructorArgs": { - "initialSupply": "1000000" - }, - "blockNumber": 1, - "address": "0x5FbDB2315678afecb367f032d93F642f64180aa3" - }, - "EnclaveToken": { - "constructorArgs": { - "owner": "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266" - }, - "blockNumber": 1, - "address": "0x5FbDB2315678afecb367f032d93F642f64180aa3" - } - },Based on learnings, in Hardhat v3 deployment scripts, use
(await signer.provider?.getNetwork())?.name ?? "localhost"instead ofhre.globalOptions.networkto reliably get the network name, ashre.globalOptions.networkcan be undefined in some contexts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/packages/crisp-contracts/deployed_contracts.json` around lines 130 - 149, The committed deployed_contracts.json contains stray fallback network sections named "undefined" and "default" (see keys "undefined", "default" and contracts "MockUSDC", "EnclaveToken") that should be removed and replaced with only real network entries; update the deployment artifact by deleting the entire "undefined" and "default" blocks so addresses are unambiguous, and fix the deployment script that produces them to derive the network name reliably by using (await signer.provider?.getNetwork())?.name ?? "localhost" instead of hre.globalOptions.network so future runs don't emit an undefined fallback.
🧹 Nitpick comments (7)
fhe.md (4)
204-208: Format algorithm steps as proper lists.The ENCRYPT and DECRYPT algorithms are currently formatted as run-on text, which makes them difficult to follow for readers learning these concepts.
✏️ Proposed formatting improvement
-ENCRYPT message m(X) = 5 + 3X²: 1. Sample random mask: c₁(X) = 7 - 2X + X² + 4X³ 2. Sample small -error: e(X) = 0 + 1 - 1 + 0 (tiny!) 3. Compute body: c₀ = -c₁ · s₁ + m + e (polynomial -multiplication mod X⁴+1, then add message and error) 4. Ciphertext: ct = (c₀, c₁) - -DECRYPT (knowing s₁): 1. Compute: c₀ + c₁ · s₁ = m + e 2. Round away e → m = 5 + 3X² ✓ +**ENCRYPT** message m(X) = 5 + 3X²: + +1. Sample random mask: c₁(X) = 7 - 2X + X² + 4X³ +2. Sample small error: e(X) = 0 + 1 - 1 + 0 (tiny!) +3. Compute body: c₀ = -c₁ · s₁ + m + e (polynomial multiplication mod X⁴+1, then add message and error) +4. Ciphertext: ct = (c₀, c₁) + +**DECRYPT** (knowing s₁): + +1. Compute: c₀ + c₁ · s₁ = m + e +2. Round away e → m = 5 + 3X² ✓🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fhe.md` around lines 204 - 208, Reformat the ENCRYPT and DECRYPT sections into explicit numbered or bulleted algorithm steps instead of run-on text: break ENCRYPT into sequential steps (1. Sample random mask c₁(X) = …; 2. Sample small error e(X) = …; 3. Compute body c₀ = -c₁ · s₁ + m + e with polynomial multiplication mod X⁴+1; 4. Output ciphertext ct = (c₀, c₁)), and break DECRYPT into steps (1. Compute c₀ + c₁ · s₁ = m + e; 2. Round away e → recover m), ensuring symbols like ENCRYPT, DECRYPT, c₀, c₁, s₁, m, e, ct are preserved and punctuation/line breaks make each step clear and readable.
54-54: Format examples with line breaks for clarity.The examples are run together on a single line, making them difficult to parse.
✏️ Proposed formatting improvement
-Examples: 3.7 → 0.7 -0.3 → 0.7 (same point!) 1.0 → 0.0 0.25 → 0.25 +Examples: +- 3.7 → 0.7 +- -0.3 → 0.7 (same point!) +- 1.0 → 0.0 +- 0.25 → 0.25🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fhe.md` at line 54, Split the run‑together examples into separate lines to improve readability; in fhe.md replace the single line "Examples: 3.7 → 0.7 -0.3 → 0.7 (same point!) 1.0 → 0.0 0.25 → 0.25" with each example on its own line (e.g., "3.7 → 0.7", "-0.3 → 0.7 (same point!)", "1.0 → 0.0", "0.25 → 0.25") so each example is clearly separated and easy to parse.
632-632: Minor: Consider more concise heading.The heading "End Result" could be simplified, though this is a minor stylistic preference.
✏️ Optional alternative
-End Result +Result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fhe.md` at line 632, Replace the heading text "End Result" with a more concise alternative (e.g., "Result" or "Outcome") in the fhe.md document; locate the header by searching for the exact string "End Result" and update it to the chosen shorter heading, ensuring any internal links or references to that header are updated to match the new heading text.
17-18: Improve the flow diagram formatting.The "↑" character disrupts the left-to-right flow of the encryption pipeline. Consider restructuring this section for better readability.
✏️ Proposed formatting improvement
-You: encrypt(x) ──────► Server: f(encrypt(x)) ──────► You: decrypt(result) = f(x) ↑ Server never -sees x! +You: encrypt(x) ──────► Server: f(encrypt(x)) ──────► You: decrypt(result) = f(x) + +**Key property:** The server never sees x!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fhe.md` around lines 17 - 18, The diagram line currently uses an upward arrow ("↑") which breaks left-to-right flow; replace it with a single left-to-right pipeline such as "You: encrypt(x) → Server: f(encrypt(x)) → You: decrypt(result) = f(x)" and move the annotation "Server never sees x!" as a parenthetical or a caption under the pipeline (or inline after decrypt(result)) so the flow reads left-to-right and the note remains visible; update occurrences of encrypt, f(encrypt(x)), decrypt(result) and f(x) in the section accordingly.packages/enclave-contracts/contracts/interfaces/IEnclave.sol (1)
531-534: ExposeparamSetRegistryon the interface as well.
paramSetRegistryis now part of the public contract surface, butIEnclavestill only declares the setter.templates/default/server/index.tshas to inline an ABI fragment just to read the mapping, which is an easy place for ABI drift.♻️ Suggested interface addition
- /// `@notice` Registers ABI-encoded BFV parameters for a param set enum variant. + /// `@notice` Registers ABI-encoded BFV parameters for a param set index. /// `@param` paramSet The param set index to register. /// `@param` encodedParams ABI-encoded BFV parameters. function setParamSet(uint8 paramSet, bytes calldata encodedParams) external; + + /// `@notice` Returns the ABI-encoded BFV parameters for a param set index. + /// `@param` paramSet The param set index to look up. + function paramSetRegistry(uint8 paramSet) external view returns (bytes memory);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/interfaces/IEnclave.sol` around lines 531 - 534, Add a public getter declaration for the paramSetRegistry mapping to the IEnclave interface so callers can read registered params without inlining ABI fragments; specifically update IEnclave to declare a view accessor for the mapping (matching the contract's public mapping) such as adding a function signature for paramSetRegistry(uint8) external view returns (bytes memory) alongside the existing setParamSet, ensuring the interface and implementing contract signatures match.packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
66-72: Consider removing the redundanttrueclause.The condition
&& trueat line 72 is a no-op leftover from removing the params array comparison. This clause adds no filtering value and makes the code harder to read.Additionally,
timeoutConfigis stored inconstructorArgsbut not compared for reuse eligibility. Either add the comparison or document why it's intentionally omitted.♻️ Suggested cleanup
preDeployedArgs?.constructorArgs?.owner === owner && preDeployedArgs?.constructorArgs?.maxDuration === maxDuration && preDeployedArgs?.constructorArgs?.registry === registry && preDeployedArgs?.constructorArgs?.bondingRegistry === bondingRegistry && preDeployedArgs?.constructorArgs?.e3RefundManager === e3RefundManager && - preDeployedArgs?.constructorArgs?.feeToken === feeToken && - true) + preDeployedArgs?.constructorArgs?.feeToken === feeToken)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/scripts/deployAndSave/enclave.ts` around lines 66 - 72, Remove the redundant "&& true" at the end of the pre-deployment condition that checks preDeployedArgs?.constructorArgs against current params (the block using preDeployedArgs, constructorArgs, owner, maxDuration, registry, bondingRegistry, e3RefundManager, feeToken) to simplify the boolean expression; additionally, either add a comparison for timeoutConfig (preDeployedArgs?.constructorArgs?.timeoutConfig === timeoutConfig) into that same condition so reuse eligibility considers it, or add a short inline comment beside the condition documenting why timeoutConfig is intentionally excluded.packages/enclave-contracts/scripts/deployEnclave.ts (1)
233-236: Only insecure param set is registered; consider registering secure params too.The script only registers
ParamSet.Insecure512at index 0. Per the SDK types inpackages/enclave-sdk/src/contracts/types.ts, there's alsoSecure8192 = 1. For production readiness, consider also registering the secure param set:// Secure 8192 params would need separate encoding with production parameters await enclave.setParamSet(1, encodedSecureParams);If this is intentional for local/mock deployments only, a comment clarifying this would be helpful.
🤖 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 233 - 236, The deploy script only registers ParamSet.Insecure512 via enclave.setParamSet(0, encoded); add registration for the secure params by preparing an encodedSecureParams value and calling enclave.setParamSet(1, encodedSecureParams) to register ParamSet.Secure8192, or if this is intentional for local/mock deployments, add a clear comment near the enclave.setParamSet call explaining that only Insecure512 is registered for testing and production Secure8192 must be added elsewhere; reference enclave.setParamSet, ParamSet.Insecure512 and ParamSet.Secure8192 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 `@examples/CRISP/server/.env.example`:
- Around line 16-19: Replace the hardcoded example addresses in the .env example
by copying the current localhost entries from the deployed_contracts.json for
the corresponding contracts: update ENCLAVE_ADDRESS, FEE_TOKEN_ADDRESS and
CIPHERNODE_REGISTRY_ADDRESS to the values listed for those names, and change
E3_PROGRAM_ADDRESS to the CRISPProgram address (it currently points to
HonkVerifier); ensure the four variables (ENCLAVE_ADDRESS, FEE_TOKEN_ADDRESS,
E3_PROGRAM_ADDRESS, CIPHERNODE_REGISTRY_ADDRESS) exactly match the
deployed_contracts.json localhost entries.
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 295-301: getE3Quote currently allows any uint8 paramSet even if
missing from paramSetRegistry; mirror the guard used in request() by loading
bytes memory e3ProgramParams = paramSetRegistry[requestParams.paramSet] inside
getE3Quote(), require(e3ProgramParams.length > 0, "BFV param set not
registered"), and only then call requestParams.e3Program.validate(...). This
ensures getE3Quote rejects unknown paramSet values the same way request() does.
---
Duplicate comments:
In `@examples/CRISP/packages/crisp-contracts/deployed_contracts.json`:
- Around line 130-149: The committed deployed_contracts.json contains stray
fallback network sections named "undefined" and "default" (see keys "undefined",
"default" and contracts "MockUSDC", "EnclaveToken") that should be removed and
replaced with only real network entries; update the deployment artifact by
deleting the entire "undefined" and "default" blocks so addresses are
unambiguous, and fix the deployment script that produces them to derive the
network name reliably by using (await signer.provider?.getNetwork())?.name ??
"localhost" instead of hre.globalOptions.network so future runs don't emit an
undefined fallback.
---
Nitpick comments:
In `@fhe.md`:
- Around line 204-208: Reformat the ENCRYPT and DECRYPT sections into explicit
numbered or bulleted algorithm steps instead of run-on text: break ENCRYPT into
sequential steps (1. Sample random mask c₁(X) = …; 2. Sample small error e(X) =
…; 3. Compute body c₀ = -c₁ · s₁ + m + e with polynomial multiplication mod
X⁴+1; 4. Output ciphertext ct = (c₀, c₁)), and break DECRYPT into steps (1.
Compute c₀ + c₁ · s₁ = m + e; 2. Round away e → recover m), ensuring symbols
like ENCRYPT, DECRYPT, c₀, c₁, s₁, m, e, ct are preserved and punctuation/line
breaks make each step clear and readable.
- Line 54: Split the run‑together examples into separate lines to improve
readability; in fhe.md replace the single line "Examples: 3.7 → 0.7 -0.3 → 0.7
(same point!) 1.0 → 0.0 0.25 → 0.25" with each example on its own line (e.g.,
"3.7 → 0.7", "-0.3 → 0.7 (same point!)", "1.0 → 0.0", "0.25 → 0.25") so each
example is clearly separated and easy to parse.
- Line 632: Replace the heading text "End Result" with a more concise
alternative (e.g., "Result" or "Outcome") in the fhe.md document; locate the
header by searching for the exact string "End Result" and update it to the
chosen shorter heading, ensuring any internal links or references to that header
are updated to match the new heading text.
- Around line 17-18: The diagram line currently uses an upward arrow ("↑") which
breaks left-to-right flow; replace it with a single left-to-right pipeline such
as "You: encrypt(x) → Server: f(encrypt(x)) → You: decrypt(result) = f(x)" and
move the annotation "Server never sees x!" as a parenthetical or a caption under
the pipeline (or inline after decrypt(result)) so the flow reads left-to-right
and the note remains visible; update occurrences of encrypt, f(encrypt(x)),
decrypt(result) and f(x) in the section accordingly.
In `@packages/enclave-contracts/contracts/interfaces/IEnclave.sol`:
- Around line 531-534: Add a public getter declaration for the paramSetRegistry
mapping to the IEnclave interface so callers can read registered params without
inlining ABI fragments; specifically update IEnclave to declare a view accessor
for the mapping (matching the contract's public mapping) such as adding a
function signature for paramSetRegistry(uint8) external view returns (bytes
memory) alongside the existing setParamSet, ensuring the interface and
implementing contract signatures match.
In `@packages/enclave-contracts/scripts/deployAndSave/enclave.ts`:
- Around line 66-72: Remove the redundant "&& true" at the end of the
pre-deployment condition that checks preDeployedArgs?.constructorArgs against
current params (the block using preDeployedArgs, constructorArgs, owner,
maxDuration, registry, bondingRegistry, e3RefundManager, feeToken) to simplify
the boolean expression; additionally, either add a comparison for timeoutConfig
(preDeployedArgs?.constructorArgs?.timeoutConfig === timeoutConfig) into that
same condition so reuse eligibility considers it, or add a short inline comment
beside the condition documenting why timeoutConfig is intentionally excluded.
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 233-236: The deploy script only registers ParamSet.Insecure512 via
enclave.setParamSet(0, encoded); add registration for the secure params by
preparing an encodedSecureParams value and calling enclave.setParamSet(1,
encodedSecureParams) to register ParamSet.Secure8192, or if this is intentional
for local/mock deployments, add a clear comment near the enclave.setParamSet
call explaining that only Insecure512 is registered for testing and production
Secure8192 must be added elsewhere; reference enclave.setParamSet,
ParamSet.Insecure512 and ParamSet.Secure8192 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12a3658e-faba-4855-a550-3a8d7d9dcf26
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
agent/flow-trace/07_CIPHERNODE_E3_DEEP_DIVE.mdcrates/aggregator/src/ext.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/events/src/enclave_event/ciphernode_selected.rscrates/events/src/enclave_event/e3_requested.rscrates/evm-helpers/src/contracts.rscrates/evm/src/enclave_sol_reader.rscrates/fhe-params/src/presets.rscrates/indexer/src/indexer.rscrates/keyshare/src/ext.rscrates/request/Cargo.tomlcrates/request/src/meta.rscrates/sortition/src/ciphernode_selector.rscrates/tests/tests/integration.rsexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.solexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonexamples/CRISP/server/.env.exampleexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/routes/rounds.rsfhe.mdlogs.txtlogs_c0c3_debug.txtpackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IE3.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/ignition/modules/enclave.tspackages/enclave-contracts/scripts/deployAndSave/enclave.tspackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Pricing/Pricing.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tspackages/enclave-sdk/src/contracts/contract-client.tspackages/enclave-sdk/src/contracts/types.tspackages/enclave-sdk/src/enclave-sdk.tspackages/enclave-sdk/src/events/types.tstemplates/default/client/src/pages/steps/RequestComputation.tsxtemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/logs.txttemplates/default/server/index.tstemplates/default/tests/integration.spec.ts
💤 Files with no reviewable changes (1)
- packages/enclave-contracts/ignition/modules/enclave.ts
✅ Files skipped from review due to trivial changes (7)
- crates/request/Cargo.toml
- templates/default/enclave.config.yaml
- examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol
- templates/default/tests/integration.spec.ts
- packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json
- packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json
- packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json
🚧 Files skipped from review as they are similar to previous changes (19)
- crates/sortition/src/ciphernode_selector.rs
- packages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.ts
- crates/fhe-params/src/presets.rs
- crates/indexer/src/indexer.rs
- crates/request/src/meta.rs
- examples/CRISP/server/src/cli/commands.rs
- crates/tests/tests/integration.rs
- templates/default/client/src/pages/steps/RequestComputation.tsx
- packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts
- examples/CRISP/server/src/server/routes/rounds.rs
- packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts
- packages/enclave-contracts/tasks/enclave.ts
- examples/CRISP/enclave.config.yaml
- packages/enclave-sdk/src/contracts/types.ts
- packages/enclave-sdk/src/enclave-sdk.ts
- packages/enclave-contracts/test/Enclave.spec.ts
- templates/default/deployed_contracts.json
- packages/enclave-contracts/contracts/interfaces/IE3.sol
- crates/aggregator/src/ext.rs
ctrlc03
force-pushed
the
refactor/param-enums
branch
from
0666ffe to
5444613
Compare
cedoor
previously approved these changes
Apr 7, 2026
# Conflicts: # crates/aggregator/src/ext.rs # crates/sortition/src/ciphernode_selector.rs # packages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.json # packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json # packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json # packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json # packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json # packages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.json # packages/enclave-contracts/deployed_contracts.json
ctrlc03
force-pushed
the
refactor/param-enums
branch
from
be89aa6 to
892ddaa
Compare
cedoor
approved these changes
Apr 7, 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
setParamSet) for managing BFV encryption parameters.ParamSetRegisteredevent for parameter set configuration tracking.Refactor
Chores