refactor: pass public key commitment [skip-line-limit]#1142
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughAggregator now computes a 32-byte public-key commitment (Greco conversion) and includes it in PublicKeyAggregated; this commitment is exposed via WASM/SDK, propagated to indexer and EVM registry, added to contracts/ABIs, and tests/clients updated; Enclave.activate no longer accepts raw public keys. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Aggregator
participant BFVHelpers as BFV‑Helpers (WASM/Rust)
participant Indexer
participant Registry as CiphernodeRegistry
participant Enclave
Aggregator->>BFVHelpers: compute_pk_commitment(pubkey, degree, plaintext_modulus, moduli)
Note right of BFVHelpers `#d6f5d6`: deserialize BFV PK → Greco\ncompute bigint commitment → 32‑byte hash
BFVHelpers-->>Aggregator: public_key_hash (32 bytes)
Aggregator->>Indexer: emit PublicKeyAggregated(pubkey, nodes, public_key_hash)
Indexer->>Registry: publishCommittee(e3Id, nodes, publicKey, public_key_hash)
Registry-->>Indexer: txReceipt
Note right of Registry `#f0e6d6`: stores publicKeyHash in registry state
Indexer->>Enclave: activate(e3Id)
Enclave->>Registry: committeePublicKey(e3Id)
Registry-->>Enclave: publicKeyHash
Enclave-->>Indexer: activation receipt / E3Activated(event committeePublicKey = hash)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
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 |
cedoor
force-pushed
the
refactor/public-key
branch
from
d902dc3 to
95653a5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/contract-client.ts (1)
160-163: Update JSDoc to reflect the new parameter.The JSDoc comment still shows the old contract signature without the
publicKeyHashparameter. This creates a mismatch between documentation and implementation.🔎 Proposed fix to update the JSDoc comment
/** * Activate an E3 computation - * activate(uint256 e3Id, bytes memory publicKey) + * activate(uint256 e3Id, bytes memory publicKey, bytes32 publicKeyHash) */packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json (1)
340-367: Breaking change toactivatefunction signature requires documentation update.The
activatefunction now requires a third parameterpublicKeyHash(bytes32). All code callers have been updated correctly, but the documentation indocs/pages/building-with-enclave.mdx(lines 77-79) still shows the old signature with only two parameters. Update the documentation to reflect the new function signature:function activate( uint256 e3Id, bytes calldata publicKey, bytes32 publicKeyHash ) external
🧹 Nitpick comments (5)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
283-284: Code changes correctly implement the new public inputs structure.The indexing changes are accurate and consistent:
proof.publicInputs[0]: public key commitment (now being passed tosetCommitteePublicKeyin tests)proof.publicInputs[1]: slot address (correctly extracted for Solidity encoding)proof.publicInputs.slice(3): vote data (correctly extracted from index 3 onward)This structure is confirmed by the circuit definition (main.nr with
pk_commitment,slot_address, andis_first_voteas public inputs) and verified across all integration points: proof generation, JavaScript verification, and Solidity contract verification.Consider adding a documentation comment above line 283 explaining the public inputs structure for clarity and add bounds validation before accessing array indices as a defensive coding practice:
// Public inputs: [pkCommitment, slotAddress, isFirstVote, ...voteData] if (proof.publicInputs.length < 4) { throw new Error(`Invalid proof: expected at least 4 public inputs, got ${proof.publicInputs.length}`) } const vote = proof.publicInputs.slice(3) as `0x${string}`[] const slotAddress = getAddress(numberToHex(BigInt(proof.publicInputs[1]), { size: 20 }))examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (1)
20-22: Clarify parameter naming:publicKeyHashvscommitteePublicKey.The parameter is named
publicKeyHashbut the storage variable iscommitteePublicKey. Are these semantically identical? Consider aligning the parameter name with the variable name to avoid confusion.🔎 Suggested refactor for naming consistency
- function setCommitteePublicKey(bytes32 publicKeyHash) external { - committeePublicKey = publicKeyHash; + function setCommitteePublicKey(bytes32 _committeePublicKey) external { + committeePublicKey = _committeePublicKey;crates/bfv-helpers/Cargo.toml (1)
17-22: Consider pinninggrecoto the same revision for consistency.The
zkfhe-shareddependency is pinned to a specific revision, butgrecofrom the same repository (zkfhe-generator) uses a floating git reference. If these packages need to stay in sync, this inconsistency could lead to version mismatches or build reproducibility issues.🔎 Suggested fix
-greco = { package = "zkfhe-greco", git = "https://github.com/gnosisguild/zkfhe-generator" } +greco = { package = "zkfhe-greco", git = "https://github.com/gnosisguild/zkfhe-generator", rev = "e2c02e6ef42348b2141293cc75f61a211c801a94" }packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
256-260: Naming inconsistency:c.publicKeystorespublicKeyHash, not the public key.The field
c.publicKey(typebytes32) now storespublicKeyHashrather than a hash of the public key computed internally. This naming is misleading. Additionally, storing the same value in bothc.publicKeyandpublicKeyHashes[e3Id]appears redundant.Consider renaming
c.publicKeytoc.publicKeyHashin theCommitteestruct for clarity, or consolidate the storage.crates/events/src/enclave_event/publickey_aggregated.rs (1)
17-22: Consider adding hex formatting forpublic_key_hashdebug output.The
pubkeyfield useshexfformatter for readable debug output, butpublic_key_hashwill display as a decimal byte array. For consistency and improved debugging experience, consider adding the same formatter.🔎 Suggested change
#[derive(Derivative, Message, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] #[derivative(Debug)] #[rtype(result = "()")] pub struct PublicKeyAggregated { #[derivative(Debug(format_with = "e3_utils::formatters::hexf"))] pub pubkey: Vec<u8>, + #[derivative(Debug(format_with = "e3_utils::formatters::hexf"))] pub public_key_hash: [u8; 32], pub e3_id: E3id, pub nodes: OrderedSet<String>, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
crates/aggregator/Cargo.tomlcrates/aggregator/src/publickey_aggregator.rscrates/bfv-helpers/Cargo.tomlcrates/bfv-helpers/src/client.rscrates/bfv-helpers/src/utils/greco.rscrates/events/src/enclave_event/publickey_aggregated.rscrates/evm-helpers/src/contracts.rscrates/evm/src/ciphernode_registry_sol.rscrates/tests/Cargo.tomlcrates/tests/tests/integration_legacy.rsexamples/CRISP/circuits/src/main.nrexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solexamples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.solexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/server/src/cli/approve.rsexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/indexer.rsexamples/CRISP/server/src/server/token_holders/etherscan.rspackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-sdk/src/contract-client.tspackages/enclave-sdk/src/enclave-sdk.ts
🧰 Additional context used
🧠 Learnings (25)
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
examples/CRISP/server/src/server/token_holders/etherscan.rsexamples/CRISP/server/src/cli/approve.rscrates/evm-helpers/src/contracts.rscrates/tests/tests/integration_legacy.rsexamples/CRISP/server/src/cli/commands.rscrates/aggregator/src/publickey_aggregator.rscrates/bfv-helpers/src/utils/greco.rsexamples/CRISP/server/src/server/indexer.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
examples/CRISP/server/src/server/token_holders/etherscan.rscrates/tests/Cargo.toml
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
examples/CRISP/server/src/server/token_holders/etherscan.rspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tscrates/tests/tests/integration_legacy.rscrates/bfv-helpers/src/utils/greco.rscrates/bfv-helpers/src/client.rs
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
examples/CRISP/server/src/server/token_holders/etherscan.rscrates/tests/Cargo.tomlcrates/aggregator/Cargo.toml
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
examples/CRISP/server/src/server/token_holders/etherscan.rscrates/evm-helpers/src/contracts.rscrates/tests/tests/integration_legacy.rsexamples/CRISP/server/src/cli/commands.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2025-10-04T14:18:45.636Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 660
File: crates/support-scripts/src/program_dev.rs:17-18
Timestamp: 2025-10-04T14:18:45.636Z
Learning: In crates/support-scripts/src/program_dev.rs, the #[allow(dead_code)] attribute on ProgramSupportDev is intentionally kept even though the struct appears to be referenced elsewhere, as confirmed by the maintainer.
Applied to files:
examples/CRISP/server/src/cli/approve.rs
📚 Learning: 2024-10-12T10:24:07.572Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-12T10:24:07.572Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Applied to files:
crates/events/src/enclave_event/publickey_aggregated.rspackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solcrates/tests/tests/integration_legacy.rscrates/aggregator/src/publickey_aggregator.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-29T01:04:19.137Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:53-70
Timestamp: 2024-10-29T01:04:19.137Z
Learning: In the `Keyshare` struct within `packages/ciphernode/keyshare/src/keyshare.rs`, the `self.secret` field is stored in encrypted form and is not considered sensitive.
Applied to files:
crates/events/src/enclave_event/publickey_aggregated.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
Applied to files:
crates/evm-helpers/src/contracts.rs
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 752
File: packages/enclave-contracts/contracts/Enclave.sol:15-17
Timestamp: 2025-09-19T11:16:53.825Z
Learning: The Enclave contract in the gnosisguild/enclave repository has not been deployed yet as of September 2025, so storage layout considerations for upgradeable contracts don't apply to current changes.
Applied to files:
crates/evm-helpers/src/contracts.rs
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
crates/evm-helpers/src/contracts.rs
📚 Learning: 2025-11-12T10:08:30.720Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 996
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:144-169
Timestamp: 2025-11-12T10:08:30.720Z
Learning: In the CRISP program (examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol), there are plans to move the merkle tree from the Enclave contract to the program contract itself in the future, which would allow direct calls to validateInput to be stored in the merkle tree.
Applied to files:
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2025-11-05T14:12:57.814Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 963
File: examples/CRISP/client/package.json:25-25
Timestamp: 2025-11-05T14:12:57.814Z
Learning: In the Enclave/CRISP codebase, `enclave-e3/sdk` and `crisp-e3/sdk` are different packages: `enclave-e3/sdk` is the general Enclave SDK, while `crisp-e3/sdk` is the CRISP-specific SDK. The CRISP client (`examples/CRISP/client`) intentionally depends on `enclave-e3/sdk`, not `crisp-e3/sdk`.
Applied to files:
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts
📚 Learning: 2024-10-16T09:52:53.807Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/src/data_store.rs:74-75
Timestamp: 2024-10-16T09:52:53.807Z
Learning: In this project, the actor model handles potential errors internally, so methods like `checkpoint` in the `Checkpoint` trait do not need to explicitly handle or propagate errors.
Applied to files:
crates/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
crates/tests/tests/integration_legacy.rscrates/bfv-helpers/src/utils/greco.rs
📚 Learning: 2025-09-11T13:21:31.031Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/tasks/utils.ts:7-8
Timestamp: 2025-09-11T13:21:31.031Z
Learning: In Hardhat v3, the task API syntax has changed significantly from v2. The new syntax uses:
- `.addOption({ name, description, defaultValue, type })` instead of `.addOptionalParam()`
- `.setAction(async () => ({ default: (args, hre) => { ... } }))` instead of direct `.setAction((args, hre) => { ... })`
- `.build()` is required to finalize task definitions
- `ArgumentType.STRING` is used for option types instead of `types.string`
Applied to files:
packages/enclave-contracts/tasks/enclave.ts
📚 Learning: 2025-09-11T13:09:03.800Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/scripts/deployAndSave/naiveRegistryFilter.ts:30-31
Timestamp: 2025-09-11T13:09:03.800Z
Learning: In Hardhat v3 deployment scripts, use `(await signer.provider?.getNetwork())?.name ?? "localhost"` instead of `hre.globalOptions.network` to reliably get the network name, as `hre.globalOptions.network` can be undefined in some contexts.
Applied to files:
packages/enclave-contracts/tasks/enclave.ts
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
crates/bfv-helpers/src/utils/greco.rs
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed to support multiple network connections: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3. Similarly, `.revertedWithoutReason(ethers)` replaces `.revertedWithoutReason`.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2024-10-28T12:04:26.578Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave_node/src/ciphernode.rs:26-28
Timestamp: 2024-10-28T12:04:26.578Z
Learning: In the `setup_ciphernode` function in `packages/ciphernode/enclave_node/src/ciphernode.rs`, panicking on errors during setup is acceptable.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/bfv-helpers/src/client.rs
📚 Learning: 2024-10-08T07:15:06.690Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/evm/src/ciphernode_registry_sol.rs:133-143
Timestamp: 2024-10-08T07:15:06.690Z
Learning: In `packages/ciphernode/evm/src/ciphernode_registry_sol.rs`, the function `helpers::stream_from_evm` in Rust returns `()`, not a `Result`, so error handling with `if let Err(e) = ...` is not applicable.
Applied to files:
crates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2025-08-29T16:46:50.289Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 666
File: examples/CRISP/contracts/CRISPChecker.sol:9-9
Timestamp: 2025-08-29T16:46:50.289Z
Learning: In hashcloak/semaphore-contracts-noir package, the interface is still named ISemaphore, not ISemaphoreNoir. The Noir support was added via Noir verifier contracts while keeping the original ISemaphore interface name.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2025-12-19T11:35:43.204Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 1109
File: examples/CRISP/server/src/server/routes/state.rs:33-37
Timestamp: 2025-12-19T11:35:43.204Z
Learning: In the CRISP voting system (examples/CRISP), ciphertext data and slot states are public and stored on-chain in the CRISPProgram smart contract. The server endpoints `/previous-ciphertext` and `/is-slot-empty` expose public blockchain data that can already be accessed by anyone through the contract's external view functions (getSlotIndex, isSlotEmptyByAddress) or by reading contract state directly. Authentication is not needed for these endpoints since they only expose data that is already publicly available on the blockchain.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
🧬 Code graph analysis (6)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (1)
examples/CRISP/packages/crisp-contracts/tests/utils.ts (2)
deployMockEnclave(43-47)deployCRISPProgram(69-87)
crates/tests/tests/integration_legacy.rs (2)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)crates/trbfv/src/trbfv_config.rs (1)
params(34-36)
examples/CRISP/server/src/cli/commands.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
crates/aggregator/src/publickey_aggregator.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
examples/CRISP/server/src/server/indexer.rs (2)
examples/CRISP/crates/crisp-constants/src/lib.rs (1)
get_default_paramset(10-13)crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
crates/bfv-helpers/src/client.rs (3)
crates/bfv-helpers/src/utils/greco.rs (1)
bfv_public_key_to_greco(233-283)crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_arc(278-300)crates/utils/src/utility_types.rs (1)
from_bytes(25-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build_crisp_sdk
- GitHub Check: build_sdk
- GitHub Check: build_ciphernode_image
- GitHub Check: test_net
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
🔇 Additional comments (52)
examples/CRISP/server/src/server/token_holders/etherscan.rs (1)
10-10: Clean import refactor with correct scoping. ✓The changes properly add the
eyreimport (necessary for error handling with.context()calls throughout the file) and scope theCONFIGimport to the test module only, which is correct since it's exclusively used in integration tests. No behavioral changes or public API impact.Also applies to: 625-625
packages/enclave-sdk/src/enclave-sdk.ts (1)
329-329: LGTM!The method signature and forwarding call correctly include the new
publicKeyHashparameter, maintaining consistency with thecontract-client.tschanges. The implementation properly threads the parameter through the SDK layer.Also applies to: 334-334
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol (1)
137-138: LGTM - E3 data fetch for committee public key.The retrieval pattern is consistent with the existing
decodeTallyfunction. SincevalidateInputis called via authorized contracts after E3 initialization, the E3 should always exist at this point.examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (4)
17-17: Consistent with the global constant update.The
publicInputsSizecorrectly matches the updatedNUMBER_OF_PUBLIC_INPUTSconstant at line 10. This ensures internal consistency within the verification key structure.
18-129: Verification key coordinates updated - auto-generated changes.The verification key G1 point coordinates have been updated across multiple selectors and permutation polynomials. These changes are expected when the circuit is modified (adding a public input changes the constraint system).
Note that
t1throught4(precomputed table commitments) andlagrangeFirstremained unchanged, which is expected as they may be circuit-independent or use standard values.Since these values are auto-generated by the circuit compiler, manual validation is not practical. The key verification is ensuring the circuit compilation was successful and the VK_HASH correctly represents this new verification key.
10-11: Verify circuit regeneration and VK_HASH correspondence with updated verification key.The public input count increased from 2066 to 2067 to add public key commitment. The constants are correctly propagated:
NUMBER_OF_PUBLIC_INPUTS = 2067(line 10) is passed to theHonkVerifierconstructor (line 2415) and used inpublicInputsSize(line 17). Calling code inCRISPProgram.sol(line 151) passes public inputs as a variable-sized array rather than a hardcoded count, so integration is not impacted by this change.However, verify that:
- The circuit was correctly regenerated with the new public key commitment as a public input
- The new VK_HASH corresponds to the regenerated verification key
2415-2419: The verification key implementation is correct. The circuit source properly includes public input commitments (including the committee public key), the compilation process viacompile_circuits.shgenerates matching artifacts with the correct VK_HASH, and comprehensive test vectors incrisp.contracts.test.tsvalidate proof generation and verification with the updated public inputs. No further changes needed.examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (1)
14-14: LGTM! Mock enclave now exposes configurable committee public key.The addition of the
committeePublicKeystate variable and its exposure throughgetE3correctly enables tests to configure the committee public key for proof verification.Also applies to: 38-38
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (3)
26-27: LGTM! Mock enclave properly integrated for tally decoding test.The mock enclave deployment and integration is correct. This test doesn't require setting the committee public key since it only validates tally decoding, not proof verification.
74-75: LGTM! Mock enclave correctly deployed for proof verification test.The mock enclave is properly instantiated and passed to the CRISP program deployment, enabling the test to configure the committee public key for proof verification.
97-97:proof.publicInputs[0]correctly contains the public key commitment.The Noir circuit (
examples/CRISP/circuits/src/main.nr) explicitly definespk_commitment: pub Fieldas the first public input, and the subsequentencodeSolidityProoffunction confirms thatpublicInputs[1]is the slot address, which validates thatpublicInputs[0]is indeed the public key commitment. The code is correct.crates/aggregator/Cargo.toml (1)
21-21: LGTM!The workspace dependency addition enables the
compute_pk_commitmentfunction usage for deriving public key hashes in the aggregator.examples/CRISP/server/src/cli/approve.rs (1)
28-28: LGTM!Removal of unused
Providerimport is a clean-up that reduces unnecessary dependencies in scope.packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json (1)
425-437: NewfeeToken()view function added.This addition appears unrelated to the PR's stated objective of passing public key commitment for proof verification. Verify this is intentionally bundled or if it should be in a separate PR.
packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
243-248: LGTM on the signature update.The
publishCommitteefunction now acceptspublicKeyHashas an explicit parameter, aligning with the PR objective to pass public key commitment for proof verification. The TODO comments appropriately flag the need for future verification that the public key matches the committee.packages/enclave-contracts/tasks/enclave.ts (2)
258-263: LGTM on thepublicKeyHashoption addition.The new option with proper validation at lines 290-292 ensures the hash is provided when publishing committees.
346-347: Hash computation inconsistency between ethers.id() and Rust compute_pk_commitment: clarify usage.
ethers.id(publicKey)computeskeccak256of the UTF-8 string representation, while the Rustcompute_pk_commitmentexpects raw bytes + BFV parameters (degree, plaintext_modulus, moduli). These produce different outputs.The contract has no validation against compute_pk_commitment (noted in the CiphernodeRegistryOwnable TODO), so the hash is only used for consistency checking between
publishCommitteeandactivatecalls. Both tasks in enclave.ts useethers.id()consistently with each other, so they work together.However, if integration with Rust-generated keys is intended, clarify whether the hash should be computed via
compute_pk_commitmentinstead, or document that these tasks are for development/testing only and incompatible with Rust-computed hashes.crates/tests/Cargo.toml (1)
34-34: LGTM!Adding
e3-bfv-helpersenables test code to usecompute_pk_commitmentfor verifying public key hash computations.packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (2)
31-31: Consistent hash computation with task implementation.Using
ethers.id(data)matches the pattern inactivateE3task. For testing purposes, this internal consistency is appropriate.
361-369: LGTM on test updates forpublishCommittee.The test correctly passes
dataHashas the fourth argument and verifies thatcommitteePublicKeyreturns the expected hash value.examples/CRISP/circuits/src/main.nr (1)
54-56: LGTM!Making
pk_commitmenta public input is correct for the proof verification flow. This allows the verifier to confirm that the commitment used in the proof matches the expected public key hash published on-chain.crates/evm-helpers/src/contracts.rs (3)
10-10: LGTM!The
FixedBytesimport is correctly added to support the newbytes32parameter type.
78-78: LGTM!The Solidity interface definition correctly reflects the updated
activatefunction signature with the newpublicKeyHashparameter.
141-146: LGTM!The trait signature and implementation are consistent. The
FixedBytes<32>type correctly maps to the Soliditybytes32parameter.Also applies to: 408-425
crates/aggregator/src/publickey_aggregator.rs (2)
9-9: LGTM!The import correctly brings in the
compute_pk_commitmentfunction needed for deriving the public key hash.
185-210: LGTM!The commitment computation is correctly placed after public key aggregation and before state update. The parameters passed (
degree,plaintext,moduli) correctly reflect the FHE parameters used during key generation. The computed hash is properly included in thePublicKeyAggregatedevent.Note that if
compute_pk_commitmentfails, the entire aggregation handler returns an error, which is appropriate behavior since the commitment is now a required part of the activation flow.packages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.sol (1)
158-164: LGTM!The interface correctly adds the
publicKeyHashparameter with appropriate documentation. This aligns with the contract implementation changes elsewhere in the PR.crates/bfv-helpers/src/client.rs (2)
8-8: LGTM!The import correctly brings in the Greco conversion utility needed for computing the public key commitment.
133-158: The implementation is sound and already defensive. The conversion at lines 152-155 uses.try_into()with explicit error handling rather than assuming the output is exactly 32 bytes, so the concern in the review comment is already addressed in the code.crates/tests/tests/integration_legacy.rs (3)
11-11: LGTM!The import correctly brings in
compute_pk_commitmentfor test verification.
223-235: LGTM!The test correctly computes the expected
public_key_hashusing the same parameters as the production aggregator code, ensuring the test validates the actual commitment computation logic.
572-593: LGTM!The duplicate E3 ID test correctly verifies that different chain IDs produce independent
public_key_hashvalues through separate aggregation flows.Also applies to: 615-635
packages/enclave-contracts/contracts/interfaces/IEnclave.sol (1)
155-161: LGTM!The interface correctly adds the
publicKeyHashparameter to theactivatefunction with appropriate documentation. This is consistent with the implementation changes in the Rust contract bindings and the registry interface.examples/CRISP/server/src/server/indexer.rs (2)
17-20: LGTM! Imports correctly added for the new PK commitment flow.The imports for
FixedBytes,get_default_paramset, andcompute_pk_commitmentare necessary for computing and passing the public key commitment during activation.
359-376: LGTM! PK commitment computation and activation flow updated correctly.The code properly:
- Retrieves BFV parameters from the default param set
- Computes the PK commitment using the public key and parameters
- Converts to
FixedBytesand passes to the updatedactivatecallError handling with
map_errappropriately propagates commitment computation failures.packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json (1)
375-380: LGTM! ABI correctly updated with newpublicKeyHashparameter.The
publishCommitteefunction signature now includes thebytes32 publicKeyHashparameter, aligning with the interface changes across the PR.crates/bfv-helpers/src/utils/greco.rs (4)
9-9: LGTM! PublicKey import added for the new conversion function.
185-220: LGTM! BFV to Greco coefficient conversion helpers implemented correctly.The conversion logic properly:
- Centers coefficients from
[0, qi)to[-(qi-1)/2, (qi-1)/2]- Reverses coefficient order (matching Greco format)
- Reduces modulo ZKP modulus with correct handling of negative values
233-283: LGTM!bfv_public_key_to_grecofunction correctly converts BFV public keys to Greco format.The implementation:
- Accesses pk0/pk1 polynomials from the public key structure
- Converts to power basis representation for coefficient extraction
- Iterates over moduli to extract and convert coefficients per RNS component
- Applies the Greco transformation (centering, reversing, ZKP modulus reduction)
385-472: LGTM! Comprehensive test validates the conversion against GrecoVectors reference.The test correctly verifies that
bfv_public_key_to_grecoproduces identical output to the establishedGrecoVectors::computepath, ensuring the new conversion is accurate.packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol (2)
41-46: LGTM! Mock updated to match interface.The
publishCommitteesignature correctly includes the newbytes32parameter to comply with the updatedICiphernodeRegistryinterface.
118-123: LGTM! Second mock also updated consistently.
MockCiphernodeRegistryEmptyKey.publishCommitteesignature updated to match the interface change.packages/enclave-contracts/test/Enclave.spec.ts (4)
58-58: Verify:ethers.id()vsethers.keccak256()for dataHash computation.
ethers.id(data)computeskeccak256of the UTF-8 encoded string"0xda7a", not the bytes0xda7a. If the intent is to hash the raw bytes, useethers.keccak256(data)instead. Please confirm this is the intended behavior for test purposes.
76-77: LGTM!setupAndPublishCommitteecorrectly computes and passespublicKeyHash.The helper function now derives
publicKeyHashusingethers.id(publicKey)and passes it toregistry.publishCommittee, aligning with the updated contract interface.
942-946: LGTM! Allactivatecalls updated with the newdataHashparameter.The test cases consistently pass
dataHashas the third argument toenclave.activate(), matching the updated contract signature.Also applies to: 978-984, 1003-1003, 1038-1040, 1060-1060, 1092-1094, 1117-1117, 1131-1133, 1173-1173, 1209-1211, 1244-1244
1601-1601: LGTM! Output expectation updated to usekeccak256.The assertion now expects
ciphertextOutputto equalethers.keccak256(data), reflecting the contract's storage of the hash.examples/CRISP/server/src/cli/commands.rs (2)
15-15: LGTM! Imports added for the new PK commitment flow.Also applies to: 19-19
211-222: LGTM! PK commitment computed and passed to activate.The implementation correctly:
- Extracts BFV parameters from the generated key's parameters
- Computes the commitment using
compute_pk_commitment- Converts to
FixedBytesfor the contract callThe error handling with
map_errprovides a descriptive message for CLI users.packages/enclave-contracts/contracts/Enclave.sol (1)
298-323: LGTM! Activation function updated to accept and verifypublicKeyHash.The changes correctly:
- Accept
publicKeyHashas a parameter instead of computing on-chain- Verify against the registry's stored
committeePublicKey(renamed toregistryPublicKeyHashfor clarity)- Store the provided hash as
committeePublicKey- Continue emitting the full
publicKeyin the event for off-chain consumersThis approach moves the hash computation off-chain while maintaining verification integrity through the registry check.
crates/evm/src/ciphernode_registry_sol.rs (3)
12-12: LGTM!FixedBytesimport added for the new hash type.
406-424: LGTM! Handler correctly extracts and propagatespublic_key_hash.The
PublicKeyAggregatedhandler now extractspublic_key_hashfrom the message and passes it through topublish_committee_to_registry.
487-514: LGTM!publish_committee_to_registryupdated withpublic_key_hashparameter.The function correctly:
- Accepts
public_key_hash: [u8; 32]as a new parameter- Converts to
FixedBytesfor the contract call- Passes the hash to the updated
publishCommitteecontract method
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-contracts/tasks/enclave.ts (1)
333-347: Refactor hash computation to use consistent mechanism across TypeScript and Rust codebases.Line 346 uses
ethers.id(publicKey)(Keccak256 hash), but the Rust server code usescompute_pk_commitment()with BFV parameters (degree, plaintext_modulus, moduli) to compute a different hash based on Greco bounds. These two approaches produce fundamentally different results. The contract will reject the activation withCommitteeSelectionFailed()because the hashes will not match what was stored by the registry.The TypeScript task must either:
- Call
compute_pk_commitment()with the same BFV parameters used by the Rust server, or- Use a native TypeScript/JavaScript implementation of the same algorithm.
🧹 Nitpick comments (1)
crates/bfv-helpers/src/client.rs (1)
133-158: Newcompute_pk_commitmentfunction looks correct.The implementation follows the same pattern as
bfv_verifiable_encryptfor parameter construction and bounds computation. The flow is clear: deserialize the public key, compute Greco bounds, convert to Greco representation, and compute the commitment.A minor note: the variable
public_keyis shadowed on line 143 (parameter → deserializedPublicKey). This is a common Rust idiom for type refinement, but if you prefer explicit naming, consider renaming the deserialized key topkfor consistency with other functions in this file.🔎 Optional: Avoid variable shadowing for clarity
pub fn compute_pk_commitment( public_key: Vec<u8>, degree: usize, plaintext_modulus: u64, moduli: Vec<u64>, ) -> Result<[u8; 32]> { use shared::commitments::compute_pk_commitment as compute_pk_commitment_shared; let params = build_bfv_params_arc(degree, plaintext_modulus, &moduli, None); - let public_key = PublicKey::from_bytes(&public_key, ¶ms) + let pk = PublicKey::from_bytes(&public_key, ¶ms) .map_err(|e| anyhow!("Error deserializing public key: {}", e))?; let (_, bounds) = GrecoBounds::compute(¶ms, 0)?; let bit_pk = shared::template::calculate_bit_width(&bounds.pk_bounds[0].to_string())?; - let (pk0is, pk1is) = bfv_public_key_to_greco(&public_key, ¶ms); + let (pk0is, pk1is) = bfv_public_key_to_greco(&pk, ¶ms); let commitment_bigint = compute_pk_commitment_shared(&pk0is, &pk1is, bit_pk);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
crates/aggregator/Cargo.tomlcrates/aggregator/src/publickey_aggregator.rscrates/bfv-helpers/src/client.rscrates/bfv-helpers/src/utils/greco.rscrates/events/src/enclave_event/publickey_aggregated.rscrates/evm-helpers/src/contracts.rscrates/evm/src/ciphernode_registry_sol.rscrates/tests/Cargo.tomlcrates/tests/tests/integration_legacy.rsexamples/CRISP/circuits/src/main.nrexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solexamples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.solexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/server/src/cli/approve.rsexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/indexer.rsexamples/CRISP/server/src/server/token_holders/etherscan.rspackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-sdk/src/contract-client.tspackages/enclave-sdk/src/enclave-sdk.ts
🧰 Additional context used
🧠 Learnings (23)
📚 Learning: 2024-10-12T10:24:07.572Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-12T10:24:07.572Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Applied to files:
crates/events/src/enclave_event/publickey_aggregated.rscrates/tests/tests/integration_legacy.rscrates/evm/src/ciphernode_registry_sol.rscrates/aggregator/src/publickey_aggregator.rs
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/events/src/enclave_event/publickey_aggregated.rsexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/token_holders/etherscan.rsexamples/CRISP/server/src/server/indexer.rscrates/tests/tests/integration_legacy.rscrates/evm-helpers/src/contracts.rscrates/bfv-helpers/src/utils/greco.rscrates/evm/src/ciphernode_registry_sol.rscrates/aggregator/src/publickey_aggregator.rsexamples/CRISP/server/src/cli/approve.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
crates/events/src/enclave_event/publickey_aggregated.rscrates/tests/tests/integration_legacy.rscrates/bfv-helpers/src/utils/greco.rscrates/evm/src/ciphernode_registry_sol.rscrates/bfv-helpers/src/client.rs
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
crates/tests/Cargo.tomlcrates/aggregator/Cargo.toml
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
crates/tests/Cargo.tomlexamples/CRISP/server/src/server/token_holders/etherscan.rs
📚 Learning: 2025-11-08T01:31:47.505Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 969
File: packages/enclave-sdk/src/enclave-sdk.ts:101-103
Timestamp: 2025-11-08T01:31:47.505Z
Learning: In packages/enclave-sdk/src/enclave-sdk.ts, the TRBFV protocol parameter setup is intentionally incomplete as of PR #969. The constructor sets BFV_THRESHOLD parameters for TRBFV, but the encryption methods (encryptNumber, encryptVector, encryptNumberAndGenInputs, encryptVectorAndGenInputs) intentionally only support BFV and will throw "Protocol not supported" for TRBFV until future implementation is completed.
Applied to files:
packages/enclave-sdk/src/enclave-sdk.ts
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
examples/CRISP/server/src/cli/commands.rscrates/tests/tests/integration_legacy.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
examples/CRISP/server/src/server/indexer.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2025-11-12T10:08:30.720Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 996
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:144-169
Timestamp: 2025-11-12T10:08:30.720Z
Learning: In the CRISP program (examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol), there are plans to move the merkle tree from the Enclave contract to the program contract itself in the future, which would allow direct calls to validateInput to be stored in the merkle tree.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts
📚 Learning: 2025-08-29T16:46:50.289Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 666
File: examples/CRISP/contracts/CRISPChecker.sol:9-9
Timestamp: 2025-08-29T16:46:50.289Z
Learning: In hashcloak/semaphore-contracts-noir package, the interface is still named ISemaphore, not ISemaphoreNoir. The Noir support was added via Noir verifier contracts while keeping the original ISemaphore interface name.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2025-12-19T11:35:43.204Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 1109
File: examples/CRISP/server/src/server/routes/state.rs:33-37
Timestamp: 2025-12-19T11:35:43.204Z
Learning: In the CRISP voting system (examples/CRISP), ciphertext data and slot states are public and stored on-chain in the CRISPProgram smart contract. The server endpoints `/previous-ciphertext` and `/is-slot-empty` expose public blockchain data that can already be accessed by anyone through the contract's external view functions (getSlotIndex, isSlotEmptyByAddress) or by reading contract state directly. Authentication is not needed for these endpoints since they only expose data that is already publicly available on the blockchain.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2024-10-16T09:52:53.807Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/src/data_store.rs:74-75
Timestamp: 2024-10-16T09:52:53.807Z
Learning: In this project, the actor model handles potential errors internally, so methods like `checkpoint` in the `Checkpoint` trait do not need to explicitly handle or propagate errors.
Applied to files:
crates/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
crates/tests/tests/integration_legacy.rscrates/bfv-helpers/src/utils/greco.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
Applied to files:
crates/evm-helpers/src/contracts.rs
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
crates/bfv-helpers/src/utils/greco.rs
📚 Learning: 2024-11-05T06:56:49.157Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/aggregator.rs:0-0
Timestamp: 2024-11-05T06:56:49.157Z
Learning: `RegistryFilterSol` does not have a reader and does not require a repository reader or deploy block when calling `RegistryFilterSol::attach` in `packages/ciphernode/enclave_node/src/aggregator.rs`.
Applied to files:
crates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2025-11-05T14:12:57.814Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 963
File: examples/CRISP/client/package.json:25-25
Timestamp: 2025-11-05T14:12:57.814Z
Learning: In the Enclave/CRISP codebase, `enclave-e3/sdk` and `crisp-e3/sdk` are different packages: `enclave-e3/sdk` is the general Enclave SDK, while `crisp-e3/sdk` is the CRISP-specific SDK. The CRISP client (`examples/CRISP/client`) intentionally depends on `enclave-e3/sdk`, not `crisp-e3/sdk`.
Applied to files:
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/bfv-helpers/src/client.rs
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed to support multiple network connections: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3. Similarly, `.revertedWithoutReason(ethers)` replaces `.revertedWithoutReason`.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2024-10-28T12:04:26.578Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave_node/src/ciphernode.rs:26-28
Timestamp: 2024-10-28T12:04:26.578Z
Learning: In the `setup_ciphernode` function in `packages/ciphernode/enclave_node/src/ciphernode.rs`, panicking on errors during setup is acceptable.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-09-11T13:21:31.031Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/tasks/utils.ts:7-8
Timestamp: 2025-09-11T13:21:31.031Z
Learning: In Hardhat v3, the task API syntax has changed significantly from v2. The new syntax uses:
- `.addOption({ name, description, defaultValue, type })` instead of `.addOptionalParam()`
- `.setAction(async () => ({ default: (args, hre) => { ... } }))` instead of direct `.setAction((args, hre) => { ... })`
- `.build()` is required to finalize task definitions
- `ArgumentType.STRING` is used for option types instead of `types.string`
Applied to files:
packages/enclave-contracts/tasks/enclave.ts
📚 Learning: 2025-09-11T13:09:03.800Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/scripts/deployAndSave/naiveRegistryFilter.ts:30-31
Timestamp: 2025-09-11T13:09:03.800Z
Learning: In Hardhat v3 deployment scripts, use `(await signer.provider?.getNetwork())?.name ?? "localhost"` instead of `hre.globalOptions.network` to reliably get the network name, as `hre.globalOptions.network` can be undefined in some contexts.
Applied to files:
packages/enclave-contracts/tasks/enclave.ts
🧬 Code graph analysis (7)
examples/CRISP/server/src/cli/commands.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
examples/CRISP/server/src/server/indexer.rs (2)
examples/CRISP/crates/crisp-constants/src/lib.rs (1)
get_default_paramset(10-13)crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
crates/tests/tests/integration_legacy.rs (2)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)crates/trbfv/src/trbfv_config.rs (1)
params(34-36)
crates/aggregator/src/publickey_aggregator.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (1)
examples/CRISP/packages/crisp-contracts/tests/utils.ts (2)
deployMockEnclave(43-47)deployCRISPProgram(69-87)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
examples/CRISP/client/libs/crispSDKWorker.js (2)
proof(25-25)slotAddress(14-14)
crates/bfv-helpers/src/client.rs (2)
crates/bfv-helpers/src/utils/greco.rs (1)
bfv_public_key_to_greco(233-283)crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_arc(278-300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_unit
🔇 Additional comments (55)
examples/CRISP/server/src/server/token_holders/etherscan.rs (1)
621-621: Redundant import statement; verify PR scope alignment.Line 620's
use super::*;already bringsCONFIGinto scope, making the explicit import on line 621 redundant. More importantly, this file's changes don't align with the PR objectives about public key commitment hashing—the Etherscan client is unrelated to proof verification or key management.Can you clarify why this file is included in the PR? If the import is intentional for documentation purposes, consider removing the wildcard import to avoid confusion. Alternatively, if this file should not be part of the public key commitment refactor, it may have been included in error.
examples/CRISP/server/src/cli/approve.rs (1)
28-28: Good cleanup: unused import removed.Removing the unused
Providerimport is good practice. The code only usesProviderBuilderand relies on type inference for the provider instance.Note: This change appears unrelated to the PR's stated objective of passing public key commitment for proof verification. If this is intentional cleanup alongside the main changes, that's fine.
crates/aggregator/Cargo.toml (1)
21-21: LGTM!The addition of the
e3-bfv-helpersworkspace dependency is correct and necessary to enable public key commitment computation in the aggregator.examples/CRISP/circuits/src/main.nr (1)
56-56: LGTM!Exposing
pk_commitmentas a public input is correct and aligns with the PR's goal of enabling on-chain verification of the public key commitment.examples/CRISP/server/src/cli/commands.rs (1)
212-222: LGTM!The public key commitment computation is correctly implemented:
- Uses
compute_pk_commitmentwith appropriate BFV parameters- Proper error handling via
map_err- Correctly converts the result to
FixedBytes<32>for the contract callpackages/enclave-contracts/contracts/interfaces/IEnclave.sol (1)
155-161: LGTM!The interface signature update correctly adds the
publicKeyHashparameter to theactivatefunction, with appropriate documentation.packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json (2)
350-355: LGTM!The ABI correctly reflects the updated
activatefunction signature with the newpublicKeyHashparameter of typebytes32.
425-437: LGTM!The addition of the
feeTokenfunction to the ABI is correct and aligns with the interface definition.packages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.sol (1)
158-164: LGTM!The
publishCommitteesignature update correctly adds thepublicKeyHashparameter with appropriate documentation.packages/enclave-contracts/tasks/enclave.ts (2)
258-263: LGTM!The addition of the
publicKeyHashoption to thepublishCommitteetask is correctly implemented.
290-298: LGTM!The validation guard and usage of
publicKeyHashin thepublishCommitteecall are correctly implemented.crates/events/src/enclave_event/publickey_aggregated.rs (1)
19-19: LGTM!The addition of the
public_key_hashfield to thePublicKeyAggregatedstruct is correct and maintains proper serialization support.crates/tests/Cargo.toml (1)
34-34: LGTM!The
e3-bfv-helpersdependency is correctly added with workspace configuration, maintaining alphabetical ordering with existing dependencies.examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
282-286: LGTM!The index adjustments correctly reflect the new public inputs layout where
publicKeyHashis now at index 0. The updated indices align withCRISPProgram.solwhere:
noirPublicInputs[0]= committeePublicKeynoirPublicInputs[1]= slotAddressnoirPublicInputs[2]= isFirstVotenoirPublicInputs[3+]= vote bytespackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (5)
31-31: Verify:ethers.id()vsethers.keccak256()behavioral difference.
ethers.id(data)computes the keccak256 of the UTF-8 string representation, whileethers.keccak256(data)hashes the raw bytes. Fordata = "0xda7a":
keccak256would hash 2 bytes[0xda, 0x7a]idhashes the 6-character string"0xda7a"If this is intentional for test simplicity (since the actual
publicKeyHashin production comes fromcompute_pk_commitment), the change is fine. Otherwise, this may cause unexpected behavior if tests are meant to mirror production hashing.
339-348: LGTM!The
publishCommitteecall correctly passesdataHashas the new fourth parameter, consistent with the updated contract signature.
361-369: LGTM!The test correctly verifies that
committeePublicKey(request.e3Id)returns the provideddataHash, aligning with the new contract behavior where the hash is stored directly rather than computed.
381-394: LGTM!The event assertion correctly passes
dataHashtopublishCommitteeand verifiesCommitteePublishedemission with the expected arguments.
509-517: LGTM!The
committeePublicKey()getter test correctly usesdataHashand verifies the returned value matches.examples/CRISP/server/src/server/indexer.rs (2)
17-20: LGTM!The new imports are correctly added for:
FixedBytesfor the 32-byte hash typeget_default_paramsetfor BFV parameterscompute_pk_commitmentfor deriving the public key hash
358-386: LGTM!The
handle_committee_time_expiredfunction correctly:
- Retrieves BFV parameters from the default parameter set
- Computes the PK commitment hash from the public key
- Propagates errors with a descriptive message
- Passes the hash as
FixedBytes<32>to theactivatecallThe error handling is appropriate, and the conversion from
[u8; 32]toFixedBytesis correct.Note that
get_default_paramset()uses insecure parameters (as documented incrisp-constants/src/lib.rs). Ensure this is only used in development/testing contexts and not in production deployments.packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
242-261: LGTM!The
publishCommitteefunction signature is correctly updated to accept a precomputedpublicKeyHash. Key observations:
- The
onlyOwnermodifier ensures only trusted callers can provide this hash- The TODO comment appropriately documents the trust assumption
- The
CommitteePublishedevent correctly emits the fullpublicKeyfor transparency while storing the hashThis aligns with the PR objective of moving commitment computation off-chain.
examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (2)
14-22: LGTM!The mock contract is correctly updated with a
committeePublicKeystate variable and setter, enabling tests to configure the public key hash thatgetE3()returns.
38-38: LGTM!The
getE3return now uses the configurablecommitteePublicKeystate variable instead of a hardcoded zero value, allowing proper test simulation.examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol (1)
137-148: LGTM!The
validateInputfunction correctly:
- Fetches the E3 data to retrieve the committed public key
- Expands the public inputs array to accommodate
committeePublicKey- Reorders public inputs:
[committeePublicKey, slotAddress, isFirstVote, ...vote]This ensures the Noir proof is verified against the on-chain committed public key, aligning with the SDK's
encodeSolidityProofchanges.examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (2)
74-75: LGTM!The test correctly deploys
mockEnclaveand passes it todeployCRISPProgram, ensuring the program has a reference to the mock forgetE3calls.
97-98: LGTM!Setting
committeePublicKeyfromproof.publicInputs[0]before callingvalidateInputensures the mock enclave returns the correct hash that matches the proof's committed public key. This is essential for the proof verification to succeed.examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (1)
10-17: Verification key updates align with circuit changes.The increment of
NUMBER_OF_PUBLIC_INPUTSfrom 2066 to 2067 correctly reflects the addition of the public key commitment as a new public input in the circuit. The updatedVK_HASHandpublicInputsSizeare consistent with this change.crates/tests/tests/integration_legacy.rs (2)
223-235: Test correctly computes and validates public key commitment.The test properly derives
public_key_hashusingcompute_pk_commitmentwith the correct BFV parameters (degree,plaintext,moduli) extracted from the params object, and includes it in the expectedPublicKeyAggregatedevent for assertion.
572-593: Duplicate E3 ID test correctly validates public key hash for both chain IDs.The test computes
public_key_hashfor each E3 request and properly asserts that thePublicKeyAggregatedevent contains the expected hash, validating the new field across different chain IDs.packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol (2)
41-46: Mock contract signature updated to match interface.The
publishCommitteefunction signature correctly adds thebytes32parameter forpublicKeyHash, aligning with the updatedICiphernodeRegistryinterface. The empty implementation is appropriate for a mock.
118-123: Consistent update applied to MockCiphernodeRegistryEmptyKey.Both mock contracts now have consistent
publishCommitteesignatures matching the updated interface.crates/aggregator/src/publickey_aggregator.rs (1)
185-210: Public key commitment computed and propagated correctly.The
compute_pk_commitmentcall uses the correct parameters fromself.fhe.paramsand the error is properly propagated with?. The commitment is computed before state update and included in thePublicKeyAggregatedevent.crates/bfv-helpers/src/utils/greco.rs (3)
185-220: Coefficient conversion helpers are correctly implemented.
convert_bfv_coefficient_to_centeredproperly centers coefficients from[0, qi)to[-(qi-1)/2, (qi-1)/2].convert_bfv_coefficients_to_grecocorrectly reverses, centers, and reduces mod ZKP modulus with proper handling of negative values.
233-283: BFV public key to Greco conversion is well-structured.The function correctly accesses the public key polynomials via
pk.c.c[0]andpk.c.c[1], converts toPowerBasisrepresentation to extract coefficients, and applies the Greco format conversion for each modulus.
385-472: Comprehensive test validates against reference implementation.The test compares the output of
bfv_public_key_to_grecoagainstGrecoVectors.compute().standard_form(), providing strong validation that the conversion matches the expected Greco format.packages/enclave-sdk/src/enclave-sdk.ts (1)
329-335: SDK activateE3 method correctly updated with publicKeyHash parameter.The new
publicKeyHashparameter is appropriately placed and typed as`0x${string}`, consistent with other hex-encoded parameters in the SDK. The parameter is correctly forwarded to the contract client.packages/enclave-sdk/src/contract-client.ts (1)
164-194: Contract client activateE3 correctly propagates publicKeyHash to contract call.The method signature and
simulateContractargs array are updated to includepublicKeyHash, matching the updatedEnclave.activatefunction signature. The transaction simulation and execution flow remain correct.packages/enclave-contracts/contracts/Enclave.sol (1)
298-324: Activation flow correctly validates publicKeyHash against registry.The function now verifies the provided
publicKeyHashagainstciphernodeRegistry.committeePublicKey(e3Id)rather than computingkeccak256(publicKey). This aligns with the new commitment scheme where the hash is a Greco/Poseidon-based commitment computed off-chain and stored in the registry during committee publication.Note that
publicKeyis still accepted and emitted in theE3Activatedevent for off-chain consumers, but the on-chain verification relies on the registry-stored commitment. This trust model is appropriate since the registry value is set by the trusted aggregator duringpublishCommittee.packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json (1)
375-380: LGTM!The
publishCommitteefunction signature correctly adds the newpublicKeyHashparameter of typebytes32, aligning with the PR objective to propagate public key commitment through the registry flow.crates/bfv-helpers/src/client.rs (1)
152-155: Potential issue: BigInt byte conversion may not always produce exactly 32 bytes.
BigInt::to_bytes_be()returns the minimal byte representation, which may be fewer than 32 bytes if there are leading zeros, or could potentially be more than 32 bytes if the commitment value is unexpectedly large. This would causetry_into()to fail.Consider padding to ensure exactly 32 bytes:
🔎 Proposed fix: Pad bytes to 32
let commitment_bigint = compute_pk_commitment_shared(&pk0is, &pk1is, bit_pk); - let bytes = commitment_bigint.to_bytes_be().1; - let public_key_hash: [u8; 32] = bytes - .try_into() - .map_err(|_| anyhow!("Commitment must be exactly 32 bytes"))?; + let bytes = commitment_bigint.to_bytes_be().1; + if bytes.len() > 32 { + return Err(anyhow!("Commitment exceeds 32 bytes")); + } + let mut public_key_hash = [0u8; 32]; + public_key_hash[32 - bytes.len()..].copy_from_slice(&bytes); Ok(public_key_hash) }Please verify that
compute_pk_commitment_sharedalways returns a value that fits exactly in 32 bytes, or consider the padding approach above to handle variable-length BigInt representations.crates/evm-helpers/src/contracts.rs (3)
10-10: LGTM!The
FixedBytesimport is correctly added to support the newbytes32parameter type.
78-78: LGTM!The Solidity ABI definition correctly adds
publicKeyHashasbytes32to theactivatefunction signature, matching the updated contract interface.
141-146: LGTM!The
EnclaveWrite::activatetrait and its implementation are correctly updated to accept and forward the newpub_key_hash: FixedBytes<32>parameter. The implementation properly passes it to the contract call on line 421.Also applies to: 408-425
crates/evm/src/ciphernode_registry_sol.rs (3)
12-12: LGTM!The
FixedBytesimport is correctly added for the new parameter type.
406-432: LGTM!The
PublicKeyAggregatedhandler correctly extracts the newpublic_key_hashfield from the message and passes it through topublish_committee_to_registry. The async block properly captures and forwards the hash.
487-515: LGTM!The
publish_committee_to_registryfunction is correctly updated:
- New parameter
public_key_hash: [u8; 32]added to signature- Conversion to
FixedByteson line 498- Contract call updated to include the hash on line 511
This aligns with the updated
ICiphernodeRegistryinterface.packages/enclave-contracts/test/Enclave.spec.ts (8)
57-58: LGTM!The
dataHashcomputation usingethers.id(data)(which computeskeccak256(toUtf8Bytes(data))) is consistent with how it's used in the test'sactivatecalls.
64-78: LGTM!The
setupAndPublishCommitteehelper is correctly updated to:
- Compute
publicKeyHashusingethers.id(publicKey)on line 76- Pass it as the 4th parameter to
publishCommitteeon line 77This aligns with the updated registry interface.
938-984: LGTM!The
activate()test cases are correctly updated to use the new 3-parameter signature(e3Id, data, dataHash). The tests properly validate:
- E3 does not exist (line 942-944)
- E3 already activated (line 978-983)
1001-1004: LGTM!All activate-related test scenarios are consistently updated with the new
dataHashparameter:
- E3NotReady checks
- E3Expired checks
- CommitteeSelectionFailed checks
- Success path validations
Also applies to: 1036-1041, 1058-1061, 1090-1095, 1115-1118, 1129-1134
1171-1177: LGTM!The remaining
activate()tests for:
- Setting
committeePublicKeycorrectly- Return value validation via
staticCallE3Activatedevent emissionAll correctly use the updated 3-parameter signature.
Also applies to: 1207-1212, 1242-1248
1309-1309: LGTM!The
publishInput()test scenarios correctly callactivate()with the new signature before testing input publication.Also applies to: 1343-1343, 1383-1383, 1404-1404, 1436-1436
1493-1493: LGTM!The
publishCiphertextOutput()tests correctly use the updatedactivate()signature.Note on line 1601: The assertion
expect(e3.ciphertextOutput).to.equal(ethers.keccak256(data))useskeccak256(raw bytes hash) rather thanethers.id(UTF-8 encoded hash), which is correct sinceciphertextOutputstores the hash of the raw ciphertext bytes.Also applies to: 1528-1528, 1567-1567, 1597-1601, 1627-1627, 1657-1657
1711-1711: LGTM!The
publishPlaintextOutput()tests correctly use the updatedactivate()signature throughout all test cases.Also applies to: 1740-1740, 1775-1775, 1806-1806, 1838-1838, 1869-1869
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 (1)
packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
242-261: Add validation to ensure the providedpublicKeyHashmatches thepublicKeyparameter.The function accepts a pre-computed
publicKeyHashwithout validating it against the providedpublicKey. While the TODO comments acknowledge trusting the owner, this creates a risk of storing a mismatched hash if the caller provides incorrect values.Since
publicKeyis available as a parameter and tests confirmkeccak256is the hash function used (viaethers.id()), add a validation check:require(publicKeyHash == keccak256(publicKey), "Hash mismatch");This prevents accidental consistency issues without additional computational overhead.
🧹 Nitpick comments (3)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
283-284: Consider adding bounds validation for publicInputs array.The function accesses
proof.publicInputs[1]andproof.publicInputs.slice(3)without validating that the array has sufficient elements. If a malformed proof is passed, this could cause runtime errors or unexpected behavior.Consider adding a defensive check:
🔎 Proposed defensive validation
export const encodeSolidityProof = (proof: ProofData): Hex => { + if (proof.publicInputs.length < 4) { + throw new Error(`Invalid proof: expected at least 4 public inputs, got ${proof.publicInputs.length}`) + } + const vote = proof.publicInputs.slice(3) as `0x${string}`[] const slotAddress = getAddress(numberToHex(BigInt(proof.publicInputs[1]), { size: 20 })) return encodeAbiParameters(parseAbiParameters('bytes, bytes32[], address'), [bytesToHex(proof.proof), vote, slotAddress]) }crates/events/src/enclave_event/publickey_aggregated.rs (1)
17-19: Consider adding hex formatting forpublic_key_hashin Debug output.The
pubkeyfield uses a custom hex formatter (e3_utils::formatters::hexf), butpublic_key_hashwill display as an array of decimal numbers in debug output. For consistency and readability, consider applying the same formatter.🔎 Proposed fix
#[derivative(Debug(format_with = "e3_utils::formatters::hexf"))] pub pubkey: Vec<u8>, + #[derivative(Debug(format_with = "e3_utils::formatters::hexf"))] pub public_key_hash: [u8; 32],packages/enclave-contracts/tasks/enclave.ts (1)
290-292: Strengthen publicKeyHash validation.The validation only checks if
publicKeyHashis truthy but doesn't verify it's a valid bytes32 format. Consider adding format validation to catch user errors early before the contract call.🔎 Suggested validation enhancement
if (!publicKeyHash) { throw new Error("publicKeyHash is required"); } + +if (!/^0x[a-fA-F0-9]{64}$/.test(publicKeyHash)) { + throw new Error("publicKeyHash must be a valid bytes32 hex string (0x + 64 hex characters)"); +}Additionally, consider adding documentation or a console message showing how to compute the hash (e.g., using
ethers.id(publicKey)) since the task requires it as input rather than computing it automatically.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
crates/aggregator/Cargo.tomlcrates/aggregator/src/publickey_aggregator.rscrates/bfv-helpers/src/client.rscrates/bfv-helpers/src/utils/greco.rscrates/events/src/enclave_event/publickey_aggregated.rscrates/evm-helpers/src/contracts.rscrates/evm/src/ciphernode_registry_sol.rscrates/tests/Cargo.tomlcrates/tests/tests/integration_legacy.rsexamples/CRISP/circuits/src/main.nrexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solexamples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.solexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/server/src/cli/approve.rsexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/indexer.rsexamples/CRISP/server/src/server/token_holders/etherscan.rspackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-sdk/src/contract-client.tspackages/enclave-sdk/src/enclave-sdk.ts
🧰 Additional context used
🧠 Learnings (25)
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
crates/aggregator/Cargo.tomlcrates/tests/Cargo.toml
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
crates/tests/Cargo.tomlexamples/CRISP/server/src/server/token_holders/etherscan.rs
📚 Learning: 2024-10-12T10:24:07.572Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-12T10:24:07.572Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Applied to files:
crates/events/src/enclave_event/publickey_aggregated.rscrates/tests/tests/integration_legacy.rspackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solcrates/aggregator/src/publickey_aggregator.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-29T01:04:19.137Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:53-70
Timestamp: 2024-10-29T01:04:19.137Z
Learning: In the `Keyshare` struct within `packages/ciphernode/keyshare/src/keyshare.rs`, the `self.secret` field is stored in encrypted form and is not considered sensitive.
Applied to files:
crates/events/src/enclave_event/publickey_aggregated.rs
📚 Learning: 2025-11-12T10:08:30.720Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 996
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:144-169
Timestamp: 2025-11-12T10:08:30.720Z
Learning: In the CRISP program (examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol), there are plans to move the merkle tree from the Enclave contract to the program contract itself in the future, which would allow direct calls to validateInput to be stored in the merkle tree.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol
📚 Learning: 2025-12-19T11:35:43.204Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 1109
File: examples/CRISP/server/src/server/routes/state.rs:33-37
Timestamp: 2025-12-19T11:35:43.204Z
Learning: In the CRISP voting system (examples/CRISP), ciphertext data and slot states are public and stored on-chain in the CRISPProgram smart contract. The server endpoints `/previous-ciphertext` and `/is-slot-empty` expose public blockchain data that can already be accessed by anyone through the contract's external view functions (getSlotIndex, isSlotEmptyByAddress) or by reading contract state directly. Authentication is not needed for these endpoints since they only expose data that is already publicly available on the blockchain.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
crates/tests/tests/integration_legacy.rscrates/bfv-helpers/src/utils/greco.rsexamples/CRISP/server/src/server/token_holders/etherscan.rscrates/bfv-helpers/src/client.rspackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/tests/tests/integration_legacy.rscrates/evm-helpers/src/contracts.rscrates/aggregator/src/publickey_aggregator.rsexamples/CRISP/server/src/cli/approve.rscrates/bfv-helpers/src/utils/greco.rsexamples/CRISP/server/src/server/token_holders/etherscan.rsexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/indexer.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
crates/tests/tests/integration_legacy.rsexamples/CRISP/server/src/cli/commands.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-16T09:52:53.807Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/src/data_store.rs:74-75
Timestamp: 2024-10-16T09:52:53.807Z
Learning: In this project, the actor model handles potential errors internally, so methods like `checkpoint` in the `Checkpoint` trait do not need to explicitly handle or propagate errors.
Applied to files:
crates/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
crates/tests/tests/integration_legacy.rscrates/bfv-helpers/src/utils/greco.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
Applied to files:
crates/evm-helpers/src/contracts.rs
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 752
File: packages/enclave-contracts/contracts/Enclave.sol:15-17
Timestamp: 2025-09-19T11:16:53.825Z
Learning: The Enclave contract in the gnosisguild/enclave repository has not been deployed yet as of September 2025, so storage layout considerations for upgradeable contracts don't apply to current changes.
Applied to files:
crates/evm-helpers/src/contracts.rs
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
crates/bfv-helpers/src/utils/greco.rs
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
examples/CRISP/server/src/cli/commands.rs
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/bfv-helpers/src/client.rs
📚 Learning: 2025-09-11T13:21:31.031Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/tasks/utils.ts:7-8
Timestamp: 2025-09-11T13:21:31.031Z
Learning: In Hardhat v3, the task API syntax has changed significantly from v2. The new syntax uses:
- `.addOption({ name, description, defaultValue, type })` instead of `.addOptionalParam()`
- `.setAction(async () => ({ default: (args, hre) => { ... } }))` instead of direct `.setAction((args, hre) => { ... })`
- `.build()` is required to finalize task definitions
- `ArgumentType.STRING` is used for option types instead of `types.string`
Applied to files:
packages/enclave-contracts/tasks/enclave.ts
📚 Learning: 2025-09-11T13:09:03.800Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/scripts/deployAndSave/naiveRegistryFilter.ts:30-31
Timestamp: 2025-09-11T13:09:03.800Z
Learning: In Hardhat v3 deployment scripts, use `(await signer.provider?.getNetwork())?.name ?? "localhost"` instead of `hre.globalOptions.network` to reliably get the network name, as `hre.globalOptions.network` can be undefined in some contexts.
Applied to files:
packages/enclave-contracts/tasks/enclave.ts
📚 Learning: 2025-11-08T01:31:47.505Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 969
File: packages/enclave-sdk/src/enclave-sdk.ts:101-103
Timestamp: 2025-11-08T01:31:47.505Z
Learning: In packages/enclave-sdk/src/enclave-sdk.ts, the TRBFV protocol parameter setup is intentionally incomplete as of PR #969. The constructor sets BFV_THRESHOLD parameters for TRBFV, but the encryption methods (encryptNumber, encryptVector, encryptNumberAndGenInputs, encryptVectorAndGenInputs) intentionally only support BFV and will throw "Protocol not supported" for TRBFV until future implementation is completed.
Applied to files:
packages/enclave-sdk/src/enclave-sdk.ts
📚 Learning: 2024-11-05T06:56:49.157Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/aggregator.rs:0-0
Timestamp: 2024-11-05T06:56:49.157Z
Learning: `RegistryFilterSol` does not have a reader and does not require a repository reader or deploy block when calling `RegistryFilterSol::attach` in `packages/ciphernode/enclave_node/src/aggregator.rs`.
Applied to files:
crates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
crates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-08T07:15:06.690Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/evm/src/ciphernode_registry_sol.rs:133-143
Timestamp: 2024-10-08T07:15:06.690Z
Learning: In `packages/ciphernode/evm/src/ciphernode_registry_sol.rs`, the function `helpers::stream_from_evm` in Rust returns `()`, not a `Result`, so error handling with `if let Err(e) = ...` is not applicable.
Applied to files:
crates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed to support multiple network connections: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3. Similarly, `.revertedWithoutReason(ethers)` replaces `.revertedWithoutReason`.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2024-10-28T12:04:26.578Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave_node/src/ciphernode.rs:26-28
Timestamp: 2024-10-28T12:04:26.578Z
Learning: In the `setup_ciphernode` function in `packages/ciphernode/enclave_node/src/ciphernode.rs`, panicking on errors during setup is acceptable.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
🧬 Code graph analysis (7)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
examples/CRISP/client/libs/crispSDKWorker.js (2)
proof(25-25)slotAddress(14-14)
crates/tests/tests/integration_legacy.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (1)
examples/CRISP/packages/crisp-contracts/tests/utils.ts (2)
deployMockEnclave(43-47)deployCRISPProgram(69-87)
crates/aggregator/src/publickey_aggregator.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
examples/CRISP/server/src/cli/commands.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
crates/bfv-helpers/src/client.rs (2)
crates/bfv-helpers/src/utils/greco.rs (1)
bfv_public_key_to_greco(233-283)crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_arc(278-300)
examples/CRISP/server/src/server/indexer.rs (2)
examples/CRISP/crates/crisp-constants/src/lib.rs (1)
get_default_paramset(10-13)crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_unit
🔇 Additional comments (44)
examples/CRISP/server/src/cli/approve.rs (1)
28-28: Good cleanup: unused import removed.The removal of the unused
Providerimport is appropriate since onlyProviderBuilderis actually used in this function. This improves code clarity.examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
282-287: The code is correct. The public inputs are structured as: index 0 =pk_commitment, index 1 =slot_address, index 2 =is_first_vote, and index 3+ = vote data. Index 2 is intentionally skipped becauseis_first_voteis a circuit-internal boolean flag used to determine behavior but not needed for Solidity encoding. The indexing accurately reflects the circuit's public input order defined incircuits/src/main.nr.Likely an incorrect or invalid review comment.
examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (3)
17-17: LGTM: Verification key public inputs size updated consistently.The
publicInputsSizefield correctly reflects the new count of 2067 public inputs, maintaining consistency with the constant defined at line 10.
18-128: Verification key regenerated for updated circuit.All verification key G1 points have been updated following the circuit modification to include the public key commitment. The selective update pattern (changed selector and permutation points, unchanged table commitments at t1-t4 and lagrangeFirst) is consistent with adding a public input without modifying the lookup table structure.
10-11: Public input count incremented to accommodate public key commitment.The increase from 2066 to 2067 public inputs and the updated VK hash are consistent with the PR objective of passing a public key commitment for proof verification. Integration tests in
crisp.contracts.test.tsvalidate proof generation and verification withgenerateVoteProof(), which correctly includes the public key in the proof inputs passed tohonkVerifier.verify().examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (3)
14-14: LGTM! Clean state variable addition.The new public state variable properly stores the committee public key hash for the mock.
20-22: LGTM! Appropriate mock setter.The setter function is simple and appropriate for a test mock. The lack of access control is acceptable since this is only used in tests.
38-38: LGTM! Correct integration of the state variable.The change correctly returns the
committeePublicKeystate variable instead of a hardcoded zero value, aligning with the PR's objective to support public key commitment verification.examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (3)
74-75: LGTM! Consistent mock setup.The test now properly deploys and injects the mock enclave, consistent with the refactored architecture.
26-27: > Likely an incorrect or invalid review comment.
97-98: The test correctly usesproof.publicInputs[0]to set the committee public key. The first public input of the CRISP circuit ispk_commitment, the committee public key commitment, so the assumption is valid.examples/CRISP/circuits/src/main.nr (1)
56-56: LGTM! Makingpk_commitmenta public input enables external verification.This change correctly exposes the public key commitment as a public circuit input, allowing the verifier to supply and validate the commitment against on-chain data during proof verification.
crates/tests/Cargo.toml (1)
34-34: LGTM!The
e3-bfv-helpersdependency is correctly added as a workspace dependency to supportcompute_pk_commitmentusage in tests.examples/CRISP/server/src/server/token_holders/etherscan.rs (1)
621-621: LGTM!The
CONFIGimport is correctly added to support the existing integration tests that reference configuration values likeetherscan_api_key,chain_id, andhttp_rpc_url.crates/aggregator/Cargo.toml (1)
21-21: LGTM!The
e3-bfv-helpersdependency is correctly added to support thecompute_pk_commitmentfunction used in the public key aggregation workflow.packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (2)
31-31: Verify the hash computation method change.The hash computation changed from
ethers.keccak256(data)toethers.id(data). These produce different results:
ethers.keccak256("0xda7a")hashes the 2 raw bytes[0xda, 0x7a]ethers.id("0xda7a")hashes the 6-character UTF-8 string"0xda7a"Ensure this change aligns with how the contract and other components compute the public key hash.
346-348: LGTM! Test correctly updated for newpublishCommitteesignature.The
dataHashparameter is correctly passed topublishCommittee, and assertions are updated to verify thatcommitteePublicKeyreturns the hash rather than the raw data.Also applies to: 364-369
packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json (2)
350-355: LGTM! Interface correctly updated withpublicKeyHashparameter.The
activatefunction signature now includes thebytes32 publicKeyHashparameter, aligning with the PR objective to pass public key commitment for verification.
425-437: Note: NewfeeToken()getter added.This new view function appears unrelated to the public key commitment refactor. Ensure this addition is intentional for this PR.
crates/aggregator/src/publickey_aggregator.rs (3)
9-9: LGTM!The
compute_pk_commitmentimport is correctly added from thee3_bfv_helperscrate.
185-190: LGTM! Public key commitment computation is correctly integrated.The commitment is computed with the appropriate BFV parameters (degree, plaintext modulus, moduli) immediately after aggregating the public key, ensuring the hash is available before broadcasting the event.
203-208: LGTM! Event correctly includes the newpublic_key_hashfield.The
PublicKeyAggregatedevent is properly constructed with the computed commitment hash.packages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.sol (1)
158-164: Interface extension looks correct.The
publishCommitteefunction signature is properly extended to include thepublicKeyHashparameter of typebytes32. The NatSpec documentation is also updated to describe the new parameter. This aligns with the PR objective to pass the public key commitment for proof verification.packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json (1)
375-380: ABI artifact correctly updated.The artifact correctly reflects the new
publicKeyHashparameter of typebytes32in thepublishCommitteefunction. ThebuildInfoIdupdate is expected as it's auto-generated during compilation.Also applies to: 543-543
packages/enclave-sdk/src/enclave-sdk.ts (1)
329-335: SDK method signature correctly extended.The
activateE3method properly accepts the newpublicKeyHashparameter and forwards it tocontractClient.activateE3. The type0x${string}is appropriate for a bytes32 hex value in viem.packages/enclave-contracts/contracts/interfaces/IEnclave.sol (1)
155-161: Interface extension is correct.The
activatefunction signature is properly extended to include thepublicKeyHashparameter of typebytes32. The NatSpec documentation accurately describes the parameter as "Hash of the committee's public key."packages/enclave-sdk/src/contract-client.ts (1)
164-186: Contract client correctly updated.The
activateE3method signature and contract call arguments are properly extended to includepublicKeyHash. The parameter order[e3Id, publicKey, publicKeyHash]correctly matches the Solidity function signatureactivate(uint256 e3Id, bytes calldata publicKey, bytes32 publicKeyHash).packages/enclave-contracts/contracts/Enclave.sol (2)
310-316: Validation logic correctly validates externally provided hash.The refactored validation:
- Retrieves the expected hash from
ciphernodeRegistry.committeePublicKey(e3Id)- Compares it against the caller-provided
publicKeyHash- Reverts with
CommitteeSelectionFailed()if they don't matchThis design correctly ensures that the activation caller provides a hash that matches what was registered during
publishCommittee, maintaining the integrity of the public key commitment workflow.
298-324: No type mismatch. ThecommitteePublicKeyfield isbytes32, notbytes.The E3 struct definition (in
packages/enclave-contracts/contracts/interfaces/IE3.sol) declaresbytes32 committeePublicKey, making the assignment on line 319 type-safe. Thehex""initialization on line 257 is a bytes literal that implicitly converts tobytes32(zero-padded). The concern about assigningbytes32to abytesfield does not apply here.examples/CRISP/server/src/server/indexer.rs (1)
363-380: PK commitment flow in indexer is correctly implemented.The
handle_committee_time_expiredfunction properly computes the public key commitment using the default parameter set and passes it to theactivatecall. The implementation correctly:
- Retrieves the default BFV parameter set with all required fields
- Computes the commitment from the event's public key with properly typed parameters
- Wraps the result in
FixedBytesfor the contract callexamples/CRISP/server/src/cli/commands.rs (1)
211-222: PK commitment computation and activation flow is correct.The implementation correctly:
- Uses
BfvParametersmethods with proper types:degree()returnsusize,plaintext()returnsu64, andmoduli()returns a slice that's converted toVec<u64>- Passes these to
compute_pk_commitmentwhich expects exactly these types- Handles errors appropriately with
map_errNote:
compute_pk_commitmentinternally callsbuild_bfv_params_arc()which may panic on invalid parameters. This is a known limitation planned for refactoring to proper Result-based error handling.crates/bfv-helpers/src/utils/greco.rs (2)
185-220: Well-structured BFV to Greco coefficient conversion helpers.The conversion logic correctly handles:
- Centering coefficients around zero (line 192:
if coeff > (qi / 2))- Reversing coefficient order (line 207:
.rev())- Reducing modulo ZKP modulus with proper handling of negative values (lines 213-217)
233-283: LGTM - Correct implementation with good test coverage.The function properly:
- Clones polynomials before mutating representation (lines 247-250)
- Extracts coefficients per modulus in the correct order
- Applies the Greco conversion format
The test at lines 385-472 validates correctness against the reference
GrecoVectorsimplementation, ensuring coefficient-by-coefficient equality.crates/tests/tests/integration_legacy.rs (2)
223-235: LGTM - Test correctly computes and verifies public key commitment.The test properly:
- Computes the commitment using the same
compute_pk_commitmentfunction used in production- Passes the required BFV parameters (degree, plaintext modulus, moduli)
- Includes the
public_key_hashin the expected event for assertion
572-591: Consistent commitment computation across test scenarios.The duplicate e3_id test correctly computes separate commitments for each chain's public key aggregation, ensuring the hash validation works across different E3 instances.
crates/bfv-helpers/src/client.rs (1)
133-158: Robust implementation with proper error handling.The function correctly:
- Builds BFV parameters and deserializes the public key (lines 141-144)
- Computes Greco bounds and bit width (lines 146-147)
- Converts to Greco format and computes commitment (lines 149-150)
- Handles the BigInt to 32-byte conversion with proper error propagation (lines 152-155)
One consideration:
to_bytes_be().1returns the magnitude bytes without sign. Since the commitment is expected to be positive and exactly 32 bytes, this should work correctly, but the error at line 155 will catch any size mismatch.crates/evm/src/ciphernode_registry_sol.rs (1)
406-432: LGTM - Clean propagation of public key hash to registry.The handler correctly:
- Extracts
public_key_hashfrom thePublicKeyAggregatedmessage (line 409)- Passes it through to the async contract call (lines 416-424)
- The function signature update at line 493 accepts
[u8; 32]and converts toFixedBytesat line 498crates/evm-helpers/src/contracts.rs (2)
78-78: Contract ABI updated correctly.The
activatefunction signature in thesol!macro now includes thepublicKeyHashparameter:function activate(uint256 e3Id, bytes calldata publicKey, bytes32 publicKeyHash)This matches the updated Solidity contract interface.
141-146: Trait and implementation are consistent.The
EnclaveWrite::activatemethod signature correctly includespub_key_hash: FixedBytes<32>, and the implementation at lines 408-425 properly passes this parameter to the contract call at line 421.Also applies to: 408-425
packages/enclave-contracts/test/Enclave.spec.ts (2)
58-58: Verify hash function consistency betweenethers.idandethers.keccak256.
dataHashis computed usingethers.id(data)which iskeccak256(toUtf8Bytes(data)), but at line 1601, the assertion usesethers.keccak256(data)which hashes the raw bytes.For
data = "0xda7a":
ethers.id(data)= keccak256 of UTF-8 string "0xda7a"ethers.keccak256(data)= keccak256 of bytes [0xda, 0x7a]If this is intentional (ciphertextOutput stores a different hash than the activation hash), please confirm. Otherwise, consider using consistent hashing:
- const dataHash = ethers.id(data); + const dataHash = ethers.keccak256(data);
64-78: Helper function correctly updated for new signature.The
setupAndPublishCommitteehelper properly:
- Computes
publicKeyHashusingethers.id(publicKey)(line 76)- Passes it as the fourth parameter to
publishCommittee(line 77)packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol (1)
41-46: Mock contracts correctly updated to match interface.Both
MockCiphernodeRegistryandMockCiphernodeRegistryEmptyKeyhave theirpublishCommitteesignatures updated to include the newbytes32parameter, maintaining compatibility with theICiphernodeRegistryinterface.Also applies to: 118-123
packages/enclave-contracts/tasks/enclave.ts (2)
258-263: Good addition of publicKeyHash parameter.The new option is properly structured and documented. The description correctly identifies it as a bytes32 hash.
333-347: Consider documenting why hash handling differs between tasks.The inconsistency is real:
activateE3computespublicKeyHashinline usingethers.id(publicKey)(line 346), whilepublishCommitteerequires it as an input parameter. However, test evidence confirms thatethers.id()is the correct and expected method for computing the hash frompublicKey—the test suite consistently uses this approach for both tasks. This appears to be intentional API design (convenience inactivateE3vs. explicit input inpublishCommittee), but consider adding a comment explaining why the two tasks differ to help future maintainers understand the distinction.
cedoor
force-pushed
the
refactor/public-key
branch
from
df0ddee to
b35f682
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
242-261: Contract trusts owner to provide correct publicKeyHash without validation.The function accepts
publicKeyHashas an external parameter without verifying it matches the providedpublicKeyor committee. While off-chain computation is supported (owner computes and provides the hash), there is no cryptographic proof that the hash actually corresponds to the public key or committee members.The risk is partially mitigated by downstream validation in
Enclave.sol(line 314), which checkspublicKeyHash == registryPublicKeyHash, but this only validates consistency with stored values—not correctness of the original hash.The TODO comments (lines 256-257) correctly identify the need for a proof mechanism to eliminate this trust assumption.
♻️ Duplicate comments (1)
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol (1)
140-141: Validate that committeePublicKey is set.The E3 fetch is correct, but there's no validation that
committeePublicKeyhas been initialized. IfvalidateInputis called before the committee has been activated,committeePublicKeywill bebytes32(0), causing anInvalidNoirProoferror rather than a clear indication of the actual problem.Add validation immediately after fetching E3:
// Fetch E3 to get committee public key E3 memory e3 = enclave.getE3(e3Id); +if (e3.committeePublicKey == bytes32(0)) revert CommitteePublicKeyNotSet();Add the custom error at the contract level:
error CommitteePublicKeyNotSet();The Noir circuit expects
committeePublicKey(namedpk_commitmentin the circuit) as the first public input, which matches the implementation.
🧹 Nitpick comments (1)
crates/events/src/enclave_event/publickey_aggregated.rs (1)
16-22: LGTM - New field correctly added to the event struct.The
public_key_hashfield is appropriately typed as[u8; 32]to hold the commitment hash. Consider adding the same hex formatter forpublic_key_hashas used forpubkeyfor consistent debug output, though this is optional.Optional: Add hex formatting for public_key_hash
pub struct PublicKeyAggregated { #[derivative(Debug(format_with = "e3_utils::formatters::hexf"))] pub pubkey: Vec<u8>, + #[derivative(Debug(format_with = "e3_utils::formatters::hexf"))] pub public_key_hash: [u8; 32], pub e3_id: E3id, pub nodes: OrderedSet<String>, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.locktemplates/default/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
crates/aggregator/Cargo.tomlcrates/aggregator/src/publickey_aggregator.rscrates/bfv-helpers/src/client.rscrates/bfv-helpers/src/utils/greco.rscrates/events/src/enclave_event/publickey_aggregated.rscrates/evm-helpers/src/contracts.rscrates/evm/src/ciphernode_registry_sol.rscrates/tests/Cargo.tomlcrates/tests/tests/integration_legacy.rsexamples/CRISP/circuits/src/main.nrexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solexamples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.solexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/server/src/cli/approve.rsexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/indexer.rsexamples/CRISP/server/src/server/token_holders/etherscan.rspackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-sdk/src/contract-client.tspackages/enclave-sdk/src/enclave-sdk.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/aggregator/Cargo.toml
- packages/enclave-sdk/src/enclave-sdk.ts
- crates/evm/src/ciphernode_registry_sol.rs
- examples/CRISP/packages/crisp-sdk/src/vote.ts
- packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json
- packages/enclave-sdk/src/contract-client.ts
- examples/CRISP/server/src/cli/approve.rs
- packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json
- packages/enclave-contracts/contracts/interfaces/IEnclave.sol
- examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts
🧰 Additional context used
🧠 Learnings (24)
📚 Learning: 2025-12-23T17:18:22.421Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1142
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:141-147
Timestamp: 2025-12-23T17:18:22.421Z
Learning: For Solidity contracts that verify Noir circuit proofs, the verifier's public inputs array must include both the circuit's public inputs and the public outputs. Ensure the verification input array length equals pub_inputs + pub_outputs. Example: a Noir circuit with 3 public inputs and 2064 public outputs should pass an array of length 2067 (3 + 2064). Apply this consistently across all contract verifier implementations and adjust tests accordingly.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.sol
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
examples/CRISP/server/src/cli/commands.rscrates/aggregator/src/publickey_aggregator.rsexamples/CRISP/server/src/server/indexer.rscrates/evm-helpers/src/contracts.rscrates/bfv-helpers/src/utils/greco.rscrates/tests/tests/integration_legacy.rsexamples/CRISP/server/src/server/token_holders/etherscan.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
Applied to files:
examples/CRISP/server/src/cli/commands.rscrates/evm-helpers/src/contracts.rsexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
examples/CRISP/server/src/cli/commands.rscrates/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tscrates/bfv-helpers/src/utils/greco.rscrates/tests/tests/integration_legacy.rsexamples/CRISP/server/src/server/token_holders/etherscan.rscrates/bfv-helpers/src/client.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: tests/basic_integration/test.sh:103-114
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `tests/basic_integration/test.sh`, the user prefers not to refactor the ciphernode addition section to reduce duplication.
Applied to files:
packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts
📚 Learning: 2024-10-12T10:24:07.572Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-12T10:24:07.572Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Applied to files:
crates/aggregator/src/publickey_aggregator.rscrates/tests/tests/integration_legacy.rscrates/events/src/enclave_event/publickey_aggregated.rs
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
crates/bfv-helpers/src/utils/greco.rs
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/bfv-helpers/src/utils/greco.rscrates/bfv-helpers/src/client.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
crates/bfv-helpers/src/utils/greco.rscrates/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-16T09:52:53.807Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/src/data_store.rs:74-75
Timestamp: 2024-10-16T09:52:53.807Z
Learning: In this project, the actor model handles potential errors internally, so methods like `checkpoint` in the `Checkpoint` trait do not need to explicitly handle or propagate errors.
Applied to files:
crates/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-12T10:24:07.572Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/core/src/events.rs:349-349
Timestamp: 2024-10-12T10:24:07.572Z
Learning: When reviewing `PlaintextAggregated` struct instantiations in the project, note that the `src_chain_id` field may not always need to be included. Avoid flagging its absence unless it is required in the specific context.
Applied to files:
crates/events/src/enclave_event/publickey_aggregated.rs
📚 Learning: 2024-10-29T01:04:19.137Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:53-70
Timestamp: 2024-10-29T01:04:19.137Z
Learning: In the `Keyshare` struct within `packages/ciphernode/keyshare/src/keyshare.rs`, the `self.secret` field is stored in encrypted form and is not considered sensitive.
Applied to files:
crates/events/src/enclave_event/publickey_aggregated.rs
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
crates/tests/Cargo.toml
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
crates/tests/Cargo.tomlexamples/CRISP/server/src/server/token_holders/etherscan.rs
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed to support multiple network connections: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3. Similarly, `.revertedWithoutReason(ethers)` replaces `.revertedWithoutReason`.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2024-10-28T12:04:26.578Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave_node/src/ciphernode.rs:26-28
Timestamp: 2024-10-28T12:04:26.578Z
Learning: In the `setup_ciphernode` function in `packages/ciphernode/enclave_node/src/ciphernode.rs`, panicking on errors during setup is acceptable.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-11-12T10:08:30.720Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 996
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:144-169
Timestamp: 2025-11-12T10:08:30.720Z
Learning: In the CRISP program (examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol), there are plans to move the merkle tree from the Enclave contract to the program contract itself in the future, which would allow direct calls to validateInput to be stored in the merkle tree.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol
📚 Learning: 2025-12-19T11:35:43.204Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 1109
File: examples/CRISP/server/src/server/routes/state.rs:33-37
Timestamp: 2025-12-19T11:35:43.204Z
Learning: In the CRISP voting system (examples/CRISP), ciphertext data and slot states are public and stored on-chain in the CRISPProgram smart contract. The server endpoints `/previous-ciphertext` and `/is-slot-empty` expose public blockchain data that can already be accessed by anyone through the contract's external view functions (getSlotIndex, isSlotEmptyByAddress) or by reading contract state directly. Authentication is not needed for these endpoints since they only expose data that is already publicly available on the blockchain.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2025-08-27T13:53:47.832Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: packages/circuits/crates/libs/polynomial/src/lib.nr:121-133
Timestamp: 2025-08-27T13:53:47.832Z
Learning: In Noir circuits, prefer relying on built-in array bounds checking rather than adding explicit preconditions for buffer overflow protection, as the language already provides safety guarantees that will fail the circuit on out-of-bounds access.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2024-10-08T01:50:45.185Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/core/src/events.rs:323-323
Timestamp: 2024-10-08T01:50:45.185Z
Learning: When suggesting that instances of `E3Requested` should include `src_chain_id`, double-check to ensure that the field is actually missing before making the suggestion.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2025-09-11T13:21:31.031Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/tasks/utils.ts:7-8
Timestamp: 2025-09-11T13:21:31.031Z
Learning: In Hardhat v3, the task API syntax has changed significantly from v2. The new syntax uses:
- `.addOption({ name, description, defaultValue, type })` instead of `.addOptionalParam()`
- `.setAction(async () => ({ default: (args, hre) => { ... } }))` instead of direct `.setAction((args, hre) => { ... })`
- `.build()` is required to finalize task definitions
- `ArgumentType.STRING` is used for option types instead of `types.string`
Applied to files:
packages/enclave-contracts/tasks/enclave.ts
📚 Learning: 2025-09-11T13:09:03.800Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/scripts/deployAndSave/naiveRegistryFilter.ts:30-31
Timestamp: 2025-09-11T13:09:03.800Z
Learning: In Hardhat v3 deployment scripts, use `(await signer.provider?.getNetwork())?.name ?? "localhost"` instead of `hre.globalOptions.network` to reliably get the network name, as `hre.globalOptions.network` can be undefined in some contexts.
Applied to files:
packages/enclave-contracts/tasks/enclave.ts
🧬 Code graph analysis (5)
examples/CRISP/server/src/cli/commands.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
crates/aggregator/src/publickey_aggregator.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
examples/CRISP/server/src/server/indexer.rs (2)
examples/CRISP/crates/crisp-constants/src/lib.rs (1)
get_default_paramset(10-13)crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
crates/tests/tests/integration_legacy.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-158)
crates/bfv-helpers/src/client.rs (2)
crates/bfv-helpers/src/utils/greco.rs (1)
bfv_public_key_to_greco(233-283)crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_arc(278-300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build_e3_support_dev
- GitHub Check: integration_prebuild
- GitHub Check: build_crisp_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_integration
- GitHub Check: Build & Push Image
🔇 Additional comments (31)
examples/CRISP/server/src/server/token_holders/etherscan.rs (1)
621-621: LGTM! Import enables test access to CONFIG.The explicit import of CONFIG in the tests module enables the integration tests to access configuration values. While
use super::*;on line 620 already imports CONFIG from the parent module scope (line 16), the explicit import here adds clarity for test readers.examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol (2)
138-138: LGTM! Array size correctly updated.The array size increase from
3 + vote.lengthto4 + vote.lengthcorrectly accounts for the additionalcommitteePublicKeypublic input.
143-148: Remove vote.length from public inputs array; include all 2048 output fields from the circuit return value.The circuit returns
([Polynomial<512>; 2], [Polynomial<512>; 2]), which represents 2048 Field values. According to the Noir proof verification pattern, the public inputs array passed to the verifier must include both the 4 circuit public inputs (pk_commitment, merkle_root, slot_address, is_first_vote) AND all 2048 output fields from the circuit return value, for a total of 2052 values.The current implementation uses
4 + vote.lengthinstead. Thevotearray is part of the Solidity data structure but is not the circuit's output—including it in the public inputs will cause verification to fail if the verifier expects the actual polynomial coefficients.Populate
noirPublicInputswith the 2048 output fields from the proof instead of appending the vote array.⛔ Skipped due to learnings
Learnt from: cedoor Repo: gnosisguild/enclave PR: 1142 File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:141-147 Timestamp: 2025-12-23T17:18:22.421Z Learning: When verifying Noir circuit proofs in Solidity, the public inputs array passed to the verifier must include both the circuit's public inputs AND public outputs. For example, if a Noir circuit has 3 public inputs and returns 2064 public output fields (-> pub [Field; 2064]), the verification array must contain all 2067 values (3 + 2064).examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (2)
14-22: LGTM - Mock contract properly extended.The addition of
committeePublicKeystate variable and setter aligns with the PR's objective to pass public key commitments through the system. The mock implementation is appropriately simple for testing purposes.
38-38: LGTM - Proper integration with E3 struct.The
committeePublicKeyis correctly used in the E3 struct construction, replacing the previous constant zero value with the configurable state variable.crates/evm-helpers/src/contracts.rs (3)
10-10: LGTM - Proper type import and ABI extension.The addition of
FixedBytesto imports and the extension of theactivatefunction signature withbytes32 publicKeyHashcorrectly align with the Solidity contract changes across the codebase.Also applies to: 78-78
141-146: LGTM - Trait signature properly extended.The
activatemethod signature is correctly updated to includepub_key_hash: FixedBytes<32>, maintaining consistency with the contract ABI and enabling proper public key commitment propagation.
408-425: LGTM - Implementation correctly propagates public key hash.The
activateimplementation properly accepts and passes thepub_key_hashparameter to the contract call, maintaining consistency with the trait signature and ABI definition.packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol (1)
41-46: LGTM - Mock contracts consistently updated.Both
MockCiphernodeRegistryandMockCiphernodeRegistryEmptyKeyvariants are correctly updated with the newbytes32parameter inpublishCommittee, maintaining consistency with theICiphernodeRegistryinterface changes.Also applies to: 118-123
crates/tests/Cargo.toml (1)
34-34: LGTM - Dependency addition enables PK commitment computation.The addition of
e3-bfv-helpersas a workspace dependency is appropriate for tests that need to compute public key commitments usingcompute_pk_commitment.packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (2)
346-346: LGTM - Consistent test updates.All
publishCommitteecall sites are consistently updated to includedataHashas the final parameter, properly exercising the extended interface signature.Also applies to: 365-365, 386-386, 513-513
31-31: The review comment is based on a false premise. The fileCiphernodeRegistryOwnable.spec.tshas always usedethers.id(data)on line 31 since its initial introduction. There was no change fromethers.keccak256(data)toethers.id(data).The git history confirms the file was first introduced with this exact code, and no commits have modified this line. Other test files in the codebase (e.g.,
Enclave.spec.ts) consistently useethers.id()for computing hashes of public key data.Additionally, the contract's
publishCommitteefunction does not validate thepublicKeyHashparameter—it simply stores it (see TODO comment: "Currently we trust the owner to publish the correct committee"). The test uses mock data withethers.id()for testing purposes, which is appropriate in this context.Likely an incorrect or invalid review comment.
crates/aggregator/src/publickey_aggregator.rs (3)
9-9: LGTM - Proper import for PK commitment computation.The import of
compute_pk_commitmentfrome3_bfv_helpers::clientenables the aggregator to compute the public key hash for inclusion in the aggregated event.
185-190: LGTM - Proper public key commitment computation.The
compute_pk_commitmentinvocation correctly passes the aggregated public key and FHE parameters to compute the 32-byte commitment hash. The computation placement (after aggregation, before state update and event emission) is appropriate.
203-209: LGTM - Event properly includes public key hash.The
PublicKeyAggregatedevent is correctly constructed with the computedpublic_key_hash, enabling downstream components to use the commitment for verification.packages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.sol (1)
158-164: LGTM - Interface properly extended with documentation.The
publishCommitteefunction signature is correctly extended with thepublicKeyHashparameter, and the documentation is appropriately updated. This change is consistent with the broader refactoring to propagate public key commitments through the system.examples/CRISP/circuits/src/main.nr (1)
56-56: The verifier contract correctly handles the new public input forpk_commitment.The change from
pk_commitment: Fieldtopk_commitment: pub Fieldproperly exposes the public key commitment as a public input. The verifier implementation already passes the complete public inputs array to verification, which includes both the explicit public inputs (pk_commitment, merkle_root, slot_address, is_first_vote) and the circuit's public outputs (the ciphertext components). No additional changes are required.examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (1)
10-17: Verification key updated correctly for the new public input.The
NUMBER_OF_PUBLIC_INPUTSincrease from 2067 to 2068 aligns with adding thepublicKeyHashparameter to the circuit. The VK_HASH and all G1 point coordinates have been regenerated accordingly.Based on learnings, the verifier's public inputs array length should equal
pub_inputs + pub_outputs. Please confirm that the circuit now has 4 public inputs (previously 3) with 2064 public outputs, totaling 2068.examples/CRISP/server/src/cli/commands.rs (1)
211-222: LGTM - PK commitment computation and activation correctly integrated.The public key commitment is properly computed using the BFV parameters from the generated key, and the activation call correctly passes the new
publicKeyHashparameter asFixedBytes. Error handling appropriately maps the Result to a displayable error string.examples/CRISP/server/src/server/indexer.rs (1)
363-380: LGTM - Indexer correctly computes PK commitment for activation.The implementation properly:
- Retrieves default BFV parameters via
get_default_paramset()- Computes the commitment using
compute_pk_commitmentwith the public key from the event- Passes the commitment as
FixedBytes::from(public_key_hash)to the updatedactivatecallThe error handling correctly wraps the commitment computation failure in an
eyreerror.crates/bfv-helpers/src/utils/greco.rs (2)
185-220: LGTM - BFV to Greco coefficient conversion implemented correctly.The centering logic properly:
- Converts BFV coefficients from
[0, qi)to centered form[-(qi-1)/2, (qi-1)/2]- Handles negative modulo reduction correctly by adding
zkp_moduluswhen result is negativeThe
convert_bfv_coefficients_to_grecocorrectly applies the reverse operation to match Greco's expected coefficient ordering.
233-283: LGTM - Public key to Greco conversion correctly extracts and transforms coefficients.The implementation properly:
- Accesses the internal polynomial structure via
pk.c.c[0]andpk.c.c[1]- Clones before calling
change_representation(required since it mutates in-place)- Extracts coefficients per-modulus and per-degree index
- Applies the Greco conversion for each modulus
The test at lines 385-472 provides good coverage by comparing against
GrecoVectors::standard_form()output, ensuring the implementation matches the reference.crates/tests/tests/integration_legacy.rs (2)
223-235: LGTM - Test correctly verifies PublicKeyAggregated with new field.The test properly computes the expected
public_key_hashusingcompute_pk_commitmentand includes it in the expectedPublicKeyAggregatedevent structure for assertion. This ensures the aggregation pipeline correctly produces the commitment hash.
572-634: LGTM - Cross-chain test correctly updated for public_key_hash.The test for duplicate E3 IDs with different chain IDs properly:
- Computes separate
public_key_hashvalues for each chain's public key- Verifies both
PublicKeyAggregatedevents include the correct hashThis ensures the commitment computation works correctly across different E3 instances.
packages/enclave-contracts/contracts/Enclave.sol (1)
298-324: Breaking API change - activate function now requires publicKeyHash parameter.The refactored
activatefunction correctly:
- Accepts
bytes32 publicKeyHashas the third parameter- Verifies it against the registry's stored commitment via
ciphernodeRegistry.committeePublicKey(e3Id)- Stores the hash instead of computing
keccak256(publicKey)on-chainThis is more gas-efficient and enables off-chain commitment computation. The event correctly continues to emit the raw
publicKeyfor clients. TheE3struct andIEnclaveinterface have been properly updated with the new function signature andbytes32 committeePublicKeyfield.packages/enclave-contracts/test/Enclave.spec.ts (2)
942-946: LGTM: Activation signature updated correctly.The test cases correctly pass the new
dataHashparameter toactivate(). The systematic updates across all test scenarios (E3NotReady, E3Expired, E3AlreadyActivated) maintain proper test coverage.Also applies to: 978-983, 1003-1004
64-78: Helper function updated correctly.The
setupAndPublishCommitteehelper properly computespublicKeyHashand passes it topublishCommittee. The implementation is consistent with the new API requirements.packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
260-260: Good practice: Event emits original public key.The event correctly emits the original
publicKeybytes rather than the hash, maintaining transparency and enabling off-chain verification. The hash is stored in state for on-chain operations.packages/enclave-contracts/tasks/enclave.ts (3)
258-263: Good validation added for required parameter.The task properly validates that
publicKeyHashis required and provides a clear error message. This prevents incomplete task execution and guides users to provide the necessary parameter.Also applies to: 290-292
333-334: Hash computation uses ethers.id() - verify consistency.The task computes
publicKeyHashusingethers.id(publicKey), which is consistent with the test file (line 76 in Enclave.spec.ts). However, if the concern raised aboutethers.id()vsethers.keccak256()in the test file is valid, this task would inherit the same issue.Ensure this hash function choice matches the contract's expectations for public key commitments.
Also applies to: 346-347
294-299: Contract call updated correctly.The
publishCommitteecall now includes thepublicKeyHashparameter, matching the updated contract signature. The implementation properly handles the new parameter flow.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/evm-helpers/src/contracts.rs (1)
10-10: Verify the necessity of theFixedBytesimport.The
FixedBytestype is imported but doesn't appear to be used in the visible code. If it's not used elsewhere in this file, consider removing it to keep imports clean.#!/bin/bash # Search for FixedBytes usage in this file rg -n '\bFixedBytes\b' crates/evm-helpers/src/contracts.rspackages/enclave-contracts/contracts/Enclave.sol (1)
298-315: Consider validating the public key hash retrieved from the registry.The activation function now retrieves publicKeyHash from ciphernodeRegistry.committeePublicKey(e3Id) and stores it directly without validation. If the registry returns
bytes32(0)(e.g., if publishCommittee hasn't been called yet or failed), the activation would succeed with an invalid hash, potentially breaking downstream verification.Consider adding validation:
bytes32 publicKeyHash = ciphernodeRegistry.committeePublicKey(e3Id); require(publicKeyHash != bytes32(0), "Committee public key not published");#!/bin/bash # Verify if publishCommittee always sets a non-zero hash before activate can be called ast-grep --pattern $'function publishCommittee($$$) { $$$ committeePublicKey[$_] = $_; $$$ }'examples/CRISP/server/src/cli/commands.rs (1)
15-15: Verify the necessity of new imports.The imports for FixedBytes and compute_pk_commitment have been added, but neither appears to be used in the visible code of activate_e3_round. The function still generates and stores the public key for later decryption but no longer passes it to activate.
If these imports were added in anticipation of future use or are used elsewhere in the file, they're fine. Otherwise, consider removing unused imports.
#!/bin/bash # Verify if these imports are used elsewhere in the file rg -n '\bFixedBytes\b|\bcompute_pk_commitment\b' examples/CRISP/server/src/cli/commands.rsAlso applies to: 19-19
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
crates/evm-helpers/src/contracts.rscrates/indexer/src/indexer.rsexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/indexer.rspackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IE3.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-sdk/src/contract-client.tspackages/enclave-sdk/src/enclave-sdk.tstemplates/default/client/src/pages/steps/ActivateE3.tsxtemplates/default/tests/integration.spec.tstests/integration/base.shtests/integration/persist.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/enclave-contracts/tasks/enclave.ts
- packages/enclave-sdk/src/contract-client.ts
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-11-08T01:31:47.505Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 969
File: packages/enclave-sdk/src/enclave-sdk.ts:101-103
Timestamp: 2025-11-08T01:31:47.505Z
Learning: In packages/enclave-sdk/src/enclave-sdk.ts, the TRBFV protocol parameter setup is intentionally incomplete as of PR #969. The constructor sets BFV_THRESHOLD parameters for TRBFV, but the encryption methods (encryptNumber, encryptVector, encryptNumberAndGenInputs, encryptVectorAndGenInputs) intentionally only support BFV and will throw "Protocol not supported" for TRBFV until future implementation is completed.
Applied to files:
packages/enclave-sdk/src/enclave-sdk.ts
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
examples/CRISP/server/src/cli/commands.rscrates/evm-helpers/src/contracts.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
Applied to files:
examples/CRISP/server/src/cli/commands.rscrates/evm-helpers/src/contracts.rs
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
examples/CRISP/server/src/cli/commands.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol
📚 Learning: 2025-12-23T17:18:22.421Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1142
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:141-147
Timestamp: 2025-12-23T17:18:22.421Z
Learning: For Solidity contracts that verify Noir circuit proofs, the verifier's public inputs array must include both the circuit's public inputs and the public outputs. Ensure the verification input array length equals pub_inputs + pub_outputs. Example: a Noir circuit with 3 public inputs and 2064 public outputs should pass an array of length 2067 (3 + 2064). Apply this consistently across all contract verifier implementations and adjust tests accordingly.
Applied to files:
packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/contracts/interfaces/IE3.solpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IEnclave.sol
📚 Learning: 2024-10-23T01:59:27.215Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: tests/basic_integration/test.sh:21-21
Timestamp: 2024-10-23T01:59:27.215Z
Learning: In `tests/basic_integration/test.sh`, the hardcoded `CIPHERNODE_SECRET` is acceptable for testing purposes and does not need to be changed.
Applied to files:
tests/integration/base.shtests/integration/persist.sh
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: tests/basic_integration/test.sh:103-114
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `tests/basic_integration/test.sh`, the user prefers not to refactor the ciphernode addition section to reduce duplication.
Applied to files:
tests/integration/base.shpackages/enclave-contracts/test/Enclave.spec.tstests/integration/persist.sh
📚 Learning: 2024-09-26T04:24:54.011Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: tests/basic_integration/test.sh:129-129
Timestamp: 2024-09-26T04:24:54.011Z
Learning: In `tests/basic_integration/test.sh`, it's acceptable to not add error handling for the `fake_encrypt.sh` script.
Applied to files:
tests/integration/base.sh
📚 Learning: 2025-10-10T12:56:40.538Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 830
File: templates/default/README.md:123-128
Timestamp: 2025-10-10T12:56:40.538Z
Learning: In the Enclave repository, the hard-coded Hardhat development private key `0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80` is acceptable in template README files and documentation for local testing/interaction purposes.
Applied to files:
tests/integration/base.shpackages/enclave-contracts/test/Enclave.spec.tstests/integration/persist.sh
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2024-10-23T01:59:42.967Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274
Timestamp: 2024-10-23T01:59:42.967Z
Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed to support multiple network connections: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3. Similarly, `.revertedWithoutReason(ethers)` replaces `.revertedWithoutReason`.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
🧬 Code graph analysis (3)
templates/default/client/src/pages/steps/ActivateE3.tsx (2)
packages/enclave-contracts/tasks/enclave.ts (1)
activateE3(308-333)packages/enclave-sdk/src/contract-client.ts (1)
activateE3(164-194)
packages/enclave-sdk/src/enclave-sdk.ts (4)
crates/wasm/init_web.js (1)
initializeWasm(11-29)crates/wasm/init_node.js (1)
initializeWasm(7-9)crates/wasm/src/lib.rs (1)
compute_pk_commitment(59-68)crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-159)
examples/CRISP/server/src/cli/commands.rs (2)
crates/wasm/src/lib.rs (1)
compute_pk_commitment(59-68)crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build_e3_support_risc0
- GitHub Check: integration_prebuild
- GitHub Check: build_ciphernode_image
- GitHub Check: build_sdk
- GitHub Check: build_e3_support_dev
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: test_net
- GitHub Check: build_crisp_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: Build & Push Image
🔇 Additional comments (19)
packages/enclave-contracts/contracts/interfaces/IE3.sol (1)
27-27: Documentation accurately reflects the refactoring to use public key commitment.The documentation correctly describes that
committeePublicKeynow stores a hash/commitment of the public key rather than the raw public key. This aligns with the PR's refactoring objective.All code consuming this field—including the CRISP on-chain proof verification flow—correctly handles it as a commitment hash. Specifically, CRISPProgram.sol properly passes
e3.committeePublicKeyas the first public input to the Noir verifier, which is the intended behavior for proof verification.tests/integration/base.sh (1)
79-79: LGTM: Activation simplified to use commitment-based approach.The removal of the
--public-key-fileparameter aligns with the PR objective to use public key commitments instead of passing raw public keys. The activation now relies on the public key hash that was computed and published during the key aggregation phase.tests/integration/persist.sh (2)
52-58: Same parameter changes as in base.sh - ensure consistency.These parameter changes are identical to
tests/integration/base.sh. Please ensure these test scripts remain synchronized and that the new parameters (especially the plaintext modulus change from 1032193 to 1000) are compatible with the persistence testing flow.
84-84: LGTM: Consistent with commitment-based activation.The removal of the
--public-key-fileparameter is consistent with the changes inbase.shand aligns with the PR's objective to use public key commitments.crates/evm-helpers/src/contracts.rs (1)
78-78: LGTM! Activation signature successfully simplified.The activation flow has been correctly updated to use a single-argument signature (e3_id only), removing the publicKey parameter from the Solidity ABI, trait definition, and implementation. This aligns with the PR's goal of using public key commitments stored in the registry instead of passing raw public keys during activation.
Also applies to: 140-141, 403-415
templates/default/client/src/pages/steps/ActivateE3.tsx (1)
74-74: LGTM! Activation call correctly updated.The activateE3 call has been correctly updated to pass only e3State.id, removing the publicKey parameter. This aligns with the new single-argument activation API.
templates/default/tests/integration.spec.ts (1)
220-225: LGTM! Test correctly updated for single-argument activation.The test has been correctly updated to extract only e3Id from state and pass it as the sole argument to activateE3. The publicKey is still properly retrieved from the state at line 235 for encryption purposes, maintaining the correct separation of concerns.
examples/CRISP/server/src/server/indexer.rs (1)
361-361: LGTM! Activation call correctly updated.The activate call has been correctly updated to pass only event.e3Id. The surrounding context (handle_committee_time_expired is triggered after CommitteePublished event) ensures that the public key hash is already set in the registry before activation is attempted, maintaining the correct flow dependency.
packages/enclave-sdk/src/enclave-sdk.ts (2)
136-146: LGTM! New public key commitment computation method.The new computePublicKeyCommitment method properly initializes WASM, fetches protocol parameters, and delegates to the wasm compute_pk_commitment function. The method signature and return type are appropriate for computing a 32-byte commitment from a public key.
342-348: LGTM! Activation method correctly simplified.The activateE3 method signature has been correctly updated to accept only e3Id and gasLimit, removing the publicKey parameter. The contract client call is properly updated to match.
examples/CRISP/server/src/cli/commands.rs (1)
196-227: LGTM! Activation correctly updated while preserving key storage.The activate call has been correctly updated to pass only e3_id. The function still properly generates and stores the public key (lines 214-217) for later decryption purposes, correctly separating the activation flow from key storage.
packages/enclave-contracts/contracts/interfaces/IEnclave.sol (1)
30-35: LGTM! Interface correctly updated for public key commitment.The interface has been correctly updated: the E3Activated event now uses bytes32 for committeePublicKey (indicating a hash), the documentation clarifies this is a hash, and the activate function signature now accepts only e3Id. These changes align perfectly with the PR's goal of using public key commitments.
Note: This is a breaking change for any contracts implementing IEnclave. Based on the PR scope, all known implementations have been updated.
Also applies to: 155-155
packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol (2)
41-46: LGTM: Mock signature correctly extended.The
publishCommitteesignature in both mock contracts has been correctly updated to accept the newpublicKeyHashparameter, aligning with the updatedICiphernodeRegistryinterface.Also applies to: 120-125
92-92: Improved error handling in mock.The addition of the
CommitteeNotPublishederror and the change from returningbytes32(0)to reverting provides more realistic mock behavior that better matches production contract semantics.Also applies to: 106-108
packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json (2)
80-84: Correct type change for commitment-based approach.The
E3Activatedevent'scommitteePublicKeyfield type changed frombytestobytes32correctly reflects the new commitment-based architecture where only the hash (commitment) is stored on-chain rather than the full public key. This is more gas-efficient and enforces the fixed-size hash constraint.
415-427: New fee token getter added.The
feeToken()view function provides a standard way to query the configured fee token address. Implementation looks correct.packages/enclave-contracts/test/Enclave.spec.ts (3)
941-943: Activation signature correctly updated.All calls to
activate()have been consistently updated fromactivate(e3Id, data)toactivate(e3Id), aligning with the simplified contract interface where activation no longer requires passing the public key or data directly. The key commitment is now retrieved from the registry internally.Also applies to: 975-979, 997-1000, 1034-1037, 1056-1059, 1090-1093, 1108-1111, 1153-1157, 1189-1190, 1222-1223
1574-1577: Correct hash function for ciphertext output verification.Line 1576 correctly uses
ethers.keccak256(data)to verify the ciphertext output hash. This treatsdataas hex bytes, which is appropriate for hashing binary data.
1284-1284: Consistent activation updates throughout test suite.All remaining
activate()calls have been consistently updated to use the single-argument signature. The refactoring is thorough and maintains consistency across all test cases.Also applies to: 1318-1318, 1358-1358, 1379-1379, 1411-1411, 1468-1468, 1503-1503, 1542-1542, 1572-1572, 1602-1602, 1632-1632, 1686-1686, 1715-1715, 1750-1750, 1781-1781, 1813-1813, 1844-1844
ctrlc03
changed the title
ctrlc03
force-pushed
the
refactor/public-key
branch
from
e6dc573 to
25a1916
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol (1)
140-143: Add validation for zero committeePublicKey.The code fetches
e3.committeePublicKeywithout validating it's been set. If committee selection hasn't completed,committeePublicKeywill bebytes32(0), causing anInvalidNoirProoferror instead of a clear indication of the actual problem. This concern was raised in a previous review and remains unaddressed.🔎 Suggested validation
// Fetch E3 to get committee public key E3 memory e3 = enclave.getE3(e3Id); +if (e3.committeePublicKey == bytes32(0)) revert CommitteePublicKeyNotSet(); noirPublicInputs[0] = e3.committeePublicKey;Note: You'll need to add the custom error definition near the other error declarations:
error CommitteePublicKeyNotSet();crates/indexer/src/indexer.rs (2)
389-391: Cleanup error is silently discarded.The result of the temporary storage cleanup is discarded with
let _ =. If cleanup fails, temporary keys will accumulate in the data store.This issue was flagged in a previous review. Consider logging the error:
🔎 Proposed fix
// Remove the temporary storage let mut db_clone = db.clone(); -let _ = db_clone.modify::<Vec<u8>, _>(&temp_key, |_| None).await; +if let Err(e) = db_clone.modify::<Vec<u8>, _>(&temp_key, |_| None).await { + tracing::warn!("Failed to cleanup temporary key {}: {}", temp_key, e); +}
366-380: Consider adding idempotency protection.If
E3Activatedis replayed (due to retries, network issues, or re-indexing), the handler will fail with "CommitteePublished event not found" because the temp key was deleted after the first successful processing.This was flagged in a previous review. Consider checking if the E3 already exists before processing:
let mut repo = E3Repository::new(db.clone(), e3_id); if repo.get_e3().await.is_ok() { info!("E3Activated: E3 {} already processed, skipping", e3_id); return Ok(()); }
🧹 Nitpick comments (4)
examples/CRISP/server/src/server/token_holders/etherscan.rs (1)
621-621: Remove redundant import.
CONFIGis already imported at the file level (line 16) and brought into the tests module viause super::*;(line 620), making this explicit import unnecessary.🔎 Proposed fix
- use crate::server::CONFIG;templates/default/client/src/pages/steps/ActivateE3.tsx (2)
14-20: Update component documentation to reflect API change.The documentation states this component "handles the activation of the E3 using the Ciphernode Committee's shared public key," but line 74 shows that
activateE3is now called without passing the public key. Update the comment to reflect that activation now uses a public key commitment handled by the backend.📝 Suggested documentation update
/** * ActivateE3 component - Third step in the Enclave wizard flow * - * This component handles the activation of the E3 using the Ciphernode Committee's - * shared public key. It provides feedback on the activation process and displays + * This component handles the activation of the E3. The backend uses the Ciphernode + * Committee's public key commitment for verification. It provides feedback on the + * activation process and displays * the status of the activation. */
137-146: Clarify button state and messaging after API change.The button is disabled when
!e3State.publicKeyand displays "Waiting for Committee Key..." (lines 144-145), yet the activation call on line 74 no longer passes the public key. This may confuse users who see the key-related messaging but don't understand why it's needed if not directly used.If waiting for the public key is still required (e.g., the backend needs it available to compute the commitment), consider updating the UI text to clarify this:
💡 Suggested UI text clarification
: !e3State.publicKey - ? 'Waiting for Committee Key...' + ? 'Waiting for Committee Key Setup...' : 'Activate E3 Environment'}Alternatively, if the public key check is no longer necessary after the API change, update the button's disabled condition:
disabled={isRequesting || e3State.isActivated || !e3State.publicKey} + // Note: Verify if publicKey check is still needed after API refactoringcrates/wasm/src/lib.rs (1)
46-68: Consider documenting the expected commitment size.The function documentation could mention that the returned commitment is 32 bytes, matching the
bytes32type used on-chain. This would help callers understand the expected output format.📝 Suggested documentation enhancement
#[wasm_bindgen] /// A function to compute the public key commitment for a given public key. /// /// # Arguments /// /// * `public_key` - The public key to compute the commitment for +/// * `degree` - Polynomial degree for BFV parameters +/// * `plaintext_modulus` - Plaintext modulus for BFV parameters +/// * `moduli` - Moduli for BFV parameters /// /// # Returns -/// Returns a `Result<Vec<u8>, JsValue>` containing the commitment and any errors. +/// Returns a `Result<Vec<u8>, JsValue>` containing the 32-byte commitment and any errors. /// /// # Panics /// /// Panics if the public key cannot be computed
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.locktemplates/default/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
crates/aggregator/Cargo.tomlcrates/aggregator/src/publickey_aggregator.rscrates/bfv-helpers/src/client.rscrates/bfv-helpers/src/utils/greco.rscrates/events/src/enclave_event/publickey_aggregated.rscrates/evm-helpers/src/contracts.rscrates/evm-helpers/src/events.rscrates/evm/src/ciphernode_registry_sol.rscrates/indexer/Cargo.tomlcrates/indexer/src/indexer.rscrates/indexer/tests/fixtures/fake_enclave.solcrates/indexer/tests/integration.rscrates/tests/Cargo.tomlcrates/tests/tests/integration_legacy.rscrates/wasm/src/lib.rsexamples/CRISP/circuits/src/main.nrexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solexamples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.solexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/server/src/cli/approve.rsexamples/CRISP/server/src/cli/commands.rsexamples/CRISP/server/src/server/indexer.rsexamples/CRISP/server/src/server/token_holders/etherscan.rspackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/interfaces/IE3.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-sdk/src/contract-client.tspackages/enclave-sdk/src/enclave-sdk.tstemplates/default/client/src/pages/steps/ActivateE3.tsxtemplates/default/tests/integration.spec.tstests/integration/base.shtests/integration/persist.sh
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/integration/persist.sh
- crates/aggregator/Cargo.toml
- crates/events/src/enclave_event/publickey_aggregated.rs
- crates/evm-helpers/src/events.rs
- packages/enclave-sdk/src/contract-client.ts
- templates/default/tests/integration.spec.ts
- examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol
- crates/tests/Cargo.toml
- examples/CRISP/server/src/server/indexer.rs
- crates/indexer/tests/fixtures/fake_enclave.sol
- packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol
- tests/integration/base.sh
- crates/indexer/Cargo.toml
🧰 Additional context used
🧠 Learnings (29)
📓 Common learnings
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-12T10:24:07.572Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-08T01:48:49.778Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/tests/tests/integration_legacy.rscrates/indexer/tests/integration.rsexamples/CRISP/server/src/cli/approve.rscrates/indexer/src/indexer.rscrates/evm/src/ciphernode_registry_sol.rsexamples/CRISP/server/src/server/token_holders/etherscan.rscrates/bfv-helpers/src/utils/greco.rsexamples/CRISP/server/src/cli/commands.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
crates/tests/tests/integration_legacy.rscrates/indexer/tests/integration.rsexamples/CRISP/server/src/server/token_holders/etherscan.rscrates/bfv-helpers/src/utils/greco.rscrates/bfv-helpers/src/client.rspackages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2024-10-16T09:52:53.807Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/src/data_store.rs:74-75
Timestamp: 2024-10-16T09:52:53.807Z
Learning: In this project, the actor model handles potential errors internally, so methods like `checkpoint` in the `Checkpoint` trait do not need to explicitly handle or propagate errors.
Applied to files:
crates/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-03T23:02:41.732Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-03T23:02:41.732Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
crates/tests/tests/integration_legacy.rscrates/indexer/tests/integration.rscrates/bfv-helpers/src/utils/greco.rs
📚 Learning: 2024-10-08T01:48:49.778Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-08T01:48:49.778Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Applied to files:
crates/tests/tests/integration_legacy.rscrates/evm/src/ciphernode_registry_sol.rscrates/aggregator/src/publickey_aggregator.rs
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/indexer/tests/integration.rscrates/wasm/src/lib.rscrates/bfv-helpers/src/client.rs
📚 Learning: 2025-11-08T01:31:47.505Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 969
File: packages/enclave-sdk/src/enclave-sdk.ts:101-103
Timestamp: 2025-11-08T01:31:47.505Z
Learning: In packages/enclave-sdk/src/enclave-sdk.ts, the TRBFV protocol parameter setup is intentionally incomplete as of PR #969. The constructor sets BFV_THRESHOLD parameters for TRBFV, but the encryption methods (encryptNumber, encryptVector, encryptNumberAndGenInputs, encryptVectorAndGenInputs) intentionally only support BFV and will throw "Protocol not supported" for TRBFV until future implementation is completed.
Applied to files:
crates/indexer/tests/integration.rscrates/wasm/src/lib.rspackages/enclave-sdk/src/enclave-sdk.tspackages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
crates/indexer/src/indexer.rscrates/bfv-helpers/src/client.rspackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol
📚 Learning: 2025-12-17T05:58:25.984Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 1086
File: crates/events/src/eventstore.rs:21-29
Timestamp: 2025-12-17T05:58:25.984Z
Learning: In the EventStore implementation (crates/events/src/eventstore.rs), if log.append succeeds but index.insert fails in handle_store_event_requested, the event is persisted in the log but not indexed. This is intentional design: such events are ignored during sync operations, as the index is the source of truth for timestamp-based event retrieval.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
Applied to files:
crates/indexer/src/indexer.rsexamples/CRISP/server/src/cli/commands.rscrates/evm-helpers/src/contracts.rsexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solpackages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2024-10-08T01:50:45.185Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/core/src/events.rs:323-323
Timestamp: 2024-10-08T01:50:45.185Z
Learning: When suggesting that instances of `E3Requested` should include `src_chain_id`, double-check to ensure that the field is actually missing before making the suggestion.
Applied to files:
crates/indexer/src/indexer.rsexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2024-10-22T03:47:04.014Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/e3_request_router.rs:202-235
Timestamp: 2024-10-22T03:47:04.014Z
Learning: In `packages/ciphernode/router/src/e3_request_router.rs`, within the `E3RequestRouter::from_snapshot` method, errors during context restoration propagate upwards due to the `?` operator, and skipping contexts when `repositories.context(&e3_id).read().await?` returns `Ok(None)` is acceptable, as missing snapshots are expected.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2025-04-30T06:25:14.721Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 372
File: packages/ciphernode/events/src/eventbus.rs:15-15
Timestamp: 2025-04-30T06:25:14.721Z
Learning: EnclaveEvent implements Display in packages/ciphernode/events/src/enclave_event/mod.rs, which satisfies the Event trait requirement. Static analysis tools may incorrectly flag implementations as missing when they do exist.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-08T07:15:06.690Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/evm/src/ciphernode_registry_sol.rs:133-143
Timestamp: 2024-10-08T07:15:06.690Z
Learning: In `packages/ciphernode/evm/src/ciphernode_registry_sol.rs`, the function `helpers::stream_from_evm` in Rust returns `()`, not a `Result`, so error handling with `if let Err(e) = ...` is not applicable.
Applied to files:
crates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
crates/evm/src/ciphernode_registry_sol.rsexamples/CRISP/server/src/cli/commands.rs
📚 Learning: 2025-11-12T10:08:30.720Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 996
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:144-169
Timestamp: 2025-11-12T10:08:30.720Z
Learning: In the CRISP program (examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol), there are plans to move the merkle tree from the Enclave contract to the program contract itself in the future, which would allow direct calls to validateInput to be stored in the merkle tree.
Applied to files:
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 752
File: packages/enclave-contracts/contracts/Enclave.sol:15-17
Timestamp: 2025-09-19T11:16:53.825Z
Learning: The Enclave contract in the gnosisguild/enclave repository has not been deployed yet as of September 2025, so storage layout considerations for upgradeable contracts don't apply to current changes.
Applied to files:
packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/contracts/interfaces/IEnclave.sol
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
examples/CRISP/server/src/server/token_holders/etherscan.rs
📚 Learning: 2024-10-23T01:59:42.967Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274
Timestamp: 2024-10-23T01:59:42.967Z
Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
Applied to files:
examples/CRISP/server/src/server/token_holders/etherscan.rspackages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
crates/bfv-helpers/src/utils/greco.rs
📚 Learning: 2025-12-23T17:18:22.421Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1142
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:141-147
Timestamp: 2025-12-23T17:18:22.421Z
Learning: For Solidity contracts that verify Noir circuit proofs, the verifier's public inputs array must include both the circuit's public inputs and the public outputs. Ensure the verification input array length equals pub_inputs + pub_outputs. Example: a Noir circuit with 3 public inputs and 2064 public outputs should pass an array of length 2067 (3 + 2064). Apply this consistently across all contract verifier implementations and adjust tests accordingly.
Applied to files:
packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solpackages/enclave-contracts/contracts/interfaces/IE3.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.sol
📚 Learning: 2025-12-19T11:35:51.645Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 1109
File: examples/CRISP/server/src/server/routes/state.rs:33-37
Timestamp: 2025-12-19T11:35:51.645Z
Learning: In the CRISP voting system (examples/CRISP), ciphertext data and slot states are public and stored on-chain in the CRISPProgram smart contract. The server endpoints `/previous-ciphertext` and `/is-slot-empty` expose public blockchain data that can already be accessed by anyone through the contract's external view functions (getSlotIndex, isSlotEmptyByAddress) or by reading contract state directly. Authentication is not needed for these endpoints since they only expose data that is already publicly available on the blockchain.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
📚 Learning: 2025-09-11T13:21:31.031Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/tasks/utils.ts:7-8
Timestamp: 2025-09-11T13:21:31.031Z
Learning: In Hardhat v3, the task API syntax has changed significantly from v2. The new syntax uses:
- `.addOption({ name, description, defaultValue, type })` instead of `.addOptionalParam()`
- `.setAction(async () => ({ default: (args, hre) => { ... } }))` instead of direct `.setAction((args, hre) => { ... })`
- `.build()` is required to finalize task definitions
- `ArgumentType.STRING` is used for option types instead of `types.string`
Applied to files:
packages/enclave-contracts/tasks/enclave.ts
📚 Learning: 2025-10-10T12:56:40.538Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 830
File: templates/default/README.md:123-128
Timestamp: 2025-10-10T12:56:40.538Z
Learning: In the Enclave repository, the hard-coded Hardhat development private key `0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80` is acceptable in template README files and documentation for local testing/interaction purposes.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2024-09-26T04:12:09.345Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: tests/basic_integration/test.sh:103-114
Timestamp: 2024-09-26T04:12:09.345Z
Learning: In `tests/basic_integration/test.sh`, the user prefers not to refactor the ciphernode addition section to reduce duplication.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/test/Enclave.spec.ts:804-807
Timestamp: 2025-09-11T12:56:39.601Z
Learning: In Hardhat v3, the chai matchers syntax changed to support multiple network connections: `.to.not.be.revert(ethers)` is the correct new syntax, replacing the old `.to.not.be.reverted` (without parameters). The ethers object must be passed as a parameter to the revert/reverted matchers in Hardhat v3. Similarly, `.revertedWithoutReason(ethers)` replaces `.revertedWithoutReason`.
Applied to files:
packages/enclave-contracts/test/Enclave.spec.ts
🧬 Code graph analysis (7)
crates/tests/tests/integration_legacy.rs (2)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-171)crates/wasm/src/lib.rs (1)
compute_pk_commitment(59-68)
crates/indexer/src/indexer.rs (2)
examples/CRISP/server/src/server/indexer.rs (2)
register_committee_published(318-354)register_e3_activated(182-211)packages/enclave-sdk/src/types.ts (1)
E3(120-134)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (1)
examples/CRISP/packages/crisp-contracts/tests/utils.ts (2)
deployMockEnclave(43-47)deployCRISPProgram(69-87)
packages/enclave-sdk/src/enclave-sdk.ts (4)
crates/wasm/init_web.js (1)
initializeWasm(11-29)crates/wasm/init_node.js (1)
initializeWasm(7-9)crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-171)crates/wasm/src/lib.rs (1)
compute_pk_commitment(59-68)
crates/bfv-helpers/src/client.rs (3)
crates/bfv-helpers/src/utils/greco.rs (1)
bfv_public_key_to_greco(233-283)crates/wasm/src/lib.rs (1)
compute_pk_commitment(59-68)crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_arc(278-300)
examples/CRISP/server/src/cli/commands.rs (1)
crates/indexer/src/indexer.rs (1)
contract(185-187)
crates/aggregator/src/publickey_aggregator.rs (1)
crates/bfv-helpers/src/client.rs (1)
compute_pk_commitment(133-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: build_sdk
- GitHub Check: build_crisp_sdk
- GitHub Check: rust_unit
- GitHub Check: test_net
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_integration
🔇 Additional comments (55)
examples/CRISP/server/src/cli/approve.rs (1)
28-28: LGTM: Import style improvement.Removing the braces for a single import aligns with idiomatic Rust conventions and improves code consistency.
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol (1)
138-149: Circuit correctly updated with 4 public inputs.The Noir circuit in
examples/CRISP/circuits/src/main.nrhas been properly updated to accept the 4 public inputs matching the Solidity implementation:pk_commitment,merkle_root,slot_address, andis_first_vote, followed by the vote outputs array. The circuit and contract are aligned.templates/default/client/src/pages/steps/ActivateE3.tsx (1)
66-69: The publicKey guard check is necessary and should be preserved.Although
activateE3()doesn't directly use the publicKey parameter, the check is still essential because the publicKey is required in the subsequent step (EnterInputs) to encrypt user inputs. The guard ensures the workflow is in a complete state before proceeding to activation, preventing a scenario where activation succeeds but the system isn't ready for the following encryption step.examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (2)
74-75: LGTM! Test setup correctly wires the mock enclave.The mock enclave deployment and injection into
deployCRISPProgramaligns with the updated signature and enables the test to control the committee public key for proof verification.
97-97: No changes needed.The code is correct as written.
proof.publicInputs[0]contains the committee public key commitment (the first public input declared in the Noir circuit), and passing it tosetCommitteePublicKeyis the right approach for setting up the test enclave state before proof verification.examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (3)
14-17: Verification keypublicInputsSizeis consistent with the constant.The
publicInputsSize: uint256(2068)matchesNUMBER_OF_PUBLIC_INPUTSat line 10, maintaining consistency within the verification key initialization.
10-11: Constants updated correctly for public key commitment support.The increment from 2067 to 2068 public inputs is correct and aligns with the PR objective of adding
committeePublicKeyto the verification flow. The Noir circuit now passes 4 public inputs (pk_commitment, merkle_root, slot_address, is_first_vote) instead of 3, with the remaining 2064 fields representing public outputs, totaling 2068 values. This is confirmed by CRISPProgram.sol which constructs the public inputs array with exactly 4 fixed elements plus the vote array at indices 4 onwards.The
VK_HASHand G1Point coordinate updates require verification against circuit compilation output to ensure they match the updated circuit's verification key, which is not available in the repository.
18-129: G1Point coordinates are correctly auto-generated from the Noir circuit.These precomputed elliptic curve points are auto-generated via the compilation pipeline:
nargo compile→bb write_vk→bb write_solidity_verifier. The circuit includespk_commitment(committeePublicKey) as a public input, and the verification key structure inloadVerificationKey()is guaranteed to match the circuit's compiled artifacts. The public inputs array passed to the verifier correctly includes both the circuit's 4 public inputs (committeePublicKey, merkleRoot, slotAddress, isFirstVote) and the circuit's public outputs (ciphertext components), per Noir verification specification.crates/wasm/src/lib.rs (1)
46-68: LGTM! Clean WASM wrapper with proper error handling.The function follows the established pattern in this file, properly delegates to the internal implementation, and handles errors appropriately.
packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json (2)
79-84: LGTM! ABI correctly reflects E3Activated event signature change.The event ABI properly represents the change from
bytestobytes32for thecommitteePublicKeyparameter, matching the updated Solidity interface.
415-427: New feeToken() getter function added.A view function to retrieve the fee token address has been added to the ABI. While not directly related to the publicKeyHash refactoring, this is a safe addition that exposes existing state.
packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol (4)
41-46: LGTM! Mock signature updated to match interface changes.The
publishCommitteefunction signature correctly adds thebytes32parameter for the public key hash, aligning with the updatedICiphernodeRegistryinterface.
92-93: Good practice: custom error for clearer failure signaling.The new
CommitteeNotPublishederror provides a clearer signal than returning a zero value, improving test reliability and gas efficiency.
120-125: LGTM! Consistent signature update across both mock contracts.The
publishCommitteesignature inMockCiphernodeRegistryEmptyKeymatches the update inMockCiphernodeRegistry, ensuring consistency.
106-108: Improved mock behavior: revert instead of returning zero.The change from returning
bytes32(0)to reverting withCommitteeNotPublished()makes the mock more realistic and prevents accidental use of zero as a valid public key hash. Tests usingMockCiphernodeRegistryEmptyKeycorrectly expect this revert behavior.packages/enclave-contracts/contracts/interfaces/IEnclave.sol (1)
148-155: Breaking change:activate()function signature simplified.The
publicKeyparameter has been removed. The function now relies on the registry to provide the public key commitment internally, consistent with the shift to using public key hashes.All callers have been properly updated to the new single-parameter signature across the codebase (tasks/enclave.ts and test files).
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
283-284: Indices are correct and align with the updated circuit structure.The shift from
slice(3)toslice(4)and frompublicInputs[1]topublicInputs[2]correctly reflects the updated circuit layout wherepk_commitmentwas added as the first public input. The circuit defines four public inputs in order:pk_commitment(index 0),merkle_root(index 1),slot_address(index 2), andis_first_vote(index 3), with the vote array starting at index 4. The indices in this code match both the circuit's public input ordering and the contract's verification setup.packages/enclave-contracts/contracts/interfaces/IE3.sol (1)
27-27: Documentation update aligns with public key hash refactoring.The NatSpec correctly reflects that
committeePublicKeynow stores a hash rather than the raw public key, consistent with the PR's shift to commitment-based verification.examples/CRISP/circuits/src/main.nr (1)
56-56: Public input visibility enables on-chain commitment verification.Making
pk_commitmenta public input allows the on-chain verifier to validate that proofs are generated against the correct committee public key. This change aligns with the PR objective to integrate public key commitments into CRISP's proof verification flow.crates/indexer/tests/integration.rs (3)
46-47: Test uses appropriate insecure parameter set.Using
InsecureSet2048_1032193_1for testing is appropriate as it provides faster test execution while maintaining the same API surface. The "Insecure" prefix clearly indicates this is not for production use.
97-107: Public key commitment computation correctly integrated.The test properly generates a BFV keypair and computes the public key commitment using
compute_pk_commitmentwith all required parameters (public key bytes, degree, plaintext modulus, and moduli). This accurately reflects the production flow introduced in this PR.
112-124: Test validates two-phase activation flow with public key commitment.The test correctly implements the new flow:
- Publish committee public key via
emitCommitteePublished(lines 112-118)- Activate E3 with the 32-byte public key commitment via
emitE3Activated(lines 120-129)This two-phase approach mirrors the production behavior where the committee public key is published off-chain and referenced on-chain via its commitment hash.
packages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.sol (1)
159-164: Breaking API change topublishCommitteesignature properly implemented.The addition of the
publicKeyHashparameter is a breaking API change to the function signature. All implementations and call sites have been updated:
- Interface and concrete implementation include the new parameter
- Test files (Enclave.spec.ts, CiphernodeRegistryOwnable.spec.ts) pass the computed hash
- Hardhat task passes the hash as a required parameter
- Rust integration code provides the hash parameter
- Internal
keccak256(publicKey)computation has been removed; hash computation responsibility is now on the caller (e.g.,ethers.id(publicKey)in tests)examples/CRISP/server/src/cli/commands.rs (1)
209-210: Activation signature correctly simplified to single e3_id parameter.The removal of the
pk_bytesparameter fromcontract.activate()is correct. However, note that the generated public key (lines 200-201) is stored locally in the database, not sent to the contract. The public key is retrieved from the smart contract's state via the/rounds/public-keyAPI endpoint when needed by participants.Likely an incorrect or invalid review comment.
packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (3)
346-347: LGTM!The
publishCommitteecalls have been correctly updated to include the newdataHashparameter, consistent with the updated contract interface.Also applies to: 365-366, 386-387, 513-514
367-369: LGTM!Test assertions correctly validate that
committeePublicKeynow stores thedataHashinstead of the raw public key, aligning with the contract's updated behavior.Also applies to: 515-517
31-31: The fileCiphernodeRegistryOwnable.spec.tswas created withethers.id(data)in the initial commit—this was not a change fromethers.keccak256(). The review comment's assumption about a transition between hash functions is unfounded.While
ethers.id()is semantically incorrect for hashing binary data (ethers.id()treats input as a UTF-8 string viakeccak256(toUtf8Bytes(text)), whereasdata = "0xda7a"represents raw bytes), this is an original design choice, not a problematic modification.Likely an incorrect or invalid review comment.
packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json (1)
375-380: LGTM!The ABI correctly reflects the addition of the
publicKeyHashparameter to thepublishCommitteefunction. The parameter is appropriately placed at the end of the parameter list, which is good practice for maintaining backward compatibility where possible.crates/aggregator/src/publickey_aggregator.rs (2)
185-190: LGTM!The
compute_pk_commitmentcall correctly passes the aggregated public key and FHE parameters to generate the 32-byte commitment. Error handling via the?operator is appropriate, and thepubkey.clone()is necessary sincepubkeyis used again for state update.
203-209: LGTM! ThePublicKeyAggregatedevent structure is correctly implemented.The event includes both the raw
pubkeyand the computedpublic_key_hash, allowing off-chain components to use the raw public key while on-chain components can verify the commitment. All four fields (pubkey,public_key_hash,e3_id,nodes) match the struct definition and are properly populated.packages/enclave-sdk/src/enclave-sdk.ts (1)
136-146: LGTM!The
computePublicKeyCommitmentmethod correctly initializes WASM, retrieves protocol parameters, and invokes the WASM binding. The parameter conversion usingBigUint64Array.fromfor moduli is appropriate. The WASM binding correctly converts the fixed-size array[u8; 32]from the underlying function toVec<u8>via.to_vec()before returning, which properly maps to the TypeScriptPromise<Uint8Array>return type.crates/tests/tests/integration_legacy.rs (4)
11-11: LGTM!The import for
compute_pk_commitmentis correctly added to support the new public key hash computation in the tests.
224-236: LGTM!The public key hash computation and its inclusion in the
PublicKeyAggregatedevent are correctly implemented. The parameters passed tocompute_pk_commitment(public key bytes, degree, plaintext modulus, and moduli) match the function signature and correctly derive the 32-byte commitment.
572-592: LGTM!The pattern for computing and including
public_key_hashin the duplicate E3 ID test is consistent with the other test cases.
615-635: LGTM!The second E3 with a different chain ID correctly computes its own
public_key_hashand includes it in thePublicKeyAggregatedassertion.crates/indexer/src/indexer.rs (3)
22-24: LGTM!The
CommitteePublishedevent is correctly added to the imports to support the new event handling flow.
334-358: LGTM!The
register_committee_publishedhandler correctly stores the committee public key in temporary storage keyed bye3_id. This approach enables the E3Activated handler to retrieve the full public key while only the hash is passed on-chain.
491-499: LGTM!The listener registration order is correct—
register_committee_publishedis called beforeregister_e3_activated, ensuring the temporary storage is populated before the activated event attempts to read it.crates/bfv-helpers/src/client.rs (1)
133-171: LGTM!The
compute_pk_commitmentfunction is well-implemented. The left-padding logic (lines 162-168) correctly handles BigInt values that produce fewer than 32 bytes, addressing the concern raised in a previous review. The function properly:
- Deserializes the BFV public key with appropriate error handling
- Computes Greco bounds and bit width for the commitment
- Converts the public key to Greco format and computes the commitment
- Ensures a consistent 32-byte output with proper padding
crates/evm/src/ciphernode_registry_sol.rs (3)
12-12: LGTM!
FixedBytesis correctly imported to support the 32-byte public key hash handling.
406-424: LGTM!The
PublicKeyAggregatedhandler correctly extractspublic_key_hashfrom the message and passes it topublish_committee_to_registry. The async block properly captures and forwards all necessary data.
487-515: LGTM!The
publish_committee_to_registryfunction is correctly updated to:
- Accept the new
public_key_hash: [u8; 32]parameter- Convert it to
FixedBytesfor the contract call- Pass it to the
publishCommitteecontract methodThe implementation aligns with the updated contract interface.
crates/bfv-helpers/src/utils/greco.rs (4)
9-9: LGTM!
PublicKeyis correctly added to the imports to support the newbfv_public_key_to_grecofunction.
185-220: LGTM!The helper functions for BFV to Greco coefficient conversion are correctly implemented:
convert_bfv_coefficient_to_centeredproperly centers coefficients from[0, qi)to[-(qi-1)/2, (qi-1)/2]convert_bfv_coefficients_to_grecocorrectly applies the centering, reversal, and modular reduction with proper handling of negative values
233-283: LGTM!The
bfv_public_key_to_grecofunction correctly:
- Accesses the internal polynomial structure of the
PublicKeyviapk.c.c[0]andpk.c.c[1]- Converts polynomials to power basis representation for coefficient extraction
- Iterates over each modulus and extracts coefficients per degree
- Applies the Greco conversion (centering, reversing, modular reduction)
The implementation mirrors the inverse of
greco_to_bfv_ciphertextand aligns with theGrecoVectorsstandard form output as validated by the test.
385-472: LGTM!The test thoroughly validates
bfv_public_key_to_grecoby comparing its output against the expected values fromGrecoVectors::compute. It checks both structure (vector lengths) and content (coefficient equality) for bothpk0isandpk1is.crates/evm-helpers/src/contracts.rs (3)
78-78: LGTM!The Solidity ABI definition is correctly updated to reflect the simplified
activate(uint256 e3Id)signature, removing the public key parameter as per the PR objectives.
140-141: LGTM!The
EnclaveWritetrait correctly reflects the simplifiedactivatemethod signature. The documentation is appropriately updated.
403-415: LGTM!The
activateimplementation is correctly updated to use the single-parameter contract call. Nonce handling, wallet address validation, and error propagation remain properly implemented.packages/enclave-contracts/tasks/enclave.ts (3)
258-263: LGTM!The new
publicKeyHashoption is correctly added with appropriate metadata (type STRING, description indicating bytes32 format).
265-299: LGTM!The
publishCommitteeaction correctly:
- Accepts the new
publicKeyHashparameter- Validates that
publicKeyHashis provided (throws if missing)- Passes it to the
ciphernodeRegistry.publishCommitteecallThe validation ensures the required parameter is not omitted.
308-333: LGTM!The
activateE3task is correctly simplified:
- Removed
publicKeyandpublicKeyFileoptions (no longer needed)- Action signature now only accepts
e3Idenclave.activatecall uses onlye3IdparameterThis aligns with the contract change where activation no longer requires the raw public key.
packages/enclave-contracts/test/Enclave.spec.ts (3)
63-77: LGTM: Test helper correctly updated for new contract signature.The
setupAndPublishCommitteehelper now computespublicKeyHashand passes it as the fourth parameter topublishCommittee, which aligns with the updated contract interface described in the PR objectives.
941-1844: Excellent: Allactivate()calls correctly migrated to single-argument form.All 27+ invocations of
activate()throughout the test suite have been updated fromactivate(e3Id, data)toactivate(e3Id), correctly reflecting the API change. Test coverage includes success paths, error cases (E3DoesNotExist, E3NotReady, E3Expired, E3AlreadyActivated, CommitteeNotPublished), static calls, and event emissions.
1576-1576: LGTM: Correct hash function for ciphertext output validation.The assertion correctly uses
ethers.keccak256(data)to verify the ciphertext output hash, which matches Solidity'skeccak256behavior for raw byte hashing.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-sdk/README.md (1)
247-261: Document thecomputePublicKeyCommitmentutility with correct signature.
computePublicKeyCommitmentis exposed as part of the SDK's public key commitment workflow, but it is missing from the Utilities section. Add documentation for this function to help developers understand how to compute public key commitments.The function takes only a
publicKeyparameter; the degree, plaintextModulus, and moduli are automatically fetched from the protocol parameters internally.🔎 Suggested addition to Utilities section
#### Utilities ```typescript // Gas estimation const gas = await sdk.estimateGas(functionName, args, contractAddress, abi, value?); // Transaction waiting const receipt = await sdk.waitForTransaction(hash); +// Compute public key commitment +const publicKeyCommitment = await sdk.computePublicKeyCommitment(publicKey); // Configuration updates sdk.updateConfig(newConfig: Partial<SDKConfig>); // Cleanup sdk.cleanup();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/evm-helpers/tests/fixtures/fake_enclave.soldocs/pages/setting-up-server.mdxpackages/enclave-sdk/README.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-12T10:24:07.572Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-08T01:48:49.778Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
Applied to files:
crates/evm-helpers/tests/fixtures/fake_enclave.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build_crisp_sdk
- GitHub Check: build_sdk
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: rust_integration
- GitHub Check: build_ciphernode_image
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
🔇 Additional comments (4)
crates/evm-helpers/tests/fixtures/fake_enclave.sol (2)
10-10: LGTM! Type change aligns with commitment-based architecture.The parameter type change from
bytestobytes32correctly reflects the shift to passing a fixed-size public key commitment (hash) instead of the full public key, as outlined in the PR objectives.
17-19: LGTM! Function signature correctly updated.The function signature and emit call are correctly updated to match the new
bytes32event parameter type, maintaining consistency with the E3Activated event declaration.packages/enclave-sdk/README.md (1)
220-220: activateE3 signature correctly updated to remove publicKey parameter.The API signature change aligns with the PR objective to use precomputed public key commitments instead of embedding keys at activation time.
docs/pages/setting-up-server.mdx (1)
268-268: The API signature change is correct, but the review comment misidentifies what was removed.The
activateE3(e3Id)call at line 268 is correct. However, the review comment incorrectly claims that thepublicKeyparameter was removed. The actual API signature isactivateE3(e3Id: bigint, gasLimit?: bigint), wheregasLimitis the optional second parameter—notpublicKey. There is no evidence in the codebase thatpublicKeywas ever a parameter foractivateE3. Public key handling in the SDK is separate and applies to encryption methods likeencryptNumber()andencryptVector(), not activation.The code change itself is valid and requires no modifications.
Likely an incorrect or invalid review comment.
2 participants
Closes #1065
Closes #1066
Summary by CodeRabbit
New Features
Improvements
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.