Skip to content

refactor: remove c7 mod circuit [skip-line-limit]#1433

Merged
cedoor merged 5 commits into
mainfrom
refactor/remove-c6-mod-circuit
Mar 17, 2026
Merged

refactor: remove c7 mod circuit [skip-line-limit]#1433
cedoor merged 5 commits into
mainfrom
refactor/remove-c6-mod-circuit

Conversation

@cedoor

@cedoor cedoor commented Mar 16, 2026

Copy link
Copy Markdown
Contributor

Closes #1432

Summary by CodeRabbit

  • Refactor

    • Consolidated decrypted-shares-aggregation into a single unified circuit and verifier, removing separate BN/mod variants.
  • Documentation

    • Updated circuit listings, operator guides, and README entries to reflect the streamlined circuit and verifier name.
  • Chores

    • Updated verifier generation scripts, ignition modules, ignored artifacts, and local deployment metadata to match the consolidated verifier.

@cedoor cedoor requested a review from 0xjei March 16, 2026 19:51
@vercel

vercel Bot commented Mar 16, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
crisp Ready Ready Preview, Comment Mar 17, 2026 8:24am
enclave-docs Ready Ready Preview, Comment Mar 17, 2026 8:24am

Request Review

@coderabbitai

coderabbitai Bot commented Mar 16, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fd3c2327-cc74-460e-b626-24810d1b0a32

📥 Commits

Reviewing files that changed from the base of the PR and between 2a3693b and 931b97d.

📒 Files selected for processing (9)
  • packages/enclave-contracts/contracts/verifier/RecursiveAggregationFoldVerifier.sol
  • packages/enclave-contracts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol
  • packages/enclave-contracts/contracts/verifier/ThresholdPkAggregationVerifier.sol
  • packages/enclave-contracts/ignition/modules/recursiveAggregationFoldVerifier.ts
  • packages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationVerifier.ts
  • packages/enclave-contracts/ignition/modules/thresholdPkAggregationVerifier.ts
  • packages/enclave-contracts/test/fixtures/attestation.ts
  • packages/enclave-contracts/test/fixtures/operators.ts
  • scripts/README.md
💤 Files with no reviewable changes (2)
  • packages/enclave-contracts/test/fixtures/operators.ts
  • packages/enclave-contracts/test/fixtures/attestation.ts
✅ Files skipped from review due to trivial changes (2)
  • packages/enclave-contracts/ignition/modules/thresholdPkAggregationVerifier.ts
  • packages/enclave-contracts/ignition/modules/recursiveAggregationFoldVerifier.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • scripts/README.md

📝 Walkthrough

Walkthrough

Consolidates the DecryptedSharesAggregation circuit: removes the modular variant, renames the BigNum variant to decrypted_shares_aggregation, and updates all references across circuits, benchmarks, Rust code, Solidity verifiers, deployment metadata, scripts, and docs.

Changes

Cohort / File(s) Summary
Benchmark Config & Scripts
circuits/benchmarks/config.json, circuits/benchmarks/scripts/generate_prover_toml.sh, circuits/benchmarks/scripts/run_benchmarks.sh
Replaced separate _bn/_mod entries with single threshold/decrypted_shares_aggregation; updated get_zk_args mapping and removed mode-specific skips so circuit runs in both modes.
Noir workspace & bin manifests
circuits/bin/threshold/Nargo.toml, circuits/bin/threshold/decrypted_shares_aggregation/Nargo.toml
Removed workspace members for _bn and _mod, added unified decrypted_shares_aggregation; updated package name in binary manifest.
Noir circuit sources
circuits/lib/src/core/threshold/decrypted_shares_aggregation.nr, circuits/bin/threshold/decrypted_shares_aggregation/src/main.nr
Renamed public struct DecryptedSharesAggregationBigNumDecryptedSharesAggregation; removed DecryptedSharesAggregationModular variant; unified constructor/execute paths and imports.
Deleted modular circuit files
circuits/bin/threshold/decrypted_shares_aggregation_mod/..., circuits/bin/threshold/decrypted_shares_aggregation_bn/README.md
Removed modular variant files (Nargo.toml, README, src/main.nr) and trimmed BN README reference.
Rust enum / zk-prover
crates/events/src/enclave_event/proof.rs, crates/events/src/enclave_event/signed_proof.rs, crates/zk-prover/src/circuits/threshold/decrypted_shares_aggregation.rs
Consolidated DecryptedSharesAggregationBn/DecryptedSharesAggregationMod into single DecryptedSharesAggregation enum variant; updated as_str/group, ProofType circuit_names, removed resolve_circuit_name, simplified circuit/valid_circuits.
Tests & integration references
crates/zk-prover/tests/local_e2e_tests.rs, crates/tests/tests/integration.rs
Updated test artifact paths and expected circuit names to decrypted_shares_aggregation.
Solidity verifiers & artifacts
packages/enclave-contracts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol, packages/enclave-contracts/artifacts/.../*.json, packages/enclave-contracts/.gitignore
Renamed verifier contract to unified name; updated VK constants, public input counts, and artifact metadata; removed BN/Mod artifact files and adjusted .gitignore exclusions.
Deployment metadata & Ignition modules
packages/enclave-contracts/deployed_contracts.json, packages/enclave-contracts/ignition/modules/*
Refreshed deployed_contracts.json with consolidated verifier entries and new addresses/blockNumbers; removed Mod ignition module and added/renamed module for unified verifier.
Build scripts & docs
package.json, scripts/generate-verifiers.ts, scripts/README.md, docs/pages/noir-circuits.mdx, docs/pages/ciphernode-operators/index.mdx
Updated generate-verifiers scripts/help to reference unified circuit; docs updated to list single C7 verifier and workspace entries.
Misc artifacts removed/edited
packages/enclave-contracts/artifacts/.../ZKTranscriptLib.json, deleted BN artifact JSON files
Artifact JSONs updated/removed to reflect verifier consolidation and new buildInfo/source metadata.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

chore

Suggested reviewers

  • ctrlc03
  • 0xjei

Poem

🐰 I hopped through code, both BN and Mod,
I nudged them close and made one tidy pod.
One circuit now, no variants to fight,
Verifier aligned and artifacts light. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'refactor: remove c6 mod circuit' accurately describes the main change: consolidating the DecryptedSharesAggregation circuits by removing the modular variant and keeping only the BigNum variant (renamed to DecryptedSharesAggregation).
Linked Issues check ✅ Passed All requirements from #1432 are met: removed DecryptedSharesAggregationModular, renamed BigNum to DecryptedSharesAggregation, dropped decrypted_shares_aggregation_mod circuit, updated Rust code, Solidity verifiers, benchmarks, and documentation.
Out of Scope Changes check ✅ Passed All changes are in scope and directly related to consolidating the DecryptedSharesAggregation circuits. No extraneous modifications beyond the stated objective were introduced.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/remove-c6-mod-circuit
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Update 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

📥 Commits

Reviewing files that changed from the base of the PR and between 43735d7 and 7c5a5d5.

📒 Files selected for processing (31)
  • circuits/benchmarks/config.json
  • circuits/benchmarks/scripts/generate_prover_toml.sh
  • circuits/benchmarks/scripts/run_benchmarks.sh
  • circuits/bin/threshold/Nargo.toml
  • circuits/bin/threshold/decrypted_shares_aggregation/Nargo.toml
  • circuits/bin/threshold/decrypted_shares_aggregation/src/main.nr
  • circuits/bin/threshold/decrypted_shares_aggregation_bn/README.md
  • circuits/bin/threshold/decrypted_shares_aggregation_mod/Nargo.toml
  • circuits/bin/threshold/decrypted_shares_aggregation_mod/README.md
  • circuits/bin/threshold/decrypted_shares_aggregation_mod/src/main.nr
  • circuits/lib/src/core/threshold/decrypted_shares_aggregation.nr
  • crates/events/src/enclave_event/proof.rs
  • crates/events/src/enclave_event/signed_proof.rs
  • crates/tests/tests/integration.rs
  • crates/zk-prover/src/circuits/threshold/decrypted_shares_aggregation.rs
  • crates/zk-prover/tests/local_e2e_tests.rs
  • docs/pages/ciphernode-operators/index.mdx
  • docs/pages/noir-circuits.mdx
  • package.json
  • packages/enclave-contracts/.gitignore
  • packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationBnVerifier.sol/ThresholdDecryptedSharesAggregationBnVerifier.json
  • packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationModVerifier.sol/ZKTranscriptLib.json
  • packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol/ThresholdDecryptedSharesAggregationVerifier.json
  • packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol/ZKTranscriptLib.json
  • packages/enclave-contracts/contracts/verifier/ThresholdDecryptedSharesAggregationModVerifier.sol
  • packages/enclave-contracts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol
  • packages/enclave-contracts/deployed_contracts.json
  • packages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationModVerifier.ts
  • packages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationVerifier.ts
  • scripts/README.md
  • scripts/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

Comment thread docs/pages/ciphernode-operators/index.mdx Outdated
@cedoor cedoor requested a review from ctrlc03 March 16, 2026 20:06
@cedoor cedoor force-pushed the refactor/remove-c6-mod-circuit branch from 7c5a5d5 to b0d7b79 Compare March 16, 2026 20:06
@cedoor cedoor changed the title refactor: remove c6 mod circuit refactor: remove c6 mod circuit [skip-size-limit] Mar 16, 2026
@vercel vercel Bot temporarily deployed to Preview – crisp March 16, 2026 20:08 Inactive
@cedoor cedoor changed the title refactor: remove c6 mod circuit [skip-size-limit] refactor: remove c6 mod circuit [skip-line-limit] Mar 16, 2026
ctrlc03
ctrlc03 previously approved these changes Mar 16, 2026

@ctrlc03 ctrlc03 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

ctrlc03
ctrlc03 previously approved these changes Mar 16, 2026
cedoor added 4 commits March 17, 2026 08:59
- 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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 new decrypted_shares_aggregation path 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7c5a5d5 and 2a3693b.

📒 Files selected for processing (31)
  • circuits/benchmarks/config.json
  • circuits/benchmarks/scripts/generate_prover_toml.sh
  • circuits/benchmarks/scripts/run_benchmarks.sh
  • circuits/bin/threshold/Nargo.toml
  • circuits/bin/threshold/decrypted_shares_aggregation/Nargo.toml
  • circuits/bin/threshold/decrypted_shares_aggregation/src/main.nr
  • circuits/bin/threshold/decrypted_shares_aggregation_bn/README.md
  • circuits/bin/threshold/decrypted_shares_aggregation_mod/Nargo.toml
  • circuits/bin/threshold/decrypted_shares_aggregation_mod/README.md
  • circuits/bin/threshold/decrypted_shares_aggregation_mod/src/main.nr
  • circuits/lib/src/core/threshold/decrypted_shares_aggregation.nr
  • crates/events/src/enclave_event/proof.rs
  • crates/events/src/enclave_event/signed_proof.rs
  • crates/tests/tests/integration.rs
  • crates/zk-prover/src/circuits/threshold/decrypted_shares_aggregation.rs
  • crates/zk-prover/tests/local_e2e_tests.rs
  • docs/pages/ciphernode-operators/index.mdx
  • docs/pages/noir-circuits.mdx
  • package.json
  • packages/enclave-contracts/.gitignore
  • packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationBnVerifier.sol/ThresholdDecryptedSharesAggregationBnVerifier.json
  • packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationModVerifier.sol/ZKTranscriptLib.json
  • packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol/ThresholdDecryptedSharesAggregationVerifier.json
  • packages/enclave-contracts/artifacts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol/ZKTranscriptLib.json
  • packages/enclave-contracts/contracts/verifier/ThresholdDecryptedSharesAggregationModVerifier.sol
  • packages/enclave-contracts/contracts/verifier/ThresholdDecryptedSharesAggregationVerifier.sol
  • packages/enclave-contracts/deployed_contracts.json
  • packages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationModVerifier.ts
  • packages/enclave-contracts/ignition/modules/thresholdDecryptedSharesAggregationVerifier.ts
  • scripts/README.md
  • scripts/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

@0xjei 0xjei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@cedoor cedoor enabled auto-merge (squash) March 17, 2026 08:49
@cedoor cedoor merged commit 09f8a3a into main Mar 17, 2026
29 checks passed
@cedoor cedoor changed the title refactor: remove c6 mod circuit [skip-line-limit] refactor: remove c7 mod circuit [skip-line-limit] Mar 17, 2026
@github-actions github-actions Bot deleted the refactor/remove-c6-mod-circuit branch March 25, 2026 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consolidate DecryptedSharesAggregation: use only BigNum circuit

3 participants