refactor: remove c7 mod circuit [skip-line-limit]#1433
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (9)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConsolidates the DecryptedSharesAggregation circuit: removes the modular variant, renames the BigNum variant to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/README.md (1)
307-314:⚠️ Potential issue | 🟡 MinorUpdate the sample output counts to match the new 3-circuit list.
Line 307 and Line 313 still say
4, but this example now shows 3 generated verifier entries.📝 Proposed doc fix
- Found 4 circuit(s) + Found 3 circuit(s) ... -✅ Generated 4 Solidity verifier(s) in: +✅ Generated 3 Solidity verifier(s) in:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/README.md` around lines 307 - 314, Update the sample output counts to match the three listed circuits: change the "Found 4 circuit(s)" and "Generated 4 Solidity verifier(s) in:" occurrences to "3" so they match the three verifier entries (threshold/pk_aggregation → ThresholdPkAggregationVerifier.sol, threshold/decrypted_shares_aggregation → ThresholdDecryptedSharesAggregationVerifier.sol, recursive_aggregation/fold → RecursiveAggregationFoldVerifier.sol).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/pages/ciphernode-operators/index.mdx`:
- Around line 53-54: Update the sentence that currently references "C7 BN/Mod"
to reflect the consolidated circuit variant: remove the "BN/Mod" variant mention
and state that the DecryptedSharesAggregation circuit is deployed as a single
verifier; e.g., mention DecryptedSharesAggregation and/or
ThresholdDecryptedSharesAggregationVerifier (C7) so the text matches the table
which lists a single ThresholdDecryptedSharesAggregationVerifier (C7) entry.
---
Outside diff comments:
In `@scripts/README.md`:
- Around line 307-314: Update the sample output counts to match the three listed
circuits: change the "Found 4 circuit(s)" and "Generated 4 Solidity verifier(s)
in:" occurrences to "3" so they match the three verifier entries
(threshold/pk_aggregation → ThresholdPkAggregationVerifier.sol,
threshold/decrypted_shares_aggregation →
ThresholdDecryptedSharesAggregationVerifier.sol, recursive_aggregation/fold →
RecursiveAggregationFoldVerifier.sol).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa439438-c2ee-49e2-b1a1-40a588b9e1cb
📒 Files selected for processing (31)
circuits/benchmarks/config.jsoncircuits/benchmarks/scripts/generate_prover_toml.shcircuits/benchmarks/scripts/run_benchmarks.shcircuits/bin/threshold/Nargo.tomlcircuits/bin/threshold/decrypted_shares_aggregation/Nargo.tomlcircuits/bin/threshold/decrypted_shares_aggregation/src/main.nrcircuits/bin/threshold/decrypted_shares_aggregation_bn/README.mdcircuits/bin/threshold/decrypted_shares_aggregation_mod/Nargo.tomlcircuits/bin/threshold/decrypted_shares_aggregation_mod/README.mdcircuits/bin/threshold/decrypted_shares_aggregation_mod/src/main.nrcircuits/lib/src/core/threshold/decrypted_shares_aggregation.nrcrates/events/src/enclave_event/proof.rscrates/events/src/enclave_event/signed_proof.rscrates/tests/tests/integration.rscrates/zk-prover/src/circuits/threshold/decrypted_shares_aggregation.rscrates/zk-prover/tests/local_e2e_tests.rsdocs/pages/ciphernode-operators/index.mdxdocs/pages/noir-circuits.mdxpackage.jsonpackages/enclave-contracts/.gitignorepackages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationBnVerifier.sol/ThresholdDecryptedSharesAggregationBnVerifier.jsonpackages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationModVerifier.sol/ZKTranscriptLib.jsonpackages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol/ThresholdDecryptedSharesAggregationVerifier.jsonpackages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol/ZKTranscriptLib.jsonpackages/enclave-contracts/contracts/verifier/ThresholdDecryptedSharesAggregationModVerifier.solpackages/enclave-contracts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.solpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationModVerifier.tspackages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationVerifier.tsscripts/README.mdscripts/generate-verifiers.ts
💤 Files with no reviewable changes (8)
- circuits/benchmarks/scripts/run_benchmarks.sh
- circuits/bin/threshold/decrypted_shares_aggregation_mod/README.md
- packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationBnVerifier.sol/ThresholdDecryptedSharesAggregationBnVerifier.json
- circuits/bin/threshold/decrypted_shares_aggregation_bn/README.md
- packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationModVerifier.sol/ZKTranscriptLib.json
- circuits/bin/threshold/decrypted_shares_aggregation_mod/Nargo.toml
- circuits/bin/threshold/decrypted_shares_aggregation_mod/src/main.nr
- packages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationModVerifier.ts
7c5a5d5 to
b0d7b79
Compare
- Remove BN/Mod variant mention from circuit verifiers intro - State DecryptedSharesAggregation (C7) is deployed as single ThresholdDecryptedSharesAggregationVerifier - Align text with table listing one C7 verifier Made-with: Cursor
d603163 to
2a3693b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/zk-prover/tests/local_e2e_tests.rs (1)
294-294: Add one secure-preset smoke test for this renamed artifact.This helper still instantiates
BfvPreset::InsecureThreshold512, so the newdecrypted_shares_aggregationpath is only exercised in the former insecure-only configuration. A second E2E case using a secure threshold preset would protect the path that used to live behind_bn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/tests/local_e2e_tests.rs` at line 294, The test currently calls setup_compiled_circuit(&backend, "threshold", "decrypted_shares_aggregation").await but only instantiates BfvPreset::InsecureThreshold512, so add a second E2E test that calls the same setup_compiled_circuit with the "decrypted_shares_aggregation" artifact under a secure threshold preset (e.g., a BfvPreset::SecureThreshold or the appropriate secure variant used elsewhere), and ensure the secure preset is instantiated and passed to the backend before invoking setup_compiled_circuit so the secure path (previously behind `_bn`) is exercised.
🤖 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/local_e2e_tests.rs`:
- Line 294: The test currently calls setup_compiled_circuit(&backend,
"threshold", "decrypted_shares_aggregation").await but only instantiates
BfvPreset::InsecureThreshold512, so add a second E2E test that calls the same
setup_compiled_circuit with the "decrypted_shares_aggregation" artifact under a
secure threshold preset (e.g., a BfvPreset::SecureThreshold or the appropriate
secure variant used elsewhere), and ensure the secure preset is instantiated and
passed to the backend before invoking setup_compiled_circuit so the secure path
(previously behind `_bn`) is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef4db38a-c042-46f7-8ce4-d49e6f2c2636
📒 Files selected for processing (31)
circuits/benchmarks/config.jsoncircuits/benchmarks/scripts/generate_prover_toml.shcircuits/benchmarks/scripts/run_benchmarks.shcircuits/bin/threshold/Nargo.tomlcircuits/bin/threshold/decrypted_shares_aggregation/Nargo.tomlcircuits/bin/threshold/decrypted_shares_aggregation/src/main.nrcircuits/bin/threshold/decrypted_shares_aggregation_bn/README.mdcircuits/bin/threshold/decrypted_shares_aggregation_mod/Nargo.tomlcircuits/bin/threshold/decrypted_shares_aggregation_mod/README.mdcircuits/bin/threshold/decrypted_shares_aggregation_mod/src/main.nrcircuits/lib/src/core/threshold/decrypted_shares_aggregation.nrcrates/events/src/enclave_event/proof.rscrates/events/src/enclave_event/signed_proof.rscrates/tests/tests/integration.rscrates/zk-prover/src/circuits/threshold/decrypted_shares_aggregation.rscrates/zk-prover/tests/local_e2e_tests.rsdocs/pages/ciphernode-operators/index.mdxdocs/pages/noir-circuits.mdxpackage.jsonpackages/enclave-contracts/.gitignorepackages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationBnVerifier.sol/ThresholdDecryptedSharesAggregationBnVerifier.jsonpackages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationModVerifier.sol/ZKTranscriptLib.jsonpackages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol/ThresholdDecryptedSharesAggregationVerifier.jsonpackages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol/ZKTranscriptLib.jsonpackages/enclave-contracts/contracts/verifier/ThresholdDecryptedSharesAggregationModVerifier.solpackages/enclave-contracts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.solpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationModVerifier.tspackages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationVerifier.tsscripts/README.mdscripts/generate-verifiers.ts
💤 Files with no reviewable changes (8)
- packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationModVerifier.sol/ZKTranscriptLib.json
- packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationBnVerifier.sol/ThresholdDecryptedSharesAggregationBnVerifier.json
- circuits/bin/threshold/decrypted_shares_aggregation_mod/README.md
- circuits/bin/threshold/decrypted_shares_aggregation_bn/README.md
- circuits/bin/threshold/decrypted_shares_aggregation_mod/src/main.nr
- packages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationModVerifier.ts
- circuits/bin/threshold/decrypted_shares_aggregation_mod/Nargo.toml
- circuits/benchmarks/scripts/run_benchmarks.sh
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/events/src/enclave_event/signed_proof.rs
- crates/tests/tests/integration.rs
- circuits/benchmarks/config.json
- scripts/generate-verifiers.ts
- package.json
- docs/pages/noir-circuits.mdx
- docs/pages/ciphernode-operators/index.mdx
- packages/enclave-contracts/.gitignore
Closes #1432
Summary by CodeRabbit
Refactor
Documentation
Chores