chore: prod switch [skip-line-limit]#1513
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 6 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThreads a BFV parameter preset (BfvPreset) through events, actor/state structs, prover APIs, circuit build scripts, tests, and smart-contract gates so proof generation/verification resolve circuit artifacts via a preset-derived artifacts_dir. Changes
Sequence Diagram(s)sequenceDiagram
participant ShareActor as ShareVerificationActor
participant ZkActor as ZkActor
participant Prover as ZkProver
ShareActor->>ShareActor: capture params_preset from event
ShareActor->>ZkActor: Send VerifyShareProofsRequest(proofs..., params_preset)
ZkActor->>ZkActor: artifacts_dir = params_preset.artifacts_dir()
ZkActor->>Prover: verify_proof(proof, e3_id, party_id, artifacts_dir)
Prover-->>ZkActor: verification result
ZkActor-->>ShareActor: publish verification outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/keyshare/src/threshold_keyshare.rs (1)
1417-1425:⚠️ Potential issue | 🟠 MajorDispatch the threshold preset here, not
share_enc_preset.Both verification publishes send the actor’s DKG preset, but the corresponding proof requests in this file are built with
threshold_counterpart(). That makes proof generation and verification disagree on the preset, and C4 can break as soon as downstream code uses the actual preset value instead of the sharedartifacts_dir()name. Based on learnings, the share-decryption circuit codegen only accepts threshold BFV presets, not DKG presets.Also applies to: 2045-2053
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_keyshare.rs` around lines 1417 - 1425, The publish is using the wrong preset: replace share_enc_preset with the actor’s threshold preset so verification uses the same preset used to build proofs (use threshold_counterpart()’s preset/field rather than share_enc_preset) in the ShareVerificationDispatched for VerificationKind::ShareProofs (the block that constructs share_proofs/decryption_proofs with e3_id and party_proofs_to_verify); make the same change in the corresponding publish at the other location (around the 2045-2053 block) so proof generation and verification agree on the threshold BFV preset.crates/aggregator/src/proof_fold.rs (1)
22-46:⚠️ Potential issue | 🟠 MajorAdd a serde default for the new persisted preset.
Line 33 makes
params_presetmandatory on a state object that's explicitly persisted for restart recovery. AnyProofFoldStateserialized before this change will fail to deserialize after deploy/restart, which breaks the recovery path this type exists for.🛠️ Suggested fix
+fn default_params_preset() -> BfvPreset { + BfvPreset::default() +} + -#[derive(Clone, Debug, Default, serde::Serialize, serde::Deserialize)] +#[derive(Clone, Debug, serde::Serialize, serde::Deserialize)] pub struct ProofFoldState { @@ - params_preset: BfvPreset, + #[serde(default = "default_params_preset")] + params_preset: BfvPreset, } + +impl Default for ProofFoldState { + fn default() -> Self { + Self::new() + } +} @@ - params_preset: BfvPreset::default(), + params_preset: default_params_preset(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/proof_fold.rs` around lines 22 - 46, ProofFoldState's new non-optional field params_preset breaks deserialization of older persisted state; mark params_preset with serde default so missing field deserializes to BfvPreset::default(). Concretely, add a serde default for the params_preset field (or apply #[serde(default)] at the struct level) so ProofFoldState::deserialize will populate params_preset using BfvPreset::default() when the field is absent; keep the existing Default implementation for ProofFoldState unchanged.
🧹 Nitpick comments (2)
crates/tests/tests/integration.rs (1)
881-893: Keep an aggregation-enabled end-to-end case around.Hardcoding
proof_aggregation_enabled = falsemakes everyif proof_aggregation_enabled { ... }assertion below dead in this test, so the wrapper/fold path added in this PR stops being exercised here. I’d split this into two cases, or parameterize the flag, so both branches keep coverage.🤖 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 881 - 893, The test currently hardcodes proof_aggregation_enabled = false causing all branches that run when aggregation is enabled to be untested; update the test around the E3Requested construction to exercise both branches by either adding a second test case with proof_aggregation_enabled = true (keeping the existing false case) or parameterizing the test to run twice with proof_aggregation_enabled true/false, ensuring the E3Requested instance (and subsequent assertions that check if proof_aggregation_enabled lead to wrapper/fold behavior) are executed for the true case so the aggregation-enabled paths are covered.crates/zk-prover/src/traits.rs (1)
55-85: Consider replacing the free-formartifacts_dirwith a typed proof context.Every caller now has to keep
params/proofcontext andartifacts_dirin sync manually. Wrapping those together—or deriving the directory inside this trait for param types that know it—would remove a runtime-only mismatch class and cut down a lot of repetitive plumbing across the call sites.Also applies to: 106-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/traits.rs` around lines 55 - 85, The trait's methods prove and prove_with_variant currently accept a free-form artifacts_dir: &str which forces callers to keep params/inputs and artifact paths in sync; introduce a typed context (e.g., an associated type ProofContext or a struct ProofContext { params: Self::Params, artifacts_dir: PathBuf, /* maybe input or id */ }) and replace the artifacts_dir parameter with that context in both fn signatures (or add a helper method on Params to derive the artifacts dir, e.g., Params::artifact_dir(&self) -> PathBuf), update trait bounds accordingly, and adjust implementations to use the typed ProofContext (or Params::artifact_dir) so callers pass a single coherent object instead of separate params and artifacts_dir strings; refer to the trait methods prove and prove_with_variant to locate where to change signatures and usages.
🤖 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/fhe-params/src/presets.rs`:
- Around line 385-390: The fixtures loader was not updated for the new
artifacts_dir() layout: update setup_test_zk_backend() in
crates/tests/tests/integration.rs so it places/looks up fixtures under
circuits/{preset.artifacts_dir()}/{variant}/... (or compute
metadata().as_config_str()/degree via Preset::artifacts_dir()) instead of the
old circuits/{recursive,default,evm}/... paths; ensure the same prefix logic
used by crates/zk-prover/src/prover.rs (which expects
circuits/<artifacts_dir>/<variant>/...) is applied when copying test fixtures so
local bb fixtures are mirrored under insecure-512/ or secure-8192/ as produced
by artifacts_dir().
In `@crates/zk-prover/src/actors/proof_verification.rs`:
- Around line 148-153: The code must not call unwrap_or_default() on
self.presets when computing artifacts_dir for msg.e3_id because that can return
a wrong VK path after actor restart; instead check for the preset explicitly
(e.g. if let Some(preset) = self.presets.get(&msg.e3_id)) and use
preset.copied().artifacts_dir(), otherwise handle the missing entry by either
using a directory provided by the incoming message or deferring/aborting
verification (do not guess); update the logic around
self.presets.get(&msg.e3_id), .copied(), and artifacts_dir() accordingly so C0
verification uses only a confirmed artifact set (and ensure this ties into
replaying CiphernodeSelected where appropriate).
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 415-423: The current branch sets success = true when
e3.proofAggregationEnabled is false, which skips plaintext verification; change
it to always call e3.decryptionVerifier.verify(...) for the plaintext proof and
only treat foldProof as optional. Specifically, replace the unconditional
success=true with a call to
e3.decryptionVerifier.verify(keccak256(plaintextOutput), proof, foldProof) (or a
variant/overload that accepts an empty/optional foldProof) and keep
require(success, InvalidOutput(plaintextOutput)); referenced symbols:
e3.proofAggregationEnabled, e3.decryptionVerifier.verify, plaintextOutput,
proof, foldProof, InvalidOutput.
In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Around line 222-225: publishCommittee currently skips verification when
e3.proofAggregationEnabled is false, allowing unverified publicKeyHash to be
published; change the logic so verification always occurs before committing
publicKeyHash and advancing lifecycle: in publishCommittee, after fetching E3
via enclave.getE3(e3Id) ensure either (A) require(e3.proofAggregationEnabled,
"aggregation disabled") to forbid publishing when aggregation is off, or (B)
call e3.pkVerifier.verify(proof, foldProof) unconditionally (and only write
e3.publicKeyHash and advance lifecycle after that call). Make sure to reference
E3 e3, e3.proofAggregationEnabled, e3.pkVerifier.verify, publishCommittee,
publicKeyHash, proof and foldProof so the verification decision and state
mutation are co-located and atomic.
In `@scripts/build-circuits.ts`:
- Around line 101-103: buildAll currently mutates the tracked file at modNrPath
(circuits/lib/src/configs/.../mod.nr) and restores originalModNr, which is
process-global and races between parallel runs; change the implementation to
perform each preset build in an isolated workspace: for each preset in
buildAll(), create a temporary working copy (e.g., copy the entire circuits tree
or at minimum the configs/default folder) and write the preset-specific mod.nr
into that temp copy, then invoke the compiler/tooling pointing at that temp
workspace (rather than rewriting modNrPath) and delete the temp when done;
update uses around modNrPath/originalModNr (and the similar blocks at the other
noted ranges) to operate on the temp copy so builds are local and non-mutating.
- Line 38: The preset field is currently treated as a plain string and can be
overridden to undefined/invalid when merging options, which later produces
invalid generated code; change the preset variable/type to CircuitPreset,
validate the CLI/parse boundary (where --preset is read in buildCircuits/main or
the options parser) and reject unknown values (exit/error) before any codegen or
before calling rewriteModNr, and when merging defaults with options ensure you
do not let undefined override defaults (use explicit preset = providedPreset ??
defaultPreset or spread defaults last). Ensure all uses (rewriteModNr, options
merging, and constructor call sites) expect CircuitPreset so invalid strings
never flow into codegen.
---
Outside diff comments:
In `@crates/aggregator/src/proof_fold.rs`:
- Around line 22-46: ProofFoldState's new non-optional field params_preset
breaks deserialization of older persisted state; mark params_preset with serde
default so missing field deserializes to BfvPreset::default(). Concretely, add a
serde default for the params_preset field (or apply #[serde(default)] at the
struct level) so ProofFoldState::deserialize will populate params_preset using
BfvPreset::default() when the field is absent; keep the existing Default
implementation for ProofFoldState unchanged.
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 1417-1425: The publish is using the wrong preset: replace
share_enc_preset with the actor’s threshold preset so verification uses the same
preset used to build proofs (use threshold_counterpart()’s preset/field rather
than share_enc_preset) in the ShareVerificationDispatched for
VerificationKind::ShareProofs (the block that constructs
share_proofs/decryption_proofs with e3_id and party_proofs_to_verify); make the
same change in the corresponding publish at the other location (around the
2045-2053 block) so proof generation and verification agree on the threshold BFV
preset.
---
Nitpick comments:
In `@crates/tests/tests/integration.rs`:
- Around line 881-893: The test currently hardcodes proof_aggregation_enabled =
false causing all branches that run when aggregation is enabled to be untested;
update the test around the E3Requested construction to exercise both branches by
either adding a second test case with proof_aggregation_enabled = true (keeping
the existing false case) or parameterizing the test to run twice with
proof_aggregation_enabled true/false, ensuring the E3Requested instance (and
subsequent assertions that check if proof_aggregation_enabled lead to
wrapper/fold behavior) are executed for the true case so the aggregation-enabled
paths are covered.
In `@crates/zk-prover/src/traits.rs`:
- Around line 55-85: The trait's methods prove and prove_with_variant currently
accept a free-form artifacts_dir: &str which forces callers to keep
params/inputs and artifact paths in sync; introduce a typed context (e.g., an
associated type ProofContext or a struct ProofContext { params: Self::Params,
artifacts_dir: PathBuf, /* maybe input or id */ }) and replace the artifacts_dir
parameter with that context in both fn signatures (or add a helper method on
Params to derive the artifacts dir, e.g., Params::artifact_dir(&self) ->
PathBuf), update trait bounds accordingly, and adjust implementations to use the
typed ProofContext (or Params::artifact_dir) so callers pass a single coherent
object instead of separate params and artifacts_dir strings; refer to the trait
methods prove and prove_with_variant to locate where to change signatures and
usages.
🪄 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: 8620de8d-794a-4eae-aabe-252db9ac20e1
📒 Files selected for processing (32)
crates/aggregator/src/proof_fold.rscrates/aggregator/src/publickey_aggregator.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/share_verification.rscrates/fhe-params/src/presets.rscrates/keyshare/src/threshold_keyshare.rscrates/multithread/src/multithread.rscrates/tests/tests/integration.rscrates/zk-prover/src/actors/accusation_manager.rscrates/zk-prover/src/actors/accusation_manager_ext.rscrates/zk-prover/src/actors/node_proof_aggregator.rscrates/zk-prover/src/actors/proof_verification.rscrates/zk-prover/src/actors/share_verification.rscrates/zk-prover/src/actors/zk_actor.rscrates/zk-prover/src/circuits/recursive_aggregation/mod.rscrates/zk-prover/src/circuits/utils.rscrates/zk-prover/src/prover.rscrates/zk-prover/src/traits.rscrates/zk-prover/tests/backend_tests.rscrates/zk-prover/tests/local_e2e_tests.rspackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/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.tsscripts/build-circuits.tsscripts/circuit-constants.tstests/integration/persist.sh
cedoor
previously approved these changes
Apr 8, 2026
cedoor
left a comment
cedoor
left a comment
Contributor
There was a problem hiding this comment.
Besides CodeRabbit's comments, lgtm
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (1)
247-271:⚠️ Potential issue | 🟠 MajorKeep proof aggregation configurable in this fixture.
packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol:196-232only callse3.pkVerifier.verify(...)whene3.proofAggregationEnabledis true. Hard-codingfalsehere means everypublishCommittee()case in this spec now runs through the no-verification branch, so a broken C5 verifier integration would still pass. Make this flag explicit per test instead of changing the shared default.Suggested change
async function makeRequest( enclave: any, usdcToken: any, mockE3Program: any, mockDecryptionVerifier: any, - signer?: Signer, + { + signer, + proofAggregationEnabled = true, + }: { signer?: Signer; proofAggregationEnabled?: boolean } = {}, ) { const abiCoder = ethers.AbiCoder.defaultAbiCoder(); const currentTime = await networkHelpers.time.latest(); const requestParams = { committeeSize: 0, inputWindow: [currentTime + 100, currentTime + 300] as [number, number], e3Program: await mockE3Program.getAddress(), paramSet: 0, computeProviderParams: abiCoder.encode( ["address"], [await mockDecryptionVerifier.getAddress()], ), customParams: abiCoder.encode( ["address"], ["0x1234567890123456789012345678901234567890"], ), - proofAggregationEnabled: false, + proofAggregationEnabled, };🤖 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 247 - 271, The fixture hard-codes requestParams.proofAggregationEnabled = false inside makeRequest, which forces the no-verification branch; change makeRequest to accept an optional proofAggregationEnabled parameter (or include it in a passed-in requestOverrides) and set requestParams.proofAggregationEnabled from that parameter instead of the literal false so individual tests can opt into true/false; update call sites in the spec to pass the desired boolean where tests need verification enabled.packages/enclave-contracts/tasks/enclave.ts (1)
107-111:⚠️ Potential issue | 🟡 MinorDon't keep
--e3Paramsas a silent no-op.
packages/enclave-contracts/contracts/interfaces/IEnclave.sol:418-431no longer has ane3Paramsfield, and therequestParamsobject here never uses this value. Renaming it to_e3Paramshides the mismatch but still acceptscommittee:new --e3Params ...without changing the request. Either remove the option or fail fast when callers pass it.Suggested change
{ committeeSize, inputWindowStart, inputWindowEnd, e3Address, - e3Params: _e3Params, + e3Params, computeParams, customParams, proofAggregationEnabled, }, hre, ) => { + if (e3Params !== ZeroAddress) { + throw new Error( + "--e3Params is no longer consumed by committee:new; remove it from the invocation.", + ); + } + if (![0, 1, 2, 3].includes(committeeSize)) {Also applies to: 131-148
🤖 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 107 - 111, The CLI option "e3Params" in packages/enclave-contracts/tasks/enclave.ts is a silent no-op (never used when building requestParams) and must either be removed or cause a hard failure when supplied; update the task that defines the "e3Params" argument (and the duplicate at the other block around lines 131-148) to either delete the addOptionalParam/addParam declaration for "e3Params" or add validation in the task action that checks the taskArgs.e3Params value and throws a clear error (e.g., "e3Params is no longer supported") if it differs from the default (ZeroAddress), and ensure requestParams remains unchanged.
🧹 Nitpick comments (1)
crates/zk-prover/src/actors/proof_verification.rs (1)
198-210: Consider cleanup of stale presets.The
presetsmap grows unboundedly as new E3s are processed. For long-running nodes, consider adding cleanup logic when an E3 completes or fails (e.g., listen forE3Completed/E3Failedevents).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/proof_verification.rs` around lines 198 - 210, The presets HashMap is never trimmed which causes unbounded growth; update the EnclaveEvent handler (handle method in proof_verification.rs) to also match EnclaveEventData::E3Completed and EnclaveEventData::E3Failed (or whatever completion/failure enum names exist) and call self.presets.remove(&data.e3_id) (or appropriate field) to drop the preset for that E3; ensure the removal branch is added alongside the existing CiphernodeSelected and EncryptionKeyReceived arms so presets are cleaned when an E3 finishes or fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/enclave-contracts/tasks/enclave.ts`:
- Around line 107-111: The CLI option "e3Params" in
packages/enclave-contracts/tasks/enclave.ts is a silent no-op (never used when
building requestParams) and must either be removed or cause a hard failure when
supplied; update the task that defines the "e3Params" argument (and the
duplicate at the other block around lines 131-148) to either delete the
addOptionalParam/addParam declaration for "e3Params" or add validation in the
task action that checks the taskArgs.e3Params value and throws a clear error
(e.g., "e3Params is no longer supported") if it differs from the default
(ZeroAddress), and ensure requestParams remains unchanged.
In `@packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts`:
- Around line 247-271: The fixture hard-codes
requestParams.proofAggregationEnabled = false inside makeRequest, which forces
the no-verification branch; change makeRequest to accept an optional
proofAggregationEnabled parameter (or include it in a passed-in
requestOverrides) and set requestParams.proofAggregationEnabled from that
parameter instead of the literal false so individual tests can opt into
true/false; update call sites in the spec to pass the desired boolean where
tests need verification enabled.
---
Nitpick comments:
In `@crates/zk-prover/src/actors/proof_verification.rs`:
- Around line 198-210: The presets HashMap is never trimmed which causes
unbounded growth; update the EnclaveEvent handler (handle method in
proof_verification.rs) to also match EnclaveEventData::E3Completed and
EnclaveEventData::E3Failed (or whatever completion/failure enum names exist) and
call self.presets.remove(&data.e3_id) (or appropriate field) to drop the preset
for that E3; ensure the removal branch is added alongside the existing
CiphernodeSelected and EncryptionKeyReceived arms so presets are cleaned when an
E3 finishes or fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06e25f6f-e212-4ad8-a382-5020be10c3c1
📒 Files selected for processing (8)
circuits/lib/src/configs/default/mod.nrcrates/tests/tests/integration.rscrates/zk-prover/src/actors/proof_verification.rspackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tsscripts/build-circuits.tstemplates/default/client/src/pages/steps/RequestComputation.tsxtemplates/default/tests/integration.spec.ts
💤 Files with no reviewable changes (2)
- templates/default/client/src/pages/steps/RequestComputation.tsx
- templates/default/tests/integration.spec.ts
✅ Files skipped from review due to trivial changes (1)
- circuits/lib/src/configs/default/mod.nr
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/build-circuits.ts
0xjei
previously approved these changes
Apr 8, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/zk-prover/tests/common/helpers.rs (1)
44-45: Parameterize the preset directory instead of hardcoding"insecure-512".Behavior is correct today, but hardcoding makes the later secure-params batch switch noisier and easier to miss in helpers.
♻️ Proposed refactor
-pub async fn setup_compiled_circuit(backend: &ZkBackend, group: &str, circuit_name: &str) { +pub async fn setup_compiled_circuit( + backend: &ZkBackend, + artifacts_dir: &str, + group: &str, + circuit_name: &str, +) { @@ - let preset_dir = backend.circuits_dir.join("insecure-512"); + let preset_dir = backend.circuits_dir.join(artifacts_dir);Based on learnings: tests intentionally use insecure BFV parameter presets consistently across the suite during the transition period.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/tests/common/helpers.rs` around lines 44 - 45, The line that hardcodes "insecure-512" when computing preset_dir should be parameterized: replace the literal in helpers.rs (where preset_dir is computed from backend.circuits_dir.join("insecure-512")) with a variable or constant passed into the helper; e.g., add a new parameter (preset_name or preset_dir_name) to the helper function that constructs/uses preset_dir (or introduce a crate-level const like DEFAULT_PRESET that tests can override), update all call sites to pass "insecure-512" for now, and use backend.circuits_dir.join(preset_name) instead of the literal so the preset can be switched centrally without editing helpers.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/zk-prover/tests/common/helpers.rs`:
- Around line 44-45: The line that hardcodes "insecure-512" when computing
preset_dir should be parameterized: replace the literal in helpers.rs (where
preset_dir is computed from backend.circuits_dir.join("insecure-512")) with a
variable or constant passed into the helper; e.g., add a new parameter
(preset_name or preset_dir_name) to the helper function that constructs/uses
preset_dir (or introduce a crate-level const like DEFAULT_PRESET that tests can
override), update all call sites to pass "insecure-512" for now, and use
backend.circuits_dir.join(preset_name) instead of the literal so the preset can
be switched centrally without editing helpers.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 503393a4-746e-45ee-b601-f48b78858755
📒 Files selected for processing (3)
crates/zk-prover/src/actors/proof_request.rscrates/zk-prover/tests/common/helpers.rscrates/zk-prover/tests/integration_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/zk-prover/tests/integration_tests.rs
cedoor
approved these changes
Apr 8, 2026
This was referenced May 22, 2026
This was referenced Jun 4, 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
Bug Fixes / Behavior Changes
Chores