feat: committee size in request e3 [skip-line-limit]#1418
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Micro committee size (N=3,T=1,H=3) to Noir configs and switches defaults to micro; replaces threshold tuples with a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SDK as EnclaveSDK
participant Contract as EnclaveContract
participant Keyshare
participant ZK as ZKHelpers
User->>SDK: requestE3(params {committeeSize, ...})
SDK->>Contract: request(E3RequestParams{committeeSize,...})
Contract->>Contract: committeeThresholds[committeeSize] -> threshold (m,n)
Contract->>Keyshare: request proofs / PK generation using threshold (m,n)
Keyshare->>ZK: CiphernodesCommitteeSize::from_threshold(m,n)
ZK-->>Keyshare: circuit params (n,h,threshold)
Keyshare->>Contract: submit proofs / keys
Contract->>SDK: emit E3Requested/E3Published (with committeeSize)
SDK->>User: return tx/hash / quote
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
changed the title
ctrlc03
force-pushed
the
feat/committee-size-request
branch
from
f76c2ff to
32da43b
Compare
ctrlc03
changed the title
ctrlc03
force-pushed
the
feat/committee-size-request
branch
from
32da43b to
0f6debb
Compare
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/zk-helpers/src/ciphernodes_committee.rs (1)
48-73:⚠️ Potential issue | 🟡 MinorMedium and Large variants still trigger
unreachable!()— consider an explicit error or placeholder values.Issue
#1335noted thatMediumandLargecallunreachable!(), causing runtime panics. While thefrom_thresholdconstructor currently only producesMicroandSmall, the enum is public and serializable (serde), so external code could deserialize or constructMedium/Largevalues. Either implement placeholder values with a TODO, or return aResultto make the incomplete implementation explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/ciphernodes_committee.rs` around lines 48 - 73, The values() method on CiphernodesCommitteeSize currently panics for Medium and Large via unreachable!(); change it to return Result<CiphernodesCommittee, SomeError> (e.g., a crate error type or anyhow::Error) instead of panicking so callers must handle incomplete variants, update the signature of CiphernodesCommitteeSize::values to -> Result<CiphernodesCommittee, E>, map Micro and Small to Ok(...) and return Err(...) for Medium and Large with a clear message, and update any callers (e.g., from_threshold) to propagate or handle the Result; alternatively, if you prefer placeholders, explicitly return CiphernodesCommittee with documented TODO values for CiphernodesCommitteeSize::Medium and ::Large (and add comments/tests) rather than using unreachable!().crates/tests/tests/integration.rs (1)
772-779:⚠️ Potential issue | 🟠 MajorDon’t pin an exact 15-event history for a 1-of-3 run.
With
threshold_m = 1, aggregation can legally finish after the first verified share. The otherDecryptionshareCreatedevents can then land before or after the C6/C7 events, so expecting exactly 15 entries here is race-prone. Prefer waiting forPlaintextAggregatedand asserting the required event types are present instead of a fixed total/order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/tests/integration.rs` around lines 772 - 779, The test currently pins expected_count to 15 which is race-prone for threshold_m = 1; instead remove the fixed 15 and wait for the terminal PlaintextAggregated event then assert required event types are present. Change the take_history_with_timeouts call (where expected_count and h are set) to request a generous max (or loop/poll) until you see a PlaintextAggregated event, then inspect h for presence/counts of DecryptionshareCreated and PlaintextAggregated (and any required C6/C7 events) without assuming total ordering or exact total; update assertions to check set-membership/counts rather than exact sequence/length.examples/CRISP/server/src/cli/commands.rs (1)
163-170:⚠️ Potential issue | 🔴 CriticalAdd
ClonetoCommitteeSizeenum derives.The code at line 164 calls
committee_size.clone(), butCommitteeSizeincrates/evm-helpers/src/contracts.rsonly derivesDebug,Serialize, andDeserialize—notClone. This will cause a compilation error. AddCloneto the derive macro:#[derive(Debug, Clone, Serialize, Deserialize)] enum CommitteeSize { Micro, Small, Medium, Large, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/server/src/cli/commands.rs` around lines 163 - 170, The compiler error comes from calling committee_size.clone() when CommitteeSize does not implement Clone; update the CommitteeSize enum declaration in crates/evm-helpers/src/contracts.rs (symbol: CommitteeSize) to derive Clone (e.g., add Clone to the existing derive list alongside Debug, Serialize, Deserialize) so committee_size.clone() compiles; no other API changes needed.
🧹 Nitpick comments (4)
examples/CRISP/scripts/dev.sh (1)
38-38: Unrelated change:--silentflag removed from anvil command.This change enables verbose output from anvil during development, which is fine for debugging. However, it appears unrelated to the PR's objective (committee size in E3 requests).
Please confirm if this was intentional or an accidental leftover from local debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/scripts/dev.sh` at line 38, The anvil command string ("anvil --host 0.0.0.0 --chain-id 31337 --block-time 1 --mnemonic 'test test test test test test test test test test test junk'") had the --silent flag removed; confirm whether this was intentional or leftover from local debugging and then either (a) restore the --silent flag in that command if removal was accidental, or (b) leave it removed but update the PR description/commit message to explicitly state the change and why verbose anvil output is desired for this PR.templates/default/enclave.config.yaml (1)
7-16: Prefer generating thesedeploy_blockvalues from deployment artifacts instead of hand-editing literals.These values are operationally sensitive; manual updates here can drift from actual deploy tx blocks and break event backfills. Consider templating this file from deployment output as a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/enclave.config.yaml` around lines 7 - 16, The hard-coded deploy_block values for ciphernode_registry, bonding_registry, and fee_token in enclave.config.yaml should be generated from deployment artifacts instead of edited manually; update the release/build templating step to read the deployment artifacts (e.g., the contract artifact or deployment receipts that include blockNumber/transactionReceipt) and populate the deploy_block fields for the keys ciphernode_registry.deploy_block, bonding_registry.deploy_block, and fee_token.deploy_block at template render time (or add a small script that extracts block numbers and writes them into the enclave.config.yaml template) so the config always reflects actual on-chain deployment blocks.crates/tests/tests/integration.rs (1)
492-493: Please keep a larger-committee case in this suite.The linked regression was in the non-small committee-size path, but this setup now uses a 3-member committee only. By itself, this test won’t catch a Medium/Large circuit-selection regression. A companion case with a larger committee would protect the behavior this PR is fixing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/tests/integration.rs` around lines 492 - 493, This test only covers a 3-member committee (threshold_m = 1, threshold_n = 3); add a companion case that exercises a larger committee size to catch Medium/Large circuit-selection regressions: duplicate the relevant test scenario and set higher values for threshold_m and threshold_n (e.g., threshold_n = 7 or 10 and threshold_m accordingly) so the non-small-committee code paths run; update any local variables or helper calls used in the current test (referencing threshold_m and threshold_n) so the new case uses the larger committee while keeping the original 3-member case intact.packages/enclave-sdk/src/utils.ts (1)
80-84: Consider using theCommitteeSizeenum directly for type safety.Using the enum value instead of a magic number would improve clarity and prevent mismatches if enum values change.
♻️ Suggested improvement
+import { CommitteeSize } from './contracts/types' + // Default E3 configuration export const DEFAULT_E3_CONFIG = { - committeeSize: 1, // Small + committeeSize: CommitteeSize.Small, duration: 1800, // 30 minutes in seconds payment_amount: '0', // 0 ETH in wei } as const🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/src/utils.ts` around lines 80 - 84, DEFAULT_E3_CONFIG currently uses a magic number for committeeSize; replace the numeric literal with the appropriate CommitteeSize enum value (e.g., CommitteeSize.Small or whichever member maps to 1) and import CommitteeSize into this module so the config uses the enum value instead of 1; ensure the exported object remains typed as const and that any consumers expecting a numeric value still accept the enum (or cast if necessary).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@circuits/lib/src/configs/committee/mod.nr`:
- Around line 7-8: Add the missing module file micro.nr to match the declared
pub mod micro; — create circuits/lib/src/configs/committee/micro.nr and export
the same symbols as small.nr (H, N_PARTIES, T) with values appropriate for the
micro configuration; follow the structure and naming in small.nr so the module
compiles and the public symbols H, N_PARTIES, and T are available to callers.
In `@circuits/lib/src/configs/default/mod.nr`:
- Line 10: The import `pub use super::committee::micro::{H, N_PARTIES, T};`
references a non-existent module `micro`, causing compilation failure; either
add the missing module file `micro.nr` under the committee config that defines
the committee params H, N_PARTIES and T (matching the API expected by
`default/mod.nr`), or change this line to import the existing module (e.g.
`super::committee::small::{H, N_PARTIES, T}`) so the symbols resolve; update
`pub use super::committee::micro::{H, N_PARTIES, T};` accordingly to point to
the implemented module or create the `micro` module with the required constants.
In `@crates/evm-helpers/src/contracts.rs`:
- Around line 42-48: Add the Clone derive to the CommitteeSize enum so it can be
cloned where used; update the enum declaration (CommitteeSize) to include
#[derive(Clone, Debug, Serialize, Deserialize)] so calls to .clone() (e.g., in
examples/CRISP/server/src/cli/commands.rs around the code that clones a
CommitteeSize) compile successfully.
In `@crates/evm/src/enclave_sol_reader.rs`:
- Around line 35-39: The match on value.0.e3.committeeSize currently only maps 0
and 1 and panics on other values; update the mapping in enclave_sol_reader.rs to
handle 2 -> CiphernodesCommitteeSize::Medium and 3 ->
CiphernodesCommitteeSize::Large (remove the panic fallback), and update the
CiphernodesCommitteeSize::values() implementation to return all four variants
(Micro, Small, Medium, Large) instead of leaving Medium/Large commented out; if
you prefer defensive behavior keep an explicit Err/Result for unexpected values
instead of panicking.
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 939-940: The call to
CiphernodesCommitteeSize::from_threshold(threshold_m as usize, threshold_n as
usize) can panic for unknown (m,n) pairs; change this site to handle a Result
return from from_threshold (or convert it to return Result) and propagate errors
with the ? operator instead of allowing a panic. Update the surrounding function
(in threshold_keyshare.rs where derived_committee_size is computed) to return a
compatible Result/error type if needed, use let derived_committee_size =
CiphernodesCommitteeSize::from_threshold(...)? and ensure any upstream callers
are adjusted to handle the propagated error.
In `@crates/zk-helpers/src/ciphernodes_committee.rs`:
- Around line 36-44: Change from_threshold to return a Result instead of
panicking: update the signature of from_threshold to return Result<Self, E> (use
a concrete error type like a new CommitteeSizeError or a descriptive String) and
replace the panic! branch with Err(...) that includes the unexpected
(threshold_m, threshold_n) values; keep the known mappings ((1,3) ->
Self::Micro, (2,5) -> Self::Small) returning Ok(Self::...). Update any callers
(e.g., code in threshold_keyshare.rs) to handle the Result (propagate the error
with ? or map it to an appropriate higher-level error) so unknown threshold
pairs no longer cause a runtime crash.
In `@examples/CRISP/server/src/config.rs`:
- Line 26: The e3_committee_size field currently typed as u8 must be replaced
with a validated domain type to prevent invalid values; define an enum (e.g.
E3CommitteeSize { Micro=0, Small=1, Medium=2, Large=3 }) or a newtype with
TryFrom<u8>/Deserialize logic that rejects out-of-range values, change the
Config struct's e3_committee_size to that enum/newtype, add a
TryFrom/FromStr/serde deserializer or implement validation in Config::validate
to map/validate incoming integers, and update all usages that read
e3_committee_size to use the new enum API instead of raw integers so invalid
values (like 4) fail at config load time.
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 983-994: The setter setCommitteeThresholds currently allows
arbitrary threshold pairs for a given CommitteeSize, letting the enum drift from
the actual committee member count; change the setter to either (A) validate
threshold[1] against a canonical member count for the given CommitteeSize (e.g.,
require(threshold[1] == canonicalMembers[size]) using a mapping/array like
canonicalMembers[CommitteeSize]) or (B) stop accepting a CommitteeSize and
derive the enum from threshold[1] (compute size =
committeeSizeFromMemberCount(threshold[1]) and then set
committeeThresholds[size] = threshold); update the require checks and emit
CommitteeThresholdsUpdated accordingly and add/adjust unit tests to cover both
valid and invalid mappings for setCommitteeThresholds and the canonical member
counts.
In `@packages/enclave-contracts/contracts/interfaces/IEnclave.sol`:
- Around line 457-464: The setter setCommitteeThresholds is missing validation
that the total threshold (threshold[1]) is positive; update the require in
setCommitteeThresholds to check threshold[1] > 0 in addition to the existing
checks (threshold[0] > 0 and threshold[0] <= threshold[1]) so that threshold[1]
cannot be zero, preventing an invalid total of 0.
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 212-219: The deployment only sets committee thresholds for indices
0 and 1 via enclave.setCommitteeThresholds, leaving other enum variants
uninitialized which can break committee-size-based requests; update the
deployEnclave.ts logic to load a single config (e.g., a thresholds map/array)
and iterate over all supported committee size enum entries, calling
enclave.setCommitteeThresholds(index, thresholds) for each entry so every enum
variant is initialized; locate and modify the block around the existing
setCommitteeThresholds calls to replace hard-coded calls with a loop over the
config (referencing enclave.setCommitteeThresholds and the committee-size
enum/constant used in the project).
In `@packages/enclave-contracts/tasks/enclave.ts`:
- Around line 24-27: The task argument "committeeSize" accepts any integer but
only valid semantic values are 0..3; add explicit bounds validation in the task
handler before making any contract calls by checking the parsed committeeSize
(the argument defined as name: "committeeSize", type: ArgumentType.INT) and
throwing or exiting with a clear error if committeeSize < 0 or committeeSize >
3; update the task's runtime validation (where the task params are read/used) to
fail fast with a descriptive message mentioning "committeeSize" so invalid
inputs never reach the contract call.
In `@packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts`:
- Around line 289-290: The test sets requestParams.committeeSize = 0 which in
the shared fixture maps to a three-member size ([1,3]) but this suite only
registers operator1 and operator2 and later publishes/finalizes a 2-node
committee; update the test so requestParams.committeeSize matches the intended
two-member scenario (set it to 2 or the fixture value for two members) or
alternatively register a third operator (e.g., add operator3 to the registration
steps) so the registered operators align with the committee size used in the
publish/finalize assertions involving requestParams, operator1, and operator2.
In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts`:
- Line 780: The test is calling makeRequest(0) which sets committeeSize=0 (Micro
-> [1,3]) but the assertions expect a 2-of-2 committee; update the test so the
request uses the committee size that maps to a 2-of-2 configuration (e.g., call
makeRequest with the index that yields the 2-of-2 mapping) or modify the fixture
mapping for committeeSize 0 to produce a 2-of-2 threshold; adjust the call to
makeRequest or the fixture mapping and ensure the thresholdM and N expectations
align with the chosen committeeSize.
In `@templates/default/client/src/pages/steps/RequestComputation.tsx`:
- Line 19: Remove the unused CommitteeSize import from the import list in
RequestComputation.tsx; the file already reads the committee size via
DEFAULT_E3_CONFIG.committeeSize so delete CommitteeSize from the named imports
(where CommitteeSize is currently imported) to eliminate the unused-symbol
warning and keep imports accurate.
---
Outside diff comments:
In `@crates/tests/tests/integration.rs`:
- Around line 772-779: The test currently pins expected_count to 15 which is
race-prone for threshold_m = 1; instead remove the fixed 15 and wait for the
terminal PlaintextAggregated event then assert required event types are present.
Change the take_history_with_timeouts call (where expected_count and h are set)
to request a generous max (or loop/poll) until you see a PlaintextAggregated
event, then inspect h for presence/counts of DecryptionshareCreated and
PlaintextAggregated (and any required C6/C7 events) without assuming total
ordering or exact total; update assertions to check set-membership/counts rather
than exact sequence/length.
In `@crates/zk-helpers/src/ciphernodes_committee.rs`:
- Around line 48-73: The values() method on CiphernodesCommitteeSize currently
panics for Medium and Large via unreachable!(); change it to return
Result<CiphernodesCommittee, SomeError> (e.g., a crate error type or
anyhow::Error) instead of panicking so callers must handle incomplete variants,
update the signature of CiphernodesCommitteeSize::values to ->
Result<CiphernodesCommittee, E>, map Micro and Small to Ok(...) and return
Err(...) for Medium and Large with a clear message, and update any callers
(e.g., from_threshold) to propagate or handle the Result; alternatively, if you
prefer placeholders, explicitly return CiphernodesCommittee with documented TODO
values for CiphernodesCommitteeSize::Medium and ::Large (and add comments/tests)
rather than using unreachable!().
In `@examples/CRISP/server/src/cli/commands.rs`:
- Around line 163-170: The compiler error comes from calling
committee_size.clone() when CommitteeSize does not implement Clone; update the
CommitteeSize enum declaration in crates/evm-helpers/src/contracts.rs (symbol:
CommitteeSize) to derive Clone (e.g., add Clone to the existing derive list
alongside Debug, Serialize, Deserialize) so committee_size.clone() compiles; no
other API changes needed.
---
Nitpick comments:
In `@crates/tests/tests/integration.rs`:
- Around line 492-493: This test only covers a 3-member committee (threshold_m =
1, threshold_n = 3); add a companion case that exercises a larger committee size
to catch Medium/Large circuit-selection regressions: duplicate the relevant test
scenario and set higher values for threshold_m and threshold_n (e.g.,
threshold_n = 7 or 10 and threshold_m accordingly) so the non-small-committee
code paths run; update any local variables or helper calls used in the current
test (referencing threshold_m and threshold_n) so the new case uses the larger
committee while keeping the original 3-member case intact.
In `@examples/CRISP/scripts/dev.sh`:
- Line 38: The anvil command string ("anvil --host 0.0.0.0 --chain-id 31337
--block-time 1 --mnemonic 'test test test test test test test test test test
test junk'") had the --silent flag removed; confirm whether this was intentional
or leftover from local debugging and then either (a) restore the --silent flag
in that command if removal was accidental, or (b) leave it removed but update
the PR description/commit message to explicitly state the change and why verbose
anvil output is desired for this PR.
In `@packages/enclave-sdk/src/utils.ts`:
- Around line 80-84: DEFAULT_E3_CONFIG currently uses a magic number for
committeeSize; replace the numeric literal with the appropriate CommitteeSize
enum value (e.g., CommitteeSize.Small or whichever member maps to 1) and import
CommitteeSize into this module so the config uses the enum value instead of 1;
ensure the exported object remains typed as const and that any consumers
expecting a numeric value still accept the enum (or cast if necessary).
In `@templates/default/enclave.config.yaml`:
- Around line 7-16: The hard-coded deploy_block values for ciphernode_registry,
bonding_registry, and fee_token in enclave.config.yaml should be generated from
deployment artifacts instead of edited manually; update the release/build
templating step to read the deployment artifacts (e.g., the contract artifact or
deployment receipts that include blockNumber/transactionReceipt) and populate
the deploy_block fields for the keys ciphernode_registry.deploy_block,
bonding_registry.deploy_block, and fee_token.deploy_block at template render
time (or add a small script that extracts block numbers and writes them into the
enclave.config.yaml template) so the config always reflects actual on-chain
deployment blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16c6175e-412f-429d-9e8e-8d29ec43825b
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
circuits/lib/src/configs/committee/mod.nrcircuits/lib/src/configs/default/mod.nrcircuits/lib/src/configs/insecure/dkg.nrcircuits/lib/src/configs/secure/dkg.nrcrates/evm-helpers/Cargo.tomlcrates/evm-helpers/src/contracts.rscrates/evm-helpers/src/events.rscrates/evm/Cargo.tomlcrates/evm/src/enclave_sol_reader.rscrates/indexer/src/indexer.rscrates/indexer/src/models.rscrates/indexer/tests/fixtures/fake_enclave.solcrates/keyshare/src/threshold_keyshare.rscrates/tests/tests/integration.rscrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/ciphernodes_committee.rsexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.solexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonexamples/CRISP/scripts/dev.shexamples/CRISP/server/.env.exampleexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/config.rsexamples/CRISP/server/src/server/routes/rounds.rspackages/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/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IE3.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/deployed_contracts.jsonpackages/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/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.tspackages/enclave-sdk/src/index.tspackages/enclave-sdk/src/types.tspackages/enclave-sdk/src/utils.tstemplates/default/client/src/pages/steps/RequestComputation.tsxtemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/tests/integration.spec.tstests/integration/base.shtests/integration/persist.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/enclave-sdk/src/contracts/contract-client.ts (1)
135-166:⚠️ Potential issue | 🟡 MinorReject invalid
committeeSizevalues before calling the contract.Line 152 and Line 224 pass
committeeSizestraight through. Since the SDK surface still accepts arbitrary numbers, out-of-range values only fail at the ABI/contract boundary and get wrapped as a genericSDKError.Suggested fix
export class ContractClient { private publicClient: PublicClient private walletClient?: WalletClient private contracts: ContractAddresses @@ constructor(config: ContractClientConfig) { @@ } } + + private assertCommitteeSize(committeeSize: number): void { + if (!Number.isInteger(committeeSize) || committeeSize < 0 || committeeSize > 3) { + throw new SDKError( + 'Invalid committeeSize. Expected 0 (Micro), 1 (Small), 2 (Medium), or 3 (Large).', + 'INVALID_COMMITTEE_SIZE', + ) + } + } @@ public async requestE3(params: E3RequestParams): Promise<Hash> { @@ try { + this.assertCommitteeSize(params.committeeSize) + const account = this.walletClient.account if (!account) { throw new SDKError('No account connected', 'NO_ACCOUNT') } @@ public async getE3Quote(requestParams: E3RequestParams): Promise<bigint> { try { + this.assertCommitteeSize(requestParams.committeeSize) + return this.publicClient.readContract({ address: this.contracts.enclave, abi: Enclave__factory.abi, functionName: 'getE3Quote',Also applies to: 216-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/src/contracts/contract-client.ts` around lines 135 - 166, Validate the committeeSize before invoking the contract: in requestE3 (and any other methods that pass committeeSize into publicClient.simulateContract), add a pre-check that committeeSize is a number, an integer, and within the contract-expected bounds (reject negatives/zero and values outside the allowed max), and if invalid throw an SDKError (e.g. 'INVALID_COMMITTEE_SIZE') before calling simulateContract or walletClient.writeContract; update both the simulateContract args construction (the object passed as the single arg to functionName 'request') and any other call sites that forward params.committeeSize so invalid values never reach the ABI.crates/evm-helpers/src/contracts.rs (1)
167-174:⚠️ Potential issue | 🟡 MinorTypo in parameter name:
commitee_sizeshould becommittee_size.The parameter name on line 169 is missing a 't' in "committee". This creates inconsistency with other usages (e.g., line 397, 453).
✏️ Proposed fix
async fn get_e3_quote( &self, - commitee_size: CommitteeSize, + committee_size: CommitteeSize, input_window: [U256; 2], e3_program: Address, e3_params: Bytes, compute_provider_params: Bytes, ) -> Result<U256>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm-helpers/src/contracts.rs` around lines 167 - 174, The async function signature get_e3_quote has a misspelled parameter name commitee_size; rename it to committee_size in the trait/signature and update all corresponding implementations and call sites (e.g., any impl of get_e3_quote and usages elsewhere) to use committee_size consistently so the symbol names match across declarations and references.
♻️ Duplicate comments (4)
crates/evm/src/enclave_sol_reader.rs (1)
35-39:⚠️ Potential issue | 🔴 CriticalRegenerate contract artifacts:
committeeSizefield missing from ABI.The generated Rust bindings from
IEnclave.jsondo not include thecommitteeSizefield because the ABI JSON is stale. The Solidity source (IE3.sol) defines theE3struct withcommitteeSize, but the compiled artifact is missing this field. The contract artifacts must be regenerated after the Solidity interface changes.Additionally, the match statement at lines 35-39 is incomplete: it only handles committee sizes 0 (
Micro) and 1 (Small), but theCiphernodesCommitteeSizeenum has four variants (Micro,Small,Medium,Large). This will panic at runtime for committee sizes 2 and 3. Add cases forMediumandLarge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm/src/enclave_sol_reader.rs` around lines 35 - 39, Regenerate the Solidity contract artifacts so the ABI/JSON for IEnclave includes the E3.committeeSize field (recompile IE3.sol and re-run the bindings generation) and then update the Rust match that sets committee_size (the code using value.0.e3.committeeSize and the CiphernodesCommitteeSize enum) to handle all four variants: map 0->Micro, 1->Small, 2->Medium, 3->Large instead of only Micro/Small to avoid panics at runtime.packages/enclave-contracts/tasks/enclave.ts (1)
24-27:⚠️ Potential issue | 🟡 MinorValidate
committeeSizebefore issuing the request.Line 24 documents a closed
0..3value set, but Line 68 still accepts any integer. Invalid values currently make it togetE3Quote/requestand then fail later with a much less helpful revert.Suggested fix
.setAction(async () => ({ default: async ( { committeeSize, inputWindowStart, inputWindowEnd, e3Address, e3Params, computeParams, customParams, }, hre, ) => { + if (committeeSize < 0 || committeeSize > 3) { + throw new Error( + "Invalid committeeSize. Expected 0 (Micro), 1 (Small), 2 (Medium), or 3 (Large).", + ); + } + const connection = await hre.network.connect(); const { ethers } = connection;Also applies to: 65-78
🤖 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 24 - 27, The task accepts a committeeSize Argument but never validates it; add an explicit check (e.g., in the task handler that reads committeeSize before calling getE3Quote or request) to ensure committeeSize is an integer within 0..3 and throw or fail fast with a clear message if out of range; update the code paths that call getE3Quote and request so they only run after this validation (refer to the committeeSize variable and the getE3Quote/request call sites around the task handler).crates/evm-helpers/src/contracts.rs (1)
42-48:⚠️ Potential issue | 🔴 CriticalMissing
Clonederive onCommitteeSize.The
CommitteeSizeenum is used with.clone()inexamples/CRISP/server/src/cli/commands.rs. Without derivingClone, this will cause a compilation error.🐛 Proposed fix
- #[derive(Debug, Serialize, Deserialize)] + #[derive(Debug, Clone, Serialize, Deserialize)] enum CommitteeSize { Micro, Small, Medium, Large, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm-helpers/src/contracts.rs` around lines 42 - 48, The enum CommitteeSize lacks a Clone derive but is cloned elsewhere (e.g., examples/CRISP/server/src/cli/commands.rs), causing a compile error; fix it by adding Clone to the derive attribute on the CommitteeSize enum (update #[derive(Debug, Serialize, Deserialize)] to include Clone) so CommitteeSize implements Clone and the existing .clone() calls compile.packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts (1)
851-878:⚠️ Potential issue | 🟡 MinorTest expectations don't align with Micro committee configuration.
The test uses
committeeSize=0(Micro → [1,3]) but:
- Only 2 operators are set up while N=3 requires 3
- Line 875 expects
thresholdM=2but Micro has M=1- Comment says "no room for error" which implies M=N, but Micro is 1-of-3
The assertion
.withArgs(0, 1, 2, false)expectsthresholdM=2, suggesting this test was designed for a 2-of-2 committee. Consider either:
- Using a committeeSize that maps to [2,2] or similar
- Adjusting the fixture threshold for Micro to match test intent
- Updating assertions to match Micro's actual [1,3] configuration
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts` around lines 851 - 878, The test's expectations conflict with the Micro committee configuration; update the setup or assertions so M/N match: either create the committee with a size that yields a 2-of-2 (or M=N) configuration before calling makeRequest and finalizeCommitteeWithOperators, or adjust expectations to Micro's [1,3] (e.g., use thresholdM=1 and expect .withArgs(0, 1, 3, false) and stage check accordingly). Locate the setup around makeRequest(0) / finalizeCommitteeWithOperators(0, [operator1, operator2]) and the assertions on registry CommitteeViabilityUpdated and enclave.getE3Stage(0) and change the committeeSize/fixture or the .withArgs/expect values to be consistent.
🧹 Nitpick comments (3)
crates/tests/tests/integration.rs (2)
779-780: Event-count assertion is brittle and can drift from real sequence.At Line 780,
expected_countis hardcoded arithmetic. This makes the test fragile when committee/event shapes change (which this PR is already doing). Prefer deriving it from a single expected-events structure (or at least fromthreshold_n) so comments and assertions cannot diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/tests/integration.rs` around lines 779 - 780, The hardcoded arithmetic for expected_count is brittle; replace it by deriving the expected total from the actual expected-events source (e.g., compute from the length of the vector/structure you use to enumerate expected events or from threshold_n) instead of manual sums. Locate the declaration/usage of expected_count and change it to compute its value from the canonical expected-events container (e.g., expected_events.len() or a formula using threshold_n) so the test assertion always reflects the declared expected-events sequence.
117-272: Reduce copy/paste in circuit artifact staging with a helper.The repeated
create_dir_all + copy(json) + copy(vk_noir→vk)pattern is error-prone to maintain as circuits evolve.♻️ Suggested refactor
+ async fn stage_recursive_circuit( + recursive_dir: &PathBuf, + group: &str, + name: &str, + target_root: &PathBuf, + ) { + let dst = recursive_dir.join(group).join(name); + tokio::fs::create_dir_all(&dst).await.unwrap(); + tokio::fs::copy( + target_root.join(format!("{name}.json")), + dst.join(format!("{name}.json")), + ) + .await + .unwrap(); + tokio::fs::copy( + target_root.join(format!("{name}.vk_noir")), + dst.join(format!("{name}.vk")), + ) + .await + .unwrap(); + } + - // Copy C1 (pk_generation) circuit - let pk_gen_circuit_dir = recursive_dir.join("threshold").join("pk_generation"); - tokio::fs::create_dir_all(&pk_gen_circuit_dir).await.unwrap(); - ... - tokio::fs::copy( - threshold_target.join("pk_generation.vk_noir"), - pk_gen_circuit_dir.join("pk_generation.vk"), - ) - .await - .unwrap(); + stage_recursive_circuit(&recursive_dir, "dkg", "pk", &dkg_target).await; + stage_recursive_circuit(&recursive_dir, "threshold", "pk_generation", &threshold_target).await; + stage_recursive_circuit(&recursive_dir, "dkg", "sk_share_computation", &dkg_target).await; + stage_recursive_circuit(&recursive_dir, "dkg", "e_sm_share_computation", &dkg_target).await; + stage_recursive_circuit(&recursive_dir, "dkg", "share_encryption", &dkg_target).await; + stage_recursive_circuit(&recursive_dir, "dkg", "share_decryption", &dkg_target).await; + stage_recursive_circuit(&recursive_dir, "threshold", "pk_aggregation", &threshold_target).await; + stage_recursive_circuit(&recursive_dir, "threshold", "share_decryption", &threshold_target).await; + stage_recursive_circuit(&recursive_dir, "threshold", "decrypted_shares_aggregation_mod", &threshold_target).await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/tests/integration.rs` around lines 117 - 272, The copies repeat the pattern of creating a circuit dir and copying foo.json and foo.vk_noir→foo.vk; extract this into a helper (e.g., async fn stage_circuit_artifact(src_target: &Path, dest_root: &Path, subdir: &str, base_name: &str) -> Result<()>) that does tokio::fs::create_dir_all(dest_dir) and two tokio::fs::copy calls (src_target.join(format!("{base_name}.json")) → dest_dir.join(format!("{base_name}.json")) and src_target.join(format!("{base_name}.vk_noir")) → dest_dir.join(format!("{base_name}.vk"))), then replace each block that builds pk_circuit_dir, pk_gen_circuit_dir, sk_share_comp_circuit_dir, e_sm_share_comp_circuit_dir, share_enc_circuit_dir, share_dec_circuit_dir, pk_agg_circuit_dir, threshold_share_dec_circuit_dir, dsa_circuit_dir with calls to stage_circuit_artifact using the appropriate dkg_target or threshold_target and the corresponding subdir and base_name; propagate/unwrap errors the same way as current code.packages/enclave-sdk/src/contracts/types.ts (1)
13-18: UseCommitteeSizein the exported SDK interfaces too.The enum is public now, but both
E3.committeeSizeandE3RequestParams.committeeSizeare still typed as rawnumber. Switching those fields toCommitteeSizekeeps the SDK surface aligned with the contract model and makes the intended values clearer to consumers.Suggested refactor
export interface E3 { seed: bigint - committeeSize: number + committeeSize: CommitteeSize requestBlock: bigint inputWindow: readonly [bigint, bigint] encryptionSchemeId: string e3Program: string @@ export interface E3RequestParams extends RequestParams { - committeeSize: number + committeeSize: CommitteeSize inputWindow: readonly [bigint, bigint] e3Program: `0x${string}` e3ProgramParams: `0x${string}` computeProviderParams: `0x${string}`Also applies to: 20-23, 38-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/src/contracts/types.ts` around lines 13 - 18, Change the raw numeric types for committee size to use the exported CommitteeSize enum: update the E3 interface's committeeSize property and the E3RequestParams interface's committeeSize property to be typed as CommitteeSize instead of number, ensuring any related imports/exports still expose CommitteeSize and that callers are updated to pass CommitteeSize enum values where those fields are used (references: CommitteeSize, E3.committeeSize, E3RequestParams.committeeSize).
🤖 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/zk-helpers/src/bin/zk_cli.rs`:
- Around line 56-64: The CLI parser function parse_committee currently only
matches "micro" and "small" and returns an error for other values; update
parse_committee and the analogous parsing at the other location (lines 173-175)
to handle all variants of the CiphernodesCommitteeSize enum (including "medium"
and "large") by adding branches that return CiphernodesCommitteeSize::Medium and
CiphernodesCommitteeSize::Large respectively, and ensure the error message (if
retained) lists all supported strings; reference the CiphernodesCommitteeSize
enum in crates/zk-helpers/src/ciphernodes_committee.rs to confirm exact variant
names.
In `@examples/CRISP/server/src/cli/commands.rs`:
- Around line 133-139: The match on CONFIG.e3_committee_size currently panics on
invalid values; change it to return an error instead so initialize_crisp_round
can propagate a failure, e.g. replace the panic arm with an Err variant (or use
anyhow::bail! / return Err(anyhow!("Invalid committee size: {}",
CONFIG.e3_committee_size))) and adjust the surrounding code in
initialize_crisp_round to return Result<..., Error> (or propagate the error)
rather than aborting; update the binding for committee_size to be produced via ?
(or map_ok) so valid values yield CommitteeSize::Micro/Small/Medium/Large and
invalid values produce a propagated error.
---
Outside diff comments:
In `@crates/evm-helpers/src/contracts.rs`:
- Around line 167-174: The async function signature get_e3_quote has a
misspelled parameter name commitee_size; rename it to committee_size in the
trait/signature and update all corresponding implementations and call sites
(e.g., any impl of get_e3_quote and usages elsewhere) to use committee_size
consistently so the symbol names match across declarations and references.
In `@packages/enclave-sdk/src/contracts/contract-client.ts`:
- Around line 135-166: Validate the committeeSize before invoking the contract:
in requestE3 (and any other methods that pass committeeSize into
publicClient.simulateContract), add a pre-check that committeeSize is a number,
an integer, and within the contract-expected bounds (reject negatives/zero and
values outside the allowed max), and if invalid throw an SDKError (e.g.
'INVALID_COMMITTEE_SIZE') before calling simulateContract or
walletClient.writeContract; update both the simulateContract args construction
(the object passed as the single arg to functionName 'request') and any other
call sites that forward params.committeeSize so invalid values never reach the
ABI.
---
Duplicate comments:
In `@crates/evm-helpers/src/contracts.rs`:
- Around line 42-48: The enum CommitteeSize lacks a Clone derive but is cloned
elsewhere (e.g., examples/CRISP/server/src/cli/commands.rs), causing a compile
error; fix it by adding Clone to the derive attribute on the CommitteeSize enum
(update #[derive(Debug, Serialize, Deserialize)] to include Clone) so
CommitteeSize implements Clone and the existing .clone() calls compile.
In `@crates/evm/src/enclave_sol_reader.rs`:
- Around line 35-39: Regenerate the Solidity contract artifacts so the ABI/JSON
for IEnclave includes the E3.committeeSize field (recompile IE3.sol and re-run
the bindings generation) and then update the Rust match that sets committee_size
(the code using value.0.e3.committeeSize and the CiphernodesCommitteeSize enum)
to handle all four variants: map 0->Micro, 1->Small, 2->Medium, 3->Large instead
of only Micro/Small to avoid panics at runtime.
In `@packages/enclave-contracts/tasks/enclave.ts`:
- Around line 24-27: The task accepts a committeeSize Argument but never
validates it; add an explicit check (e.g., in the task handler that reads
committeeSize before calling getE3Quote or request) to ensure committeeSize is
an integer within 0..3 and throw or fail fast with a clear message if out of
range; update the code paths that call getE3Quote and request so they only run
after this validation (refer to the committeeSize variable and the
getE3Quote/request call sites around the task handler).
In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts`:
- Around line 851-878: The test's expectations conflict with the Micro committee
configuration; update the setup or assertions so M/N match: either create the
committee with a size that yields a 2-of-2 (or M=N) configuration before calling
makeRequest and finalizeCommitteeWithOperators, or adjust expectations to
Micro's [1,3] (e.g., use thresholdM=1 and expect .withArgs(0, 1, 3, false) and
stage check accordingly). Locate the setup around makeRequest(0) /
finalizeCommitteeWithOperators(0, [operator1, operator2]) and the assertions on
registry CommitteeViabilityUpdated and enclave.getE3Stage(0) and change the
committeeSize/fixture or the .withArgs/expect values to be consistent.
---
Nitpick comments:
In `@crates/tests/tests/integration.rs`:
- Around line 779-780: The hardcoded arithmetic for expected_count is brittle;
replace it by deriving the expected total from the actual expected-events source
(e.g., compute from the length of the vector/structure you use to enumerate
expected events or from threshold_n) instead of manual sums. Locate the
declaration/usage of expected_count and change it to compute its value from the
canonical expected-events container (e.g., expected_events.len() or a formula
using threshold_n) so the test assertion always reflects the declared
expected-events sequence.
- Around line 117-272: The copies repeat the pattern of creating a circuit dir
and copying foo.json and foo.vk_noir→foo.vk; extract this into a helper (e.g.,
async fn stage_circuit_artifact(src_target: &Path, dest_root: &Path, subdir:
&str, base_name: &str) -> Result<()>) that does
tokio::fs::create_dir_all(dest_dir) and two tokio::fs::copy calls
(src_target.join(format!("{base_name}.json")) →
dest_dir.join(format!("{base_name}.json")) and
src_target.join(format!("{base_name}.vk_noir")) →
dest_dir.join(format!("{base_name}.vk"))), then replace each block that builds
pk_circuit_dir, pk_gen_circuit_dir, sk_share_comp_circuit_dir,
e_sm_share_comp_circuit_dir, share_enc_circuit_dir, share_dec_circuit_dir,
pk_agg_circuit_dir, threshold_share_dec_circuit_dir, dsa_circuit_dir with calls
to stage_circuit_artifact using the appropriate dkg_target or threshold_target
and the corresponding subdir and base_name; propagate/unwrap errors the same way
as current code.
In `@packages/enclave-sdk/src/contracts/types.ts`:
- Around line 13-18: Change the raw numeric types for committee size to use the
exported CommitteeSize enum: update the E3 interface's committeeSize property
and the E3RequestParams interface's committeeSize property to be typed as
CommitteeSize instead of number, ensuring any related imports/exports still
expose CommitteeSize and that callers are updated to pass CommitteeSize enum
values where those fields are used (references: CommitteeSize, E3.committeeSize,
E3RequestParams.committeeSize).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b77190d1-01ca-4262-93f6-9e54c4adef29
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
circuits/lib/src/configs/committee/micro.nrcircuits/lib/src/configs/committee/mod.nrcircuits/lib/src/configs/default/mod.nrcircuits/lib/src/configs/insecure/dkg.nrcircuits/lib/src/configs/secure/dkg.nrcrates/evm-helpers/Cargo.tomlcrates/evm-helpers/src/contracts.rscrates/evm-helpers/src/events.rscrates/evm/Cargo.tomlcrates/evm/src/enclave_sol_reader.rscrates/indexer/src/indexer.rscrates/indexer/src/models.rscrates/indexer/tests/fixtures/fake_enclave.solcrates/keyshare/src/threshold_keyshare.rscrates/tests/tests/integration.rscrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/ciphernodes_committee.rsexamples/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/config.rsexamples/CRISP/server/src/server/routes/rounds.rspackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IE3.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/deployed_contracts.jsonpackages/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/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.tspackages/enclave-sdk/src/index.tspackages/enclave-sdk/src/types.tspackages/enclave-sdk/src/utils.tstemplates/default/client/src/pages/steps/RequestComputation.tsxtemplates/default/deployed_contracts.jsontemplates/default/tests/integration.spec.tstests/integration/base.shtests/integration/persist.sh
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/enclave-contracts/scripts/deployEnclave.ts
- circuits/lib/src/configs/committee/mod.nr
- examples/CRISP/server/src/config.rs
- packages/enclave-sdk/src/utils.ts
- tests/integration/base.sh
- crates/keyshare/src/threshold_keyshare.rs
- templates/default/client/src/pages/steps/RequestComputation.tsx
- packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts
- examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol
- crates/indexer/src/indexer.rs
- packages/enclave-sdk/src/enclave-sdk.ts
- tests/integration/persist.sh
- packages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.ts
- crates/zk-helpers/src/ciphernodes_committee.rs
- circuits/lib/src/configs/insecure/dkg.nr
- packages/enclave-sdk/src/events/types.ts
- crates/evm-helpers/Cargo.toml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.ts (1)
317-319: Only Micro committee size is configured.The setup only configures Micro (
[1, 3]), which is sufficient since all tests in this file usecommitteeSize: 0. However, if additional tests requiring Small or Medium committees are added later, they will fail withCommitteeSizeNotConfigured.Consider adding Small and Medium configurations for consistency with other test files:
await enclave.setCommitteeThresholds(0, [1, 3]); // Micro await enclave.setCommitteeThresholds(1, [2, 3]); // Small await enclave.setCommitteeThresholds(2, [2, 4]); // Medium🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.ts` around lines 317 - 319, Add Small and Medium committee configurations in addition to the existing Micro setup so future tests won't hit CommitteeSizeNotConfigured; call enclave.setCommitteeThresholds for committee sizes 1 and 2 (Small and Medium) with appropriate threshold arrays (e.g., [2,3] and [2,4]) alongside the existing enclave.setCommitteeThresholds(0, [1,3]) call so all committee sizes are configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.ts`:
- Around line 317-319: Add Small and Medium committee configurations in addition
to the existing Micro setup so future tests won't hit
CommitteeSizeNotConfigured; call enclave.setCommitteeThresholds for committee
sizes 1 and 2 (Small and Medium) with appropriate threshold arrays (e.g., [2,3]
and [2,4]) alongside the existing enclave.setCommitteeThresholds(0, [1,3]) call
so all committee sizes are configured.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4af0c981-b5d8-49e0-afe7-1e82dc158144
📒 Files selected for processing (10)
circuits/lib/src/configs/insecure/dkg.nrcircuits/lib/src/configs/secure/dkg.nrpackages/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/test/E3Lifecycle/E3Integration.spec.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts
✅ Files skipped from review due to trivial changes (3)
- packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json
- packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json
- packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/zk-helpers/src/bin/zk_cli.rs (1)
175-177:⚠️ Potential issue | 🟡 MinorCLI help text is incomplete.
The help text says
"micro" or "small"butparse_committeenow accepts all four variants including"medium"and"large".📝 Proposed fix
- /// Committee size: "micro" or "small". + /// Committee size: "micro", "small", "medium", or "large". #[arg(long, default_value = "micro")] committee: String,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/bin/zk_cli.rs` around lines 175 - 177, Update the CLI help for the committee argument to reflect all supported variants; replace the current docstring on the committee field (the #[arg(long, default_value = "micro")] committee: String) that says `"micro" or "small"` with text listing all four accepted values (e.g., "micro, small, medium, or large") and/or a brief hint that the value is parsed by parse_committee so users know all valid options.
🤖 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 27-28: The E3_COMMITTEE_SIZE range check should be moved from
runtime to startup: in Config::from_env() validate that the deserialized u8
E3_COMMITTEE_SIZE is within 0..=3 and return an explicit configuration error if
not, so invalid values (e.g., 99) fail to load; remove or keep defensive checks
in routes/rounds.rs and cli/commands.rs as assertions only, but rely on
Config::from_env() to enforce the canonical range and produce a clear error
message referencing E3_COMMITTEE_SIZE when constructing the Config.
---
Duplicate comments:
In `@crates/zk-helpers/src/bin/zk_cli.rs`:
- Around line 175-177: Update the CLI help for the committee argument to reflect
all supported variants; replace the current docstring on the committee field
(the #[arg(long, default_value = "micro")] committee: String) that says `"micro"
or "small"` with text listing all four accepted values (e.g., "micro, small,
medium, or large") and/or a brief hint that the value is parsed by
parse_committee so users know all valid options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8e5b805-b270-4554-88f6-b752723194f6
📒 Files selected for processing (18)
crates/evm/src/enclave_sol_reader.rscrates/keyshare/src/threshold_keyshare.rscrates/tests/tests/integration.rscrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/ciphernodes_committee.rscrates/zk-prover/src/circuits/recursive_aggregation/mod.rscrates/zk-prover/tests/local_e2e_tests.rsexamples/CRISP/server/.env.exampleexamples/CRISP/server/src/cli/commands.rspackages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/DkgPkVerifier.jsonpackages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/ZKTranscriptLib.jsonpackages/enclave-contracts/contracts/verifier/DkgPkVerifier.solpackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tstemplates/default/client/src/pages/steps/RequestComputation.tsxtemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/enclave-contracts/contracts/verifier/DkgPkVerifier.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/default/client/src/pages/steps/RequestComputation.tsx
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.
fix #1335
Summary by CodeRabbit
New Features
Updates