feat: ciphernode economic contracts [skip-line-limit]#766
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
WalkthroughThis PR introduces a comprehensive bonding registry system that replaces the naive registry filter with sophisticated operator lifecycle management. It adds ERC20 fee-based payments, implements operator licensing with time-locked exit queues, introduces ticket-based sortition mechanics, expands the event system for committee and operator state tracking, and integrates these components across Solidity contracts, Rust actors, deployment infrastructure, and client SDKs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Enclave as Enclave Contract
participant BondingReg as BondingRegistry
participant TicketToken as EnclaveTicketToken
participant CiphernodeReg as CiphernodeRegistry
participant Sortition as Sortition Actor
User->>Enclave: requestE3(E3RequestParams)
Enclave->>Enclave: getE3Quote() → fee
Enclave->>TicketToken: transferFrom(user, fee)
Enclave->>CiphernodeReg: requestCommittee(e3Id, seed, threshold)
CiphernodeReg->>CiphernodeReg: store request + deadline
CiphernodeReg->>CiphernodeReg: emit CommitteeRequested
Sortition->>Sortition: listen for CommitteeRequested
Sortition->>Sortition: schedule finalization after deadline
rect Committee Formation Window
Note over Sortition: submitTicket (multiple nodes)
Note over Sortition: compute scores, finalize top-N
end
Sortition->>CiphernodeReg: emit CommitteeFinalized
CiphernodeReg->>CiphernodeReg: store finalized committee
Enclave->>Enclave: on E3 completion
Enclave->>BondingReg: distributeRewards(nodes, amounts)
BondingReg->>TicketToken: transfer rewards
sequenceDiagram
actor Operator
participant BondingReg as BondingRegistry
participant TicketToken as EnclaveTicketToken
participant LicenseToken as IERC20 License
participant ExitQueue as ExitQueueLib
Operator->>BondingReg: registerOperator()
BondingReg->>BondingReg: check license bond met
BondingReg->>CiphernodeReg: add to registry
Operator->>BondingReg: bondLicense(amount)
BondingReg->>LicenseToken: transferFrom(operator, amount)
BondingReg->>BondingReg: update operator status
Operator->>BondingReg: deregisterOperator()
BondingReg->>ExitQueue: queueAssetsForExit(exitDelay)
ExitQueue->>ExitQueue: compute unlockTimestamp
rect Time-locked Exit
Note over ExitQueue: operator waits exitDelay seconds
end
Operator->>BondingReg: claimExits(maxTicket, maxLicense)
ExitQueue->>ExitQueue: check unlockTimestamp passed
BondingReg->>TicketToken: payout(operator, amount)
BondingReg->>LicenseToken: transfer(operator, amount)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
46-68: Critical validation logic flaw.The validation condition at lines 46-59 has a logic error. The current condition is:
if (!params || !owner || !maxDuration || !registry || !bondingRegistry || !usdcToken || (preDeployedArgs equality checks))This means if any required parameter is missing, the condition is
true, and the code proceeds to the pre-deployed contract reuse block. However, if the pre-deployed contract doesn't exist (line 60), it throws an error. If it does exist, it reuses it even though the caller didn't provide all required parameters.The correct logic should validate that all parameters are provided before attempting to reuse or deploy:
Apply this fix:
- if ( - !params || - !owner || - !maxDuration || - !registry || - !bondingRegistry || - !usdcToken || - (preDeployedArgs?.constructorArgs?.params === params && - preDeployedArgs?.constructorArgs?.owner === owner && - preDeployedArgs?.constructorArgs?.maxDuration === maxDuration && - preDeployedArgs?.constructorArgs?.registry === registry && - preDeployedArgs?.constructorArgs?.bondingRegistry === bondingRegistry && - preDeployedArgs?.constructorArgs?.usdcToken === usdcToken) - ) { + // Validate all required parameters are provided + if (!params || !owner || !maxDuration || !registry || !bondingRegistry || !usdcToken) { + throw new Error("All deployment parameters (params, owner, maxDuration, registry, bondingRegistry, usdcToken) are required"); + } + + // Check if we can reuse pre-deployed contract + if ( + preDeployedArgs?.constructorArgs?.params === params && + preDeployedArgs?.constructorArgs?.owner === owner && + preDeployedArgs?.constructorArgs?.maxDuration === maxDuration && + preDeployedArgs?.constructorArgs?.registry === registry && + preDeployedArgs?.constructorArgs?.bondingRegistry === bondingRegistry && + preDeployedArgs?.constructorArgs?.usdcToken === usdcToken + ) { if (!preDeployedArgs?.address) { throw new Error("Enclave address not found, it must be deployed first"); }packages/enclave-contracts/contracts/registry/NaiveRegistryFilter.sol (1)
90-104: Use calldata for external params and fix unused modifier
- Change parameters to
address[] calldata nodesandbytes calldata publicKeyinpublishCommitteeto avoid gas-costly memory copies (alchemy.com)- The
onlyOwnerOrCiphernodemodifier (defined at line 50) is never applied; replaceonlyOwnerwithonlyOwnerOrCiphernodeonpublishCommitteeif ciphernode actors should be permitted, or remove the unused modifier.
🧹 Nitpick comments (23)
packages/enclave-contracts/ignition/modules/mockSlashingVerifier.ts (1)
7-14: Confirm consistent pattern foras any. All Hardhat Ignition modules inpackages/enclave-contracts/ignition/modulesend with}) as any;, so this isn’t unique tomockSlashingVerifier.ts. No change required here—if you’d like to remove these assertions, consider an optional global refactor to define and apply proper return types for all modules..gitmodules (1)
6-6: Normalize indentation in .gitmodulesLine 6 has extra spaces before the tab; change it to a single tab for consistency:
- url = https://github.com/gnosisguild/risc0-ethereum + url = https://github.com/gnosisguild/risc0-ethereumGit already resolves this to
https://github.com/gnosisguild/risc0-ethereum. After merging, rungit submodule sync -- templates/default/lib/risc0-ethereumto update local config.packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (7)
6-6: Prefer stable HRE import pathImport from "hardhat/types" to avoid relying on internal subpaths that may change.
-import type { HardhatRuntimeEnvironment } from "hardhat/types/hre"; +import type { HardhatRuntimeEnvironment } from "hardhat/types";Based on learnings.
37-41: Checksum and reuse the validated owner addressNormalize
owneronce and use consistently to avoid mismatches with stored args. Also prevents accidental reuse when the same address is passed with different casing.const chain = (await signer.provider?.getNetwork())?.name ?? "localhost"; const preDeployedArgs = readDeploymentArgs("EnclaveToken", chain); - if (!owner || preDeployedArgs?.constructorArgs?.owner === owner) { + const ownerChecksummed = owner ? ethers.getAddress(owner) : undefined; + + if (!ownerChecksummed || preDeployedArgs?.constructorArgs?.owner === ownerChecksummed) { @@ - EnclaveToken: { - owner, - }, + EnclaveToken: { owner: ownerChecksummed }, @@ - constructorArgs: { - owner, - }, + constructorArgs: { owner: ownerChecksummed },Also applies to: 55-57, 69-71
42-45: Clarify error message for missing predeployMessage should hint at how to proceed (pass
ownerto deploy) to reduce friction.- throw new Error( - "EnclaveToken address not found, it must be deployed first", - ); + throw new Error( + "EnclaveToken not found in deployments. Pass a valid `owner` to deploy a new instance for this chain key." + );
61-66: Record the actual deployment block number from the receiptUsing the provider’s current block can drift if other blocks mine before you read. Prefer the deployment receipt’s blockNumber.
- await enclaveToken.enclaveToken.waitForDeployment(); - - const blockNumber = await ethers.provider.getBlockNumber(); - - const enclaveTokenAddress = await enclaveToken.enclaveToken.getAddress(); + const deployed = await enclaveToken.enclaveToken.waitForDeployment(); + const tx = deployed.deploymentTransaction?.(); + const receipt = tx ? await tx.wait() : undefined; + const blockNumber = receipt?.blockNumber ?? (await ethers.provider.getBlockNumber()); + const enclaveTokenAddress = await deployed.getAddress();
46-51: Optional: sanity‑check that the predeployed address has codeDefend against stale/corrupted deployments.json by verifying code exists at the address before returning the connected instance.
const enclaveTokenContract = EnclaveTokenFactory.connect( preDeployedArgs.address, signer, ); + const code = await signer.provider!.getCode(preDeployedArgs.address); + if (!code || code === "0x") { + throw new Error(`No contract code at ${preDeployedArgs.address} for EnclaveToken on chain ${chain}`); + } return { enclaveToken: enclaveTokenContract };
79-85: Avoid redundant reconnect; you already have a connected instanceYou can return the
enclaveToken.enclaveTokeninstance directly after deployment. Re‑connecting is harmless but unnecessary.- const enclaveTokenContract = EnclaveTokenFactory.connect( - enclaveTokenAddress, - signer, - ); - - return { enclaveToken: enclaveTokenContract }; + return { enclaveToken: enclaveToken.enclaveToken };
23-27: Docstring: reflect “get or deploy” behaviorThe function reuses an existing deployment when
owneris omitted or matches stored args. Update the summary to reduce confusion.- * Deploys the EnclaveToken contract and saves the deployment arguments + * Connects to an existing EnclaveToken (if args match) or deploys a new one via Ignition, then saves deployment argspackages/enclave-contracts/ignition/modules/enclaveToken.ts (1)
7-7: Consider narrowing the linting exception.The
@typescript-eslint/no-explicit-anyrule is disabled for the entire file, but the only usage ofanyis the type assertion on line 16 (as any). Disabling at the file level reduces type safety for any future additions to this module.Consider using an inline disable comment instead:
-/* eslint-disable @typescript-eslint/no-explicit-any */ import { buildModule } from "@nomicfoundation/hardhat-ignition/modules"; export default buildModule("EnclaveToken", (m) => { const owner = m.getParameter("owner"); const enclaveToken = m.contract("EnclaveToken", [owner]); return { enclaveToken }; -}) as any; +// eslint-disable-next-line @typescript-eslint/no-explicit-any +}) as any;This keeps type checking enabled for any future code additions to this file.
packages/enclave-contracts/ignition/modules/mockStableToken.ts (2)
7-7: Document why the lint rule is disabled.The
@typescript-eslint/no-explicit-anyrule is disabled for the entire file. Consider adding a comment explaining why this is necessary (likely due to Ignition module typing constraints).
16-16: Avoidas anycast if possible.The
as anycast weakens type safety. If this is required due to Ignition module typing limitations, consider adding a comment explaining why. Otherwise, investigate if a more specific type can be used.Apply this approach:
-}) as any; +}); // Note: Cast required due to Ignition module type inference limitationsOr better yet, if the Ignition types support it, use a proper type annotation.
packages/enclave-contracts/contracts/test/MockSlashingVerifier.sol (1)
11-18: Simplify unused parameter handling.Line 15 (
data;) appears to be an attempt to mark the parameter as used. In Solidity, you can omit the parameter name for unused parameters or use a comment to suppress warnings.Apply this diff to improve clarity:
function verify( - uint256, + uint256 /* proposalId */, bytes memory data ) external pure returns (bool success) { - data; - - if (data.length > 0) success = true; + return data.length > 0; }This is cleaner and more idiomatic Solidity.
packages/enclave-contracts/ignition/modules/slashingManager.ts (1)
7-7: Type safety bypassed with 'any' cast.The
anycast on line 20 and the eslint-disable on line 7 eliminate type safety for this module. While this may be necessary for Ignition module compatibility, consider whether stricter typing is possible or document why the escape hatch is required.Also applies to: 20-20
packages/enclave-contracts/ignition/modules/bondingRegistry.ts (1)
7-7: Type safety bypassed with 'any' cast.The module uses
anytyping, eliminating compile-time checks. Document why this is necessary or explore stricter typing options.Also applies to: 34-34
packages/enclave-contracts/ignition/modules/enclaveTicketToken.ts (1)
7-7: Consider removing theanyescape hatch if TypeScript types can be refined.The
eslint-disablecomment and theas anycast on line 22 suggest the ignition module type definitions may be incomplete. If the hardhat-ignition types have been updated to properly typebuildModulereturn values, these workarounds could be removed.packages/enclave-contracts/contracts/registry/NaiveRegistryFilter.sol (1)
50-57: Consider gas optimization for repeated external calls.The modifier calls
ICiphernodeRegistry(registry).isCiphernodeEligible(msg.sender)on every invocation, which incurs an external call. If this modifier is used frequently, consider caching eligibility status or using events to maintain an internal mapping.packages/enclave-contracts/contracts/lib/ExitQueueLib.sol (2)
88-94: Tranche creation pattern works but could be more explicit.The code pushes an empty tranche (line 89), then immediately accesses and populates it (lines 90-93). While this works correctly, it's slightly unconventional. Consider using
operatorQueue.push(ExitTranche({...}))for clarity.Apply this diff for a more explicit pattern:
- if (!merged) { - operatorQueue.push(); - ExitTranche storage t = operatorQueue[len]; - t.unlockTimestamp = unlockTimestamp; - t.ticketAmount = ticketAmount; - t.licenseAmount = licenseAmount; - } + if (!merged) { + operatorQueue.push(ExitTranche({ + unlockTimestamp: unlockTimestamp, + ticketAmount: ticketAmount, + licenseAmount: licenseAmount + })); + }
138-155: Preview function has unbounded loop.The
previewClaimableAmountsfunction iterates through all tranches from the head index. If an operator has accumulated many tranches, this could consume significant gas. Since this is a view function, it won't fail transactions but could time out for off-chain callers.Consider documenting this limitation or providing a paginated version for operators with many tranches.
packages/enclave-contracts/contracts/Enclave.sol (2)
154-155: Fee calculation is placeholder.The
getE3Quotefunction (line 511-515) returns a hardcoded fee of 1 USDC. Consider implementing dynamic fee calculation based on computation requirements (duration, threshold, etc.).
370-373: Reward distribution truncates remainder.When dividing the payment equally (line 370), any remainder is lost due to integer division. The comment at line 381 acknowledges this "dust" but doesn't specify where it goes.
Consider implementing a dust collection mechanism (treasury address) or distributing the remainder to the first node to avoid permanent loss of funds.
packages/enclave-contracts/contracts/slashing/SlashingManager.sol (1)
49-71: Consider removing unusedonlyVerifiermodifier.The
onlyVerifiermodifier (lines 58-61) is defined but doesn't appear to be used in this contract. If it's not part of a required interface or planned for future use, consider removing it.Run the following script to confirm usage:
#!/bin/bash # Description: Check if onlyVerifier modifier is used # Search for onlyVerifier usage in function declarations rg -n 'function.*onlyVerifier' --type=solpackages/enclave-contracts/contracts/token/EnclaveToken.sol (1)
116-143: Consider consolidating validations in the first loop.The function is correct, but you could optimize gas by checking for zero addresses and amounts during the first loop (lines 127-129) rather than during minting (lines 134-137). This avoids processing the entire batch if any item is invalid.
Apply this diff to consolidate validations:
uint256 totalAmount = 0; for (uint256 i = 0; i < amounts.length; i++) { + if (recipients[i] == address(0)) revert ZeroAddress(); + if (amounts[i] == 0) revert ZeroAmount(); totalAmount += amounts[i]; } if (totalMinted + totalAmount > TOTAL_SUPPLY) revert ExceedsTotalSupply(); for (uint256 i = 0; i < recipients.length; i++) { address rec = recipients[i]; uint256 amt = amounts[i]; - if (rec == address(0)) revert ZeroAddress(); - if (amt == 0) revert ZeroAmount(); _mint(rec, amt); emit AllocationMinted(rec, amt, allocations[i]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
.gitmodules(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json(6 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json(4 hunks)packages/enclave-contracts/artifacts/contracts/registry/NaiveRegistryFilter.sol/NaiveRegistryFilter.json(2 hunks)packages/enclave-contracts/contracts/Enclave.sol(12 hunks)packages/enclave-contracts/contracts/interfaces/IBondingRegistry.sol(1 hunks)packages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.sol(4 hunks)packages/enclave-contracts/contracts/interfaces/IEnclave.sol(3 hunks)packages/enclave-contracts/contracts/interfaces/IRegistryFilter.sol(1 hunks)packages/enclave-contracts/contracts/interfaces/ISlashVerifier.sol(1 hunks)packages/enclave-contracts/contracts/interfaces/ISlashingManager.sol(1 hunks)packages/enclave-contracts/contracts/lib/ExitQueueLib.sol(1 hunks)packages/enclave-contracts/contracts/registry/BondingRegistry.sol(1 hunks)packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol(10 hunks)packages/enclave-contracts/contracts/registry/NaiveRegistryFilter.sol(4 hunks)packages/enclave-contracts/contracts/slashing/SlashingManager.sol(1 hunks)packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol(4 hunks)packages/enclave-contracts/contracts/test/MockRegistryFilter.sol(4 hunks)packages/enclave-contracts/contracts/test/MockSlashingVerifier.sol(1 hunks)packages/enclave-contracts/contracts/test/MockStableToken.sol(1 hunks)packages/enclave-contracts/contracts/token/EnclaveTicketToken.sol(1 hunks)packages/enclave-contracts/contracts/token/EnclaveToken.sol(1 hunks)packages/enclave-contracts/ignition/modules/bondingRegistry.ts(1 hunks)packages/enclave-contracts/ignition/modules/enclave.ts(1 hunks)packages/enclave-contracts/ignition/modules/enclaveTicketToken.ts(1 hunks)packages/enclave-contracts/ignition/modules/enclaveToken.ts(1 hunks)packages/enclave-contracts/ignition/modules/mockSlashingVerifier.ts(1 hunks)packages/enclave-contracts/ignition/modules/mockStableToken.ts(1 hunks)packages/enclave-contracts/ignition/modules/slashingManager.ts(1 hunks)packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts(1 hunks)packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts(1 hunks)packages/enclave-contracts/scripts/deployAndSave/enclave.ts(5 hunks)packages/enclave-contracts/scripts/deployAndSave/enclaveTicketToken.ts(1 hunks)packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts(1 hunks)packages/enclave-contracts/scripts/deployAndSave/mockStableToken.ts(1 hunks)packages/enclave-contracts/scripts/deployAndSave/slashingManager.ts(1 hunks)packages/enclave-contracts/scripts/deployEnclave.ts(3 hunks)packages/enclave-contracts/scripts/index.ts(1 hunks)packages/enclave-contracts/test/BondingRegistry.spec.ts(1 hunks)packages/enclave-contracts/test/CiphernodeRegistry/CiphernodeRegistryOwnable.spec.ts(3 hunks)packages/enclave-contracts/test/CiphernodeRegistry/NaiveRegistryFilter.spec.ts(1 hunks)packages/enclave-contracts/test/Enclave.spec.ts(24 hunks)packages/enclave-contracts/test/SlashingManager.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
packages/enclave-contracts/scripts/deployAndSave/mockStableToken.ts (1)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(74-88)storeDeploymentArgs(34-66)
packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(74-88)storeDeploymentArgs(34-66)
packages/enclave-contracts/scripts/deployAndSave/slashingManager.ts (1)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(74-88)storeDeploymentArgs(34-66)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(74-88)storeDeploymentArgs(34-66)
packages/enclave-contracts/scripts/deployAndSave/enclaveTicketToken.ts (1)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(74-88)storeDeploymentArgs(34-66)
packages/enclave-contracts/scripts/deployEnclave.ts (8)
packages/enclave-contracts/scripts/deployAndSave/mockStableToken.ts (1)
deployAndSaveMockStableToken(25-84)packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
deployAndSaveEnclaveToken(28-85)packages/enclave-contracts/scripts/deployAndSave/enclaveTicketToken.ts (1)
deployAndSaveEnclaveTicketToken(30-101)packages/enclave-contracts/scripts/deployAndSave/slashingManager.ts (1)
deployAndSaveSlashingManager(29-95)packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
deployAndSaveBondingRegistry(36-140)packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts (1)
deployAndSaveCiphernodeRegistryOwnable(29-92)packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
deployAndSaveEnclave(30-108)packages/enclave-contracts/scripts/deployAndSave/naiveRegistryFilter.ts (1)
deployAndSaveNaiveRegistryFilter(21-87)
🪛 GitHub Actions: License Header Check
packages/enclave-contracts/test/SlashingManager.spec.ts
[error] 1-1: Missing license header.
🪛 Gitleaks (8.28.0)
packages/enclave-contracts/test/Enclave.spec.ts
[high] 745-745: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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_sdk
- GitHub Check: test_contracts
- GitHub Check: integration_prebuild
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: build_e3_support_risc0
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
🔇 Additional comments (141)
packages/enclave-contracts/ignition/modules/mockSlashingVerifier.ts (1)
10-13: LGTM! Standard ignition module structure.The module correctly wires the MockSlashingVerifier contract deployment using Hardhat Ignition's buildModule API. The structure is clean and follows the declarative pattern expected for test mock deployments.
packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
53-59: Parameters shape verified TheEnclaveTokenmodule’s ID and itsownerparameter match the deployment call, so theparametersobject is correct.packages/enclave-contracts/contracts/test/MockStableToken.sol (3)
11-19: LGTM!The contract structure and initialization are correct. The constructor properly scales the initial supply by 10^6 to match USDC's 6 decimal places, and ownership is correctly assigned to the deployer.
21-23: LGTM!The decimals override correctly returns 6, matching the standard USDC token specification.
25-27: LGTM!The owner-restricted mint function is appropriate for a test token, allowing flexible token provisioning in test scenarios.
packages/enclave-contracts/contracts/interfaces/IEnclave.sol (3)
73-80: LGTM!The new events follow the existing naming and documentation patterns established in the interface. They correctly emit when bonding registry and USDC token addresses are configured.
136-136: Good gas optimization.Changing the parameter location from
memorytocalldatafor this external function reduces gas costs since the parameter is read-only and doesn't need to be copied to memory.
232-238: LGTM!The new
getE3Quotefunction provides a valuable query mechanism for clients to estimate fees before submitting an E3 request. The view function signature and calldata parameter are appropriate, and the documentation correctly indicates validation behavior.packages/enclave-contracts/scripts/deployAndSave/enclave.ts (3)
20-22: LGTM!The new optional parameters follow the existing pattern in the
EnclaveArgsinterface and correctly represent address strings for the bonding registry and USDC token.
35-37: LGTM!Function parameters correctly extended to include the new bonding registry and USDC token addresses.
70-108: LGTM after validation fix.The deployment and storage logic correctly includes the new
bondingRegistryandusdcTokenparameters. Once the validation logic issue (flagged above) is fixed, this segment will function correctly.packages/enclave-contracts/contracts/interfaces/ISlashingManager.sol (1)
1-286: LGTM! Well-structured slashing governance interface.The interface is comprehensive and well-documented. The separation of concerns (policies, proposals, appeals, bans) is clear, and the event/error definitions provide good observability. The two-phase slash workflow (propose → appeal window → execute) with proof verification support demonstrates thoughtful design.
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
56-78: Verify the conditional logic for cache reuse.The condition checks if any required parameter is missing OR if all parameters match the cached deployment. However, the logic appears inverted: if parameters are missing, the function should not proceed to reuse a cached deployment without validating completeness.
The current structure suggests: "reuse cached deployment if (any param missing) OR (all params match)". This could lead to returning a cached contract when required parameters are undefined.
Review the conditional flow:
- Lines 56-65: Check if any required param is undefined
- Lines 66-77: Check if all params match cached args
- Line 78: If either condition is true, attempt to reuse cached deployment
- Lines 79-83: Throw error if cached address is missing
This logic seems to conflate two distinct scenarios:
- Missing parameters during invocation (should error or prompt for values)
- All parameters match cached deployment (safe to reuse)
Consider refactoring to separate these cases:
+ // First, validate all required parameters are provided + if ( + !owner || + !ticketToken || + !licenseToken || + !registry || + !slashedFundsTreasury || + !ticketPrice || + !licenseRequiredBond || + minTicketBalance === undefined || + exitDelay === undefined + ) { + throw new Error("All BondingRegistry parameters are required"); + } + + // Then, check if cached deployment matches current parameters if ( - !owner || - !ticketToken || - !licenseToken || - !registry || - !slashedFundsTreasury || - !ticketPrice || - !licenseRequiredBond || - minTicketBalance === undefined || - exitDelay === undefined || - (preDeployedArgs?.constructorArgs?.owner === owner && + preDeployedArgs?.constructorArgs?.owner === owner && preDeployedArgs?.constructorArgs?.ticketToken === ticketToken &&This makes the intent clearer: require all parameters first, then reuse cached deployment only if they match.
packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts (1)
86-89: LGTM! Adds signer to contract connection.The change ensures the BondingRegistry contract instance returned from new deployments is properly connected with a signer, matching the pattern already used in the cached deployment path (line 55). This enables the contract to send transactions.
packages/enclave-contracts/ignition/modules/enclaveToken.ts (1)
10-16: LGTM! Standard ignition module pattern.The module correctly retrieves the owner parameter and deploys the EnclaveToken contract with it. The structure follows Hardhat Ignition conventions.
packages/enclave-contracts/test/CiphernodeRegistry/CiphernodeRegistryOwnable.spec.ts (3)
240-242: LGTM! Test updated for new access control.The error expectation correctly reflects the updated access control in
CiphernodeRegistryOwnable, whereaddCiphernodenow allows both the owner and the bonding registry to add nodes.
247-247: LGTM! Test updated for renamed method.The test correctly uses the new
isEnabled()method name, which replaces the previousisCiphernodeEligible()API.
276-277: LGTM! Test updated for new access control.The error expectation correctly reflects the updated access control for
removeCiphernode, consistent with the change inaddCiphernode.packages/enclave-contracts/contracts/interfaces/ISlashVerifier.sol (1)
8-18: LGTM! Clean interface design.The interface is well-documented and provides a clear contract for slash proof verification. The function signature appropriately uses
external viewfor read-only verification, and the parameter names are descriptive.packages/enclave-contracts/ignition/modules/mockStableToken.ts (1)
10-15: LGTM! Module structure follows Ignition conventions.The module correctly retrieves the
initialSupplyparameter and deploys the MockUSDC contract with it.packages/enclave-contracts/contracts/interfaces/IRegistryFilter.sol (2)
20-22: LGTM! Clean accessor function.The
getCommitteefunction provides a straightforward way to retrieve committee data by E3 ID.
9-13: ConfirmpublicKeystores the keccak256 hash;bytes32is sufficient.packages/enclave-contracts/test/CiphernodeRegistry/NaiveRegistryFilter.spec.ts (1)
106-122: LGTM! Test properly validates registry-mediated committee flow.The refactored test now correctly exercises the registry-mediated committee request path, which better reflects the actual usage pattern. The test:
- Wires the filter to the registry
- Requests the committee through the registry (not directly on the filter)
- Publishes the committee with nodes and public key
- Validates the threshold was set correctly
This is a more realistic integration test.
packages/enclave-contracts/ignition/modules/bondingRegistry.ts (1)
21-31: Verify BondingRegistry constructor parameter orderI was unable to locate the
BondingRegistryconstructor in the codebase—please manually confirm that the instantiation arguments
[owner, ticketToken, licenseToken, registry, slashedFundsTreasury, ticketPrice, licenseRequiredBond, minTicketBalance, exitDelay]
match theBondingRegistry.solconstructor signature.packages/enclave-contracts/ignition/modules/enclave.ts (1)
15-16: Constructor parameter order matches Enclave.sol signature.No changes needed.
packages/enclave-contracts/ignition/modules/slashingManager.ts (1)
14-17: Constructor parameter order matches contract signature. The deployment args[admin, bondingRegistry]align withSlashingManager.sol’sconstructor(address admin, address _bondingRegistry).packages/enclave-contracts/ignition/modules/enclaveTicketToken.ts (1)
10-22: LGTM!The ignition module correctly retrieves the three required constructor parameters and passes them to the EnclaveTicketToken contract. The pattern is consistent with other ignition modules in the codebase.
packages/enclave-contracts/test/Enclave.spec.ts (5)
66-78: LGTM! Helper improves test readability.The
makeRequesthelper centralizes the USDC approval + request flow, reducing duplication across tests. The optionalsignerparameter enables testing with different accounts.
276-290: LGTM! Token minting provides sufficient test balances.The setup mints USDC and EnclaveToken allocations for both owner and notTheOwner accounts, enabling comprehensive testing of fee payments and bonding flows.
42-45: False positive: Not an API key.The static analysis tool flagged
encryptionSchemeIdas a potential API key. This is a test fixture constant representing a keccak256 hash used as an encryption scheme identifier, not a secret credential.
733-746: Confirm ERC20InsufficientAllowance is defined in imported ERC20
I couldn’t locate anyerror ERC20InsufficientAllowancein MockUSDC or test contracts—this must come from the OpenZeppelin ERC20 you import. Please verify your ERC20 dependency actually declares that custom error (otherwise update the test to use the correct revert string, e.g."ERC20: insufficient allowance").
85-233: No circular‐dependency issues—constructors use non-zero placeholders
Each contract’s constructor requires a non-zero address but is passed the dummy addressOne placeholder (not zero) at deploy, so no logic ever operates on an uninitialized registry or slashing manager before the subsequent setters configure the real addresses.packages/enclave-contracts/contracts/test/MockRegistryFilter.sol (3)
30-31: LGTM! Type migration to external interface.The storage mapping correctly adopts
IRegistryFilter.Committeefrom the external interface, ensuring consistency across the codebase.
116-122: LGTM! New getter enforces publication requirement.The
getCommitteefunction correctly returns the committee and enforces that it has been published (publicKey is non-zero). This aligns with the interface requirements.
93-98: publicKey stored as hash correctly; full key passed through for activation
IRegistryFilter.Committee.publicKey is defined as bytes32 and both NaiveRegistryFilter and MockRegistryFilter consistently use keccak256(publicKey) for storage. The full public key is still forwarded via publishCommittee (and emitted by CiphernodeRegistryOwnable) for Enclave.activate to verify and use.packages/enclave-contracts/scripts/deployEnclave.ts (7)
57-63: LGTM!EnclaveToken deployment is correctly configured with the owner address and proper logging.
65-73: LGTM!EnclaveTicketToken deployment correctly uses
addressOneas a placeholder for the registry address, which is properly configured later in the cross-contract dependencies section (line 144).
75-82: LGTM!SlashingManager deployment correctly uses
addressOneas a placeholder for the bonding registry address, which is properly configured later (line 150).
84-98: Verify the slashed funds treasury address.The
slashedFundsTreasuryis currently set to the owner address. Ensure this is the intended destination for slashed funds in production deployments, as a dedicated treasury or multisig wallet may be more appropriate.
109-120: LGTM!Enclave deployment is correctly configured with all required parameters, including the registry addresses and USDC token.
131-156: LGTM!Cross-contract dependency configuration is comprehensive and correctly resolves all placeholder addresses used during deployment. The wiring sequence properly connects all contracts in the system.
187-200: LGTM!Deployment summary provides comprehensive logging of all deployed contract addresses with clear formatting.
packages/enclave-contracts/test/SlashingManager.spec.ts (9)
1-23: LGTM on imports and test setup.The imports and dynamic network connection pattern align with the Hardhat v3 and Ignition workflow. The destructuring of helpers from
networkHelpersis clear and appropriate.
75-204: Fixture setup is well-structured.The setup function properly deploys all contracts, assigns roles, and wires dependencies. The two-step registry setup pattern (deploy with placeholder, then call
setRegistry) at lines 103, 135, and 171-177 is acceptable for test fixtures, though it requires care to ensure all connections are established before tests run.
246-420: Policy validation tests are comprehensive.The tests thoroughly cover policy validation rules, including the mutual exclusivity of proof requirements and appeal windows (lines 385-401 and 403-419). This ensures that governance cannot configure conflicting policy parameters.
482-672: Slash proposal tests are thorough.The tests cover both proof-based (immediate execution) and evidence-based (appeal window) proposals. Event emission verification includes proper timestamp calculations (lines 498-515, 539-557), and the
totalProposalscounter is verified to increment correctly.
674-807: Execution tests cover critical paths.The tests properly verify immediate execution for proof-based slashes, delayed execution after appeal windows (lines 712-736), and automatic node banning (lines 738-760). Error conditions are appropriately tested.
809-1070: Appeal system tests are comprehensive.The appeal lifecycle is thoroughly tested, including filing, resolution (approve/deny), and the interaction with execution. The tests correctly verify that pending appeals block execution (lines 985-1010), approved appeals permanently block execution (lines 1012-1039), and denied appeals allow execution to proceed (lines 1041-1069).
1072-1135: Ban management tests are appropriate.The tests verify governance control over banning/unbanning and correctly test that new slash proposals are rejected for banned nodes (lines 1115-1134). This prevents redundant slashing of already-sanctioned operators.
1137-1193: View function tests validate data retrieval.The tests appropriately verify that policies and proposals can be retrieved with accurate field values, and that invalid proposal IDs are rejected (lines 1185-1191).
463-471: Assertion syntax is correct The.to.be.revert(ethers)matcher aligns with Hardhat v3’s updated syntax for asserting reverts (hardhat.org)Likely an incorrect or invalid review comment.
packages/enclave-contracts/scripts/deployAndSave/enclaveTicketToken.ts (3)
1-13: LGTM on license header and imports.The file includes the required license header and imports all necessary dependencies for the deployment script.
15-23: Interface design supports reusable deployments.The optional parameters (
underlyingUSDC,registry,owner) enable the deployment function to reuse pre-deployed contracts, which is the intended pattern shown in the function implementation.
64-101: Deployment and storage logic is sound.The deployment flow properly waits for contract deployment, captures the block number and address, and stores the deployment arguments for future reuse. The factory connection pattern at lines 95-98 is correct.
packages/enclave-contracts/scripts/index.ts (1)
10-17: LGTM on new deployment script exports.The new exports at lines 10, 13-15, and 17 follow the established pattern and provide centralized access to the deployment utilities introduced in this PR.
packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json (1)
19-31: Artifact additions align with new economic features.The new
BondingRegistrySetandUsdcTokenSetevents, along with thegetE3Quoteview function, correctly reflect the interface expansions for the USDC-based payment subsystem. TheE3RequestParamsstruct properly encapsulates the parameters needed for quote calculation.Also applies to: 307-319, 473-528
packages/enclave-contracts/scripts/deployAndSave/mockStableToken.ts (3)
37-50: Deployment reuse logic is correct.The condition properly handles reusing an existing deployment when
initialSupplyis unchanged. The type conversion viatoString()correctly compares the numeric parameter with the stored string value from previous deployments.
52-76: Deployment flow follows best practices.The ignition-based deployment correctly waits for confirmation, captures the block number for history tracking, and persists deployment arguments for future reuse. Storing
initialSupplyas a string maintains consistency with the reuse logic.
78-83: Contract connection and return are correct.The factory pattern correctly connects to the deployed contract address with the signer, enabling transaction signing. The return type matches the function signature.
packages/enclave-contracts/test/BondingRegistry.spec.ts (4)
43-200: Comprehensive test fixture with proper contract wiring.The setup fixture correctly deploys and integrates all components of the bonding/slashing subsystem. Note that
AddressOneis used as a placeholder forticketToken.registry(line 86) andslashingManager.bondingRegistry(line 99), which are correctly set to the actualBondingRegistryaddress after deployment (lines 151-156). This pattern is appropriate for circular dependencies in deployment.
202-226: Constructor tests validate all initial parameters.The test correctly verifies that all constructor parameters are properly set, including the default
licenseActiveBpsvalue of 8000 (80%).
228-371: Bonding and unbonding tests are comprehensive.Tests correctly cover the full lifecycle of license bonding: successful bonding with event emission, zero amount validation, exit-in-progress restrictions, balance accumulation, unbonding flows, and exit queue management. All assertions verify both state changes and event emissions.
1018-1158: Edge case tests validate complex state transitions.The test suite thoroughly exercises complex scenarios: operator deactivation through license reduction (testing the boundary condition with
unbondAmount + 1n), concurrent multi-operator states, and complete lifecycle flows including exit, claim, and re-registration. These tests provide strong confidence in the system's handling of real-world usage patterns.packages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.sol (7)
8-8: LGTM - Import enables shared Committee type.The import of
IRegistryFilteris necessary to reference theIRegistryFilter.Committeetype used in thegetCommitteereturn value, establishing proper type coupling between the registry and filter interfaces.
26-29: LGTM - Event properly documents committee lifecycle.The
CommitteeActivationChangedevent provides clear signaling for committee status transitions (active/completed), which is essential for off-chain monitoring and state tracking.
61-64: LGTM - isEnabled function signature is clean.The
isEnabledfunction provides a straightforward view function to check ciphernode status without side effects.
66-68: LGTM - addCiphernode function signature is appropriate.The function signature is clean and follows Solidity conventions for registry operations.
109-113: LGTM - getFilter function is straightforward.The function provides clean access to the filter address for a given E3 ID with appropriate revert behavior documented.
115-121: LGTM - getCommittee properly uses shared Committee type.The function correctly returns
IRegistryFilter.Committeetype, establishing a consistent data structure across the registry and filter components. The documentation clearly specifies revert conditions.
70-76: ValidatesiblingNodeslength in Merkle removal
We couldn’t locate theLeanIMTData._removeimplementation in the repo—confirm that the Merkle-proof array is validated (correct length and order) in the tree‐removal logic or document and enforce its expected size.packages/enclave-contracts/contracts/registry/NaiveRegistryFilter.sol (3)
23-24: LGTM - Storage properly uses shared Committee type.The mapping correctly uses
IRegistryFilter.Committeeas the value type, aligning with the interface standardization across the codebase.
95-95: LGTM - Correct storage reference for Committee.The storage reference properly uses
IRegistryFilter.Committeetype, ensuring type consistency with the mapping declaration.
122-128: LGTM - getCommittee correctly returns shared Committee type.The function properly returns
IRegistryFilter.Committee memorywith appropriate validation (checking for non-zero public key), aligning with the interface contract.packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol (6)
9-9: LGTM - Import required for Committee type.The import is necessary to construct and return
IRegistryFilter.Committeeinstances in the mock implementations.
24-30: LGTM - Mock stubs are appropriate.The empty implementations for
addCiphernodeandremoveCiphernode, along withisEnabledalways returningtrue, are reasonable defaults for test mocks where the specific behavior isn't under test.
51-53: LGTM - getFilter returns appropriate default.Returning
address(0)is a reasonable default for a mock when the specific filter address isn't relevant to the test scenario.
55-64: LGTM - getCommittee returns valid empty Committee.The mock returns a properly constructed
IRegistryFilter.Committeewith empty values (empty nodes array, zero threshold, zero public key), which is appropriate for tests that don't specifically validate committee data.
80-87: LGTM - Consistent mock implementation in second contract.The
MockCiphernodeRegistryEmptyKeycontract correctly duplicates the stub implementations foraddCiphernode,isEnabled, andremoveCiphernode, maintaining consistency across mock variants.
95-108: LGTM - Consistent getFilter and getCommittee implementations.Both mocks now implement
getFilterandgetCommitteeconsistently, returning appropriate defaults. The distinction between the two mocks (standard vs. empty key) is preserved in their respectivecommitteePublicKeyimplementations.packages/enclave-contracts/contracts/lib/ExitQueueLib.sol (5)
10-14: Timestamp range is adequate for use case.Using
uint64forunlockTimestampsupports dates until year 2554, which is sufficient for this use case. The overflow check at line 68-71 ensures safety.
112-136: LGTM!Helper functions correctly delegate to the main queueing function and provide clean read access to pending amounts.
157-193: Claim logic is well-structured.The function correctly:
- Processes tickets and licenses independently
- Only updates pending totals when assets are actually claimed
- Cleans up empty tranches once
- Emits events only when state changes
195-237: Slash logic mirrors claim logic correctly.The
slashPendingAssetsfunction follows the same pattern asclaimAssetsbut adds theincludeLockedAssetsflag. The structure is consistent and correct.
282-339: Asset taking logic is correct and handles partial claims.The
_takeAssetsFromQueuefunction correctly:
- Iterates through tranches from the head index
- Respects unlock timestamps unless
includeLockedAssetsis set- Handles partial consumption of tranches (one asset type may be depleted while the other remains)
- Only advances past tranches when both asset types are fully consumed
The interaction between line 315-318 (skipping empty asset types) and line 333-335 (advancing only when both are zero) is correct.
packages/enclave-contracts/contracts/Enclave.sol (4)
11-12: LGTM - Proper imports for token integration.The addition of
IBondingRegistry,IRegistryFilter,SafeERC20, andIERC20imports correctly supports the new payment and reward distribution features.Also applies to: 17-20, 29-29
38-39: Storage variables added appropriately.The new storage variables (
bondingRegistry,usdcToken,e3Payments) are properly placed and support the new economic model.Also applies to: 63-63
106-143: Initialization properly extended for new dependencies.The constructor and
initializefunction correctly accept and set the newbondingRegistryandusdcTokenparameters.
221-232: Incorrect assumption: payment isn’t lost on failure
Reverting viarequireundoes all prior state changes in the transaction—including the ERC-20 transfer—so there is no need for an explicit refund path here.Likely an incorrect or invalid review comment.
packages/enclave-contracts/contracts/token/EnclaveTicketToken.sol (8)
26-32: Inheritance order is correct.The contract properly inherits ERC20, ERC20Permit, ERC20Votes, Ownable, and ERC20Wrapper. This order ensures correct method resolution order for OpenZeppelin's diamond pattern.
40-43: Access control properly implemented.The
onlyRegistrymodifier and validation in constructor/setter ensure only authorized addresses can perform critical operations.Also applies to: 55-56, 60-60
70-80: Auto-delegation on deposit is well-designed.The
depositForfunction correctly auto-delegates voting power to the operator on first deposit. This ensures voting power is tracked without requiring separate delegation transactions.
89-98:depositFromprovides flexible deposit flow.This function allows depositing from one address while minting to another, useful for registry-managed operations. Auto-delegation is correctly applied to the recipient.
138-147: Transfer restrictions correctly implemented.The
_updateoverride properly prevents transfers between users, allowing only minting and burning. This makes the token non-transferable as required for staking tickets.
153-155: Delegation correctly locked to preserve voting power.Overriding
delegateanddelegateBySigto revert ensures that voting power cannot be transferred separately from the ticket tokens. Combined with auto-delegation on deposit, this maintains the invariant that tickets = voting power.Also applies to: 161-170
172-185: Multiple inheritance overrides properly specified.The
decimalsandnoncesfunctions correctly specify all parent contracts being overridden and delegate tosuper, allowing OpenZeppelin's method resolution order to work correctly.
131-133: Clarifypayoutfunction intent.
payouttransfers underlying USDC without burning ETK (breaking the 1:1 backing). SincewithdrawToalready handles burning + transfer, document or renamepayout, restrict it to reward distributions funded from a separate pool, or remove it if unused.packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (13)
22-28: LGTM!Event declaration is well-structured with indexed parameter for efficient off-chain filtering.
37-37: LGTM!Public storage variable placement and visibility are appropriate.
57-62: LGTM!Custom error declarations are specific and provide clear failure reasons for the new bonding integration paths.
75-86: LGTM!Access control modifiers are correctly implemented with appropriate authorization checks and custom error reverts.
98-105: LGTM!Zero-address validation guards prevent initialization with invalid addresses, protecting against deployment errors.
142-156: LGTM!Access control expansion and short-circuit logic for duplicate adds are correct. The early return prevents unnecessary state mutations and events.
158-169: LGTM!Access control changes and the enabled-state guard correctly prevent removal of non-existent nodes.
177-181: LGTM!Zero-address validation is consistent with initialization guards and prevents misconfiguration.
183-187: LGTM!Admin setter is properly guarded with owner-only access and zero-address validation. Event emission provides transparency.
221-223: LGTM!Visibility change to
publicallows both internal and external access. Return type simplification toaddressis a minor ABI refinement.
225-229: LGTM!Delegation to the filter contract is correct. The function will revert with a clear error if the filter is not set, which is acceptable behavior.
235-237: LGTM!Standard getter implementation for external access to the bonding registry address.
202-207: Initialization flow validated — guard is safe.Deployment scripts (
deployEnclave.ts) always callsetBondingRegistrybefore any calls toisCiphernodeEligible, and tests forisCiphernodeEligibleonly exercise the disabled‐node path (never hitting therequire), so the newrequirecannot revert in practice.packages/enclave-contracts/contracts/interfaces/IBondingRegistry.sol (12)
1-7: LGTM!License and pragma declarations are standard and consistent with the codebase.
9-11: LGTM!Import statements are appropriate for the interface's type dependencies.
18-37: LGTM!Custom error definitions provide comprehensive coverage of failure modes with clear, descriptive names.
43-55: LGTM!Event design with signed delta and indexed reason provides excellent audit trail capabilities. NatSpec clearly documents the semantics.
57-69: LGTM!Event structure is consistent with
TicketBalanceUpdated, maintaining a parallel design pattern.
71-110: LGTM!Remaining events are well-documented with appropriate indexed parameters for efficient filtering and lifecycle tracking.
112-205: LGTM!View function interface is comprehensive and well-organized, with clear NatSpec documentation for each query method.
207-250: LGTM!Operator lifecycle functions are well-designed with clear preconditions documented. The
deregisterOperatorsignature requiring sibling nodes enables immediate IMT removal.
252-264: LGTM!The
claimExitsfunction design with separate max parameters provides flexibility for partial claims.
266-294: LGTM!Slashing functions are well-separated by balance type, use absolute amounts for precision, and include reason parameters for audit trails.
296-310: LGTM!Batch reward distribution design is gas-efficient and flexible with the generic
IERC20reward token parameter.
312-395: LGTM!Admin interface is comprehensive with clear separation between configuration parameters and component addresses. All functions are appropriately documented as owner-only.
packages/enclave-contracts/contracts/slashing/SlashingManager.sol (10)
1-14: LGTM!License, pragma, and imports are appropriate for the contract's functionality.
16-47: LGTM!Role constants and storage layout are well-organized with appropriate visibility modifiers. Internal
_proposalsmapping ensures controlled access via getters.
73-85: LGTM!Constructor implements proper input validation and secure-by-default role initialization. Only admin and governance roles are granted initially.
87-106: LGTM!View functions include appropriate bounds checking and return memory copies for safety.
112-132: LGTM!Policy validation correctly enforces mutually exclusive proof-based and evidence-based modes. The logic ensures proper configuration for each slashing approach.
134-163: LGTM!Admin functions use appropriate role guards and include necessary zero-address validation.
282-300: LGTM!Appeal filing logic correctly enforces access control, timing windows, and single-appeal constraints. Evidence is appropriately logged via events without incurring storage costs.
302-323: LGTM!Appeal resolution logic correctly enforces governance-only access and maintains the proposal state machine.
325-341: LGTM!Ban management functions are correctly restricted to governance with appropriate validation and event emission.
222-280: Verify partial execution behavior and add tests/documentation.Logic in SlashingManager.sol:227 allows ticket and license slashes in separate transactions, but I found no tests or documentation covering this path. Confirm it’s intentional and add tests or docs to ensure both branches behave correctly.
packages/enclave-contracts/contracts/token/EnclaveToken.sol (5)
1-85: LGTM! Initialization and setup are correct.The constructor properly initializes all roles, state variables, and events. The owner is whitelisted and transfers are restricted by default, which is a secure starting configuration for a governance token.
87-107: LGTM! Minting logic is secure and correct.The function properly validates inputs, enforces the supply cap, and updates state. The order of operations is safe and event emission is appropriate.
145-198: LGTM! Transfer control functions are well-designed.All functions are properly access-controlled, emit appropriate events, and handle edge cases (e.g., zero address checks in
whitelistContracts).
203-214: Verify the transfer restriction logic meets requirements.The current implementation allows transfers if either the sender or recipient is whitelisted. This means:
- Whitelisted addresses can send to anyone (expected).
- Non-whitelisted addresses can send to whitelisted addresses (potentially unintended).
If the intent is to prevent all non-whitelisted transfers during the restricted period, consider requiring the sender to be whitelisted:
if (from != address(0) && to != address(0) && transfersRestricted) { if (!transferWhitelisted[from]) revert TransferNotAllowed(); }Confirm whether the current OR-based logic is intentional (e.g., to allow deposits to whitelisted contracts).
216-233: LGTM! Interface overrides are correct.Both functions properly resolve multi-inheritance ambiguity by delegating to the parent implementation. This is the standard pattern for handling diamond inheritance in Solidity.
packages/enclave-contracts/contracts/registry/BondingRegistry.sol (5)
1-111: LGTM! Contract structure and storage are well-organized.The contract uses clear naming conventions, appropriate access control modifiers, and a comprehensive storage layout. The
noExitInProgressmodifier correctly prevents operations during active exit delays.
113-158: LGTM! Initialization pattern is correct.The constructor and
initializefunction follow the standard upgradeable contract pattern. Ownership is correctly transferred to_ownerafter initialization, and all configuration is validated through setter functions.
160-216: LGTM! View functions are well-implemented.All view functions correctly expose operator state and handle edge cases (e.g., zero
ticketPriceinavailableTickets). The use ofgetPastVotesfor historical balance queries is appropriate for governance integration.
404-502: LGTM! Slashing logic is sound (pending syntax fixes).The slashing functions correctly:
- Prioritize slashing from active balances before pending exits
- Use
Math.minto prevent over-slashing- Update cumulative slashed totals for treasury withdrawal
- Emit appropriate events and update operator status
Once the
require/custom error syntax issues are fixed throughout the contract, this logic will work correctly.
641-658: LGTM! Internal helpers are well-designed.
_updateOperatorStatuscorrectly maintains the active flag based on all relevant conditions, including graceful handling of zeroticketPrice. The percentage-based_minLicenseBondprovides flexibility for active status thresholds.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
34-37: Avoid non-standardhre.network.connect(); usehredirectly and key deployments bychainIdThis issue was flagged in previous review.
hre.network.connect()isn't part of Hardhat's public API. Accessignition/ethersfromhredirectly and derive a stable chain key fromchainId.Based on learnings.
Apply this diff:
- const { ignition, ethers } = await hre.network.connect(); - const [signer] = await ethers.getSigners(); - const chain = (await signer.provider?.getNetwork())?.name ?? "localhost"; + const { ignition, ethers } = hre; + const [signer] = await ethers.getSigners(); + const { chainId } = await ethers.provider.getNetwork(); + const chain = String(chainId);packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol (1)
103-112: Empty committee return may cause division by zero in Enclave.Same issue as in
MockCiphernodeRegistry: the emptynodesarray will cause division by zero inEnclave._distributeRewards(line 371) if this mock is used in integration tests that trigger reward distribution.Apply the same fix as suggested for
MockCiphernodeRegistry.getCommitteeabove, or ensure tests never reach the reward distribution code path when using this mock.packages/enclave-contracts/contracts/Enclave.sol (1)
364-384: Reentrancy protection improved, but division by zero risk remains.Good news: The reentrancy issue flagged in past reviews has been addressed. The code now zeros out
e3Payments[e3Id](line 379) before the external call tobondingRegistry.distributeRewards(line 381), following the checks-effects-interactions pattern correctly.Critical issue remaining: Line 371 divides
e3Payments[e3Id]bycommittee.nodes.lengthwithout checking if the array is empty. IfgetCommitteereturns an empty committee (as the mock implementations do), this will cause a division-by-zero revert.Add a guard before the division:
function _distributeRewards(uint256 e3Id) internal { IRegistryFilter.Committee memory committee = ciphernodeRegistry .getCommittee(e3Id); + require(committee.nodes.length > 0, "Empty committee"); uint256[] memory amounts = new uint256[](committee.nodes.length); // We might need to pay different amounts to different nodes.Note: The
CiphernodeRegistryOwnable.getCommitteefunction (line 229) does validate non-empty committees, but relying on that implicit check is fragile. An explicit check here improves clarity and defense-in-depth.packages/enclave-contracts/scripts/deployEnclave.ts (1)
40-55: Production deployment is blocked—accept USDC address via environment variable.The production branch (lines 52-54) unconditionally throws an error, making it impossible to deploy to production networks without modifying the code.
Accept the USDC token address via an environment variable:
const shouldDeployMocks = process.env.DEPLOY_MOCKS === "true" || withMocks; let usdcTokenAddress: string; if (shouldDeployMocks) { console.log("Deploying mock USDC token..."); const { mockStableToken } = await deployAndSaveMockStableToken({ initialSupply: 1000000, hre, }); usdcTokenAddress = await mockStableToken.getAddress(); console.log("MockUSDC deployed to:", usdcTokenAddress); } else { - throw new Error( - "USDC token address must be provided for production deployment", - ); + const envUsdcAddress = process.env.USDC_TOKEN_ADDRESS; + if (!envUsdcAddress || !ethers.isAddress(envUsdcAddress)) { + throw new Error( + "USDC_TOKEN_ADDRESS environment variable must be set to a valid address for production deployment", + ); + } + usdcTokenAddress = envUsdcAddress; + console.log("Using USDC token at:", usdcTokenAddress); }
🧹 Nitpick comments (14)
packages/enclave-contracts/tasks/ciphernode.ts (4)
85-86: Validate numeric string inputs.The
BigInt()constructor will throw iflicenseBondAmountorticketAmountcontain invalid numeric strings. Consider adding try-catch around the conversion or validating the input format before conversion to provide clearer error messages.+ let licenseBondAmountBigInt: bigint; + let ticketAmountBigInt: bigint; + + try { + licenseBondAmountBigInt = BigInt(licenseBondAmount); + ticketAmountBigInt = BigInt(ticketAmount); + } catch (error) { + throw new Error( + `Invalid numeric input. licenseBondAmount: ${licenseBondAmount}, ticketAmount: ${ticketAmount}`, + ); + } - const licenseBondAmountBigInt = BigInt(licenseBondAmount); - const ticketAmountBigInt = BigInt(ticketAmount);
198-198: Validate siblings input format.If the
siblingsparameter contains invalid comma-separated values (non-numeric strings),BigInt(s)will throw an unclear error. Consider validating the format and providing a helpful error message.- const siblingsArray = siblings.split(",").map((s: string) => BigInt(s)); + const siblingsArray = siblings.split(",").map((s: string) => { + const trimmed = s.trim(); + try { + return BigInt(trimmed); + } catch (error) { + throw new Error(`Invalid sibling value: "${trimmed}". Expected numeric string.`); + } + });
476-480: Consider whether second balance funding is necessary.The impersonated account was funded with 1 ETH at lines 426-430. Unless gas consumption is expected to be extremely high, the second
hardhat_setBalancecall (lines 476-480) may be redundant since the account should still have sufficient ETH from the first funding.
417-510: Consider refactoring repeated impersonation pattern.The task impersonates the ciphernode account twice (lines 421-460 and lines 471-510) with similar setup/teardown code. Consider extracting this pattern into a helper function to reduce duplication and improve maintainability.
Example:
async function impersonateAndExecute( connection: any, address: string, callback: (signer: any) => Promise<void> ) { await connection.provider.request({ method: "hardhat_impersonateAccount", params: [address], }); await connection.provider.request({ method: "hardhat_setBalance", params: [address, "0x1000000000000000000000"], }); try { const signer = await connection.ethers.getSigner(address); await callback(signer); } finally { await connection.provider.request({ method: "hardhat_stopImpersonatingAccount", params: [address], }); } }packages/enclave-contracts/test/SlashingManager.spec.ts (6)
80-209: Consider adding USDC and ticket token setup for the operator.The
setupfixture mints ENCL tokens to the operator (lines 186-190) but doesn't provide USDC or ticket tokens. Since the SlashingManager applies penalties to ticket and license balances via the BondingRegistry, tests that exercise the full slashing flow might need the operator to have:
- USDC balance for purchasing tickets
- Ticket balance for ticket penalty application
- Bonded ENCL tokens in the BondingRegistry
If later tests don't mock or stub these balances, execution penalties might fail silently or produce unexpected results.
Consider adding operator setup like:
await enclaveToken.mintAllocation( operatorAddress, ethers.parseEther("2000"), "Test allocation", ); + +// Provide USDC to operator for ticket purchases +await usdcToken.transfer(operatorAddress, ethers.parseUnits("1000", 6)); + +// Operator approves and bonds tokens (if needed for slashing tests) +await usdcToken.connect(operator).approve(await ticketToken.getAddress(), ethers.parseUnits("100", 6)); +await enclaveToken.connect(operator).approve(await bondingRegistry.getAddress(), ethers.parseEther("1000"));
237-248: Improve error assertion specificity.The test uses
.to.be.rejected(line 247), which passes for any revert reason. This reduces test precision and makes it harder to catch regressions if the contract changes its error handling.If the SlashingManager contract reverts with a custom error for zero addresses, use:
- ).to.be.rejected; + ).to.be.revertedWithCustomError(SlashingManagerFactory, "ZeroAddress");Additionally, consider adding a test for zero bondingRegistry address, since line 229-235 verify the registry is set correctly but don't test the validation.
265-267: Brittle event assertion using Object.values.Using
Object.values(policy)(line 267) to assert event arguments relies on the JavaScript object property iteration order matching the Solidity struct field order. This is fragile and will break silently if:
- The struct field order changes in the contract
- The TypeScript interface/type definition changes
- Object properties are inadvertently reordered
Use explicit individual arguments for clarity and maintainability:
- .withArgs(REASON_MISBEHAVIOR, Object.values(policy)); + .withArgs( + REASON_MISBEHAVIOR, + policy.ticketPenalty, + policy.licensePenalty, + policy.requiresProof, + policy.proofVerifier, + policy.banNode, + policy.appealWindow, + policy.enabled + );This pattern should be applied to similar event assertions throughout the file (e.g., line 292).
506-520: Timestamp assertions are fragile.Lines 518 and similar assertions throughout the file (e.g., line 560) use
currentTime + 1to predict the exact timestamp of the next block. This assumes deterministic single-block increments, which may not hold in all test environments or if Hardhat's behavior changes.Consider using range checks or nearness assertions instead:
- .withArgs( - 0, - operatorAddress, - REASON_MISBEHAVIOR, - ethers.parseUnits("50", 6), - ethers.parseEther("100"), - currentTime + 1, - await slasher.getAddress(), - ); + .to.emit(slashingManager, "SlashProposed"); + + const proposal = await slashingManager.getSlashProposal(0); + expect(proposal.executableAt).to.be.closeTo(currentTime, 2);Alternatively, capture the emitted event and assert on the actual values rather than predicted ones. This pattern applies to multiple tests in the file.
679-812: Consider adding integration tests for penalty application.The
executeSlashtests verify events and state flags but don't validate that penalties are actually applied to the BondingRegistry. Since SlashingManager interacts with BondingRegistry to apply ticket and license penalties, integration tests should verify:
- Ticket balance decreases by the correct penalty amount
- License balance decreases by the correct penalty amount
- Slashed funds are transferred to the treasury
- Proper handling when operator has insufficient balance (partial vs. full execution)
Add test cases that query BondingRegistry, EnclaveToken, and EnclaveTicketToken balances before and after
executeSlashto ensure end-to-end correctness:it("should apply penalties to operator balances", async function () { // Setup: operator bonds tokens and purchases tickets // Propose and execute slash // Assert: balances decreased by expected amounts // Assert: treasury received slashed funds });This would catch integration issues that unit-level state checks might miss.
1077-1140: Add edge cases for ban management.The ban management tests cover the main flows but could benefit from additional edge cases to ensure robustness:
- Attempting to ban a node that is already banned (idempotency/error handling)
- Attempting to unban a node that is not banned (should this error or no-op?)
- Banning a zero address (should be prevented)
it("should revert when banning zero address", async function () { await expect( slashingManager.connect(owner).banNode(ethers.ZeroAddress, reason) ).to.be.revertedWithCustomError(slashingManager, "ZeroAddress"); }); it("should handle banning already banned node", async function () { // Ban once, then try again - verify expected behavior });These tests would ensure the contract handles edge cases gracefully and document expected behavior for future maintainers.
packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol (1)
27-29:isEnabledalways returns true—consider making it configurable for negative test cases.The hardcoded
truereturn prevents testing scenarios where a node is not enabled. Consider adding a mapping to track enabled state or a flag to toggle the return value for more flexible testing.packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (2)
80-86: Naming inconsistency:onlyOwnerOrBondingVaultvsbondingRegistry.The modifier is named
onlyOwnerOrBondingVaultbut checksbondingRegistryand uses errorNotOwnerOrBondingRegistry. Consider renaming toonlyOwnerOrBondingRegistryfor consistency with the storage variable and error names.
142-156: IdempotentaddCiphernodewith silent early return.The function now silently returns early if the node is already enabled (lines 143-145). This makes the operation idempotent, which is useful for bonding operations, but the silent behavior might be unexpected for direct owner calls.
Consider emitting a distinct event or logging when skipping an already-enabled node, if tracking re-add attempts is valuable for monitoring.
packages/enclave-contracts/scripts/deployEnclave.ts (1)
131-156: LGTM: Comprehensive cross-contract wiring.The configuration section properly updates all placeholder addresses with the actual deployed contract addresses. The order appears correct:
- Registry contracts are configured first
- Token contracts are linked to registries
- Manager contracts are wired bidirectionally
Consider adding error handling (try-catch) around these configuration calls to provide clearer error messages if any setter reverts due to validation failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
package.json(2 hunks)packages/enclave-contracts/contracts/Enclave.sol(12 hunks)packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol(10 hunks)packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol(4 hunks)packages/enclave-contracts/hardhat.config.ts(2 hunks)packages/enclave-contracts/package.json(1 hunks)packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts(1 hunks)packages/enclave-contracts/scripts/deployEnclave.ts(3 hunks)packages/enclave-contracts/tasks/ciphernode.ts(1 hunks)packages/enclave-contracts/test/SlashingManager.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-11T13:09:03.800Z
Learnt from: ctrlc03
PR: gnosisguild/enclave#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/scripts/deployAndSave/enclaveToken.ts
📚 Learning: 2025-09-11T13:09:03.800Z
Learnt from: ctrlc03
PR: gnosisguild/enclave#677
File: packages/enclave-contracts/scripts/deployAndSave/naiveRegistryFilter.ts:30-31
Timestamp: 2025-09-11T13:09:03.800Z
Learning: In Hardhat v3, hre.network.name is not available anymore. Use hre.globalOptions.network to get the network name instead.
Applied to files:
packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts
🧬 Code graph analysis (4)
packages/enclave-contracts/scripts/deployEnclave.ts (7)
packages/enclave-contracts/scripts/deployAndSave/mockStableToken.ts (1)
deployAndSaveMockStableToken(25-84)packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
deployAndSaveEnclaveToken(28-109)packages/enclave-contracts/scripts/deployAndSave/enclaveTicketToken.ts (1)
deployAndSaveEnclaveTicketToken(30-101)packages/enclave-contracts/scripts/deployAndSave/slashingManager.ts (1)
deployAndSaveSlashingManager(29-95)packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
deployAndSaveBondingRegistry(36-140)packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
deployAndSaveEnclave(30-108)packages/enclave-contracts/scripts/deployAndSave/naiveRegistryFilter.ts (1)
deployAndSaveNaiveRegistryFilter(21-87)
packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(74-88)storeDeploymentArgs(34-66)
packages/enclave-contracts/hardhat.config.ts (1)
packages/enclave-contracts/tasks/ciphernode.ts (2)
ciphernodeAdminAdd(310-535)ciphernodeMintTokens(220-308)
packages/enclave-contracts/tasks/ciphernode.ts (3)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
deployAndSaveBondingRegistry(36-140)packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
deployAndSaveEnclaveToken(28-109)packages/enclave-contracts/scripts/deployAndSave/mockStableToken.ts (1)
deployAndSaveMockStableToken(25-84)
🪛 GitHub Actions: Build and Deploy Ciphernode
packages/enclave-contracts/tasks/ciphernode.ts
[error] 1-1: ERR_MODULE_NOT_FOUND Cannot find module '/build/packages/enclave-contracts/types' imported from /build/packages/enclave-contracts/tasks/ciphernode.ts. Command failed: 'cd packages/enclave-contracts && pnpm install && pnpm compile:contracts'.
⏰ 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: build_enclave_cli
- GitHub Check: build_e3_support_dev
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: rust_unit
🔇 Additional comments (27)
packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
40-45: LGTM: Clear error handling for missing deploymentThe logic correctly validates that a prior deployment address exists before attempting to connect, with a clear error message.
packages/enclave-contracts/tasks/ciphernode.ts (1)
220-308: LGTM - Well-structured test utility.The
ciphernodeMintTokenstask provides a clean testing utility for token provisioning. Good use of TypeChain factories for proper typing (lines 266-273), clear step logging, and appropriate error handling.packages/enclave-contracts/hardhat.config.ts (1)
16-17: LGTM - Proper task registration.The new tasks
ciphernodeAdminAddandciphernodeMintTokensare correctly imported and registered in the Hardhat configuration.Also applies to: 102-103
package.json (1)
23-23: Verify the script change aligns with intended workflow.The root
ciphernode:addscript now points tociphernode:admin-addinstead of the user-drivenciphernode:addtask. This means developers usingpnpm ciphernode:addfrom the root will get the admin flow (which requires admin privileges and uses impersonation) rather than the self-service flow (which requires users to have their own ENCL/USDC).Ensure this change aligns with the intended developer experience. If both workflows should be accessible, consider:
- Keeping this as
admin-addand documenting when to use each- Or providing both scripts at the root level
packages/enclave-contracts/package.json (1)
153-154: LGTM - Script entries for new tasks.The new scripts correctly map to the Hardhat tasks introduced in
ciphernode.ts, following the existing pattern for task invocation.packages/enclave-contracts/test/SlashingManager.spec.ts (3)
814-1075: Excellent coverage of the appeal lifecycle.The appeal system tests comprehensively cover the entire lifecycle including filing, governance resolution (approve/deny), and interaction with execution. The tests properly verify:
- Authorization checks (only operator can file)
- Time-window constraints
- State transitions (pending → resolved → approved/denied)
- Execution blocking logic based on appeal state
Minor suggestion: Consider adding an edge case test for attempting to resolve an appeal that was never filed (if not already covered by the contract's validation).
1142-1198: LGTM - View functions are well tested.The view function tests appropriately verify that
getSlashPolicyandgetSlashProposalreturn complete and accurate data structures, including:
- All struct fields are returned with correct values
- Computed fields like
proofHashare correctly hashed- Invalid inputs (e.g., non-existent proposal ID) trigger appropriate errors
26-27: No action needed:network.connect()usage is consistent.
The top-levelawait network.connect()pattern is deliberately used across all test suites via Hardhat Ignition’s network helper and requires no changes.packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (7)
28-28: LGTM: Bonding registry storage and event added.The new storage variable and event follow the existing pattern used for the
enclaveregistry.Also applies to: 37-37
57-62: LGTM: Comprehensive error set for bonding integration.The new custom errors provide clear revert messages for bonding-related failures. Note that
Unauthorizedis quite generic—consider using more specific errors where possible to aid debugging.
98-105: LGTM: Zero-address validation added to initialization.The new
requirechecks prevent initializing the contract with invalid zero addresses for owner and enclave, improving contract safety.
158-169: LGTM: Enhanced access control and validation for node removal.The modifier change allows the bonding registry to remove nodes, and the
require(isEnabled(node))check prevents removing non-existent nodes, improving safety.
177-187: LGTM: Setter validation is consistent.Both
setEnclaveandsetBondingRegistrynow validate against zero addresses, following a consistent pattern.
225-230: LGTM:getCommitteevalidates non-empty committee.The function correctly delegates to the registry filter and validates that the committee has been published by checking for non-empty nodes array (line 229). This prevents returning invalid committees.
236-238: LGTM: Simple bonding registry getter.packages/enclave-contracts/contracts/Enclave.sol (8)
11-12: LGTM: Payment and bonding infrastructure added.The new imports and storage variables properly set up the USDC payment system and bonding registry integration. Using
SafeERC20is a good practice.Also applies to: 17-20, 29-29, 38-39, 62-63
94-97: LGTM: Payment-related errors defined.
108-145: LGTM: Initialization updated for new dependencies.Constructor and initialize function properly incorporate the bonding registry and USDC token parameters.
156-157: LGTM: USDC payment collection implemented.The payment model correctly shifts from ETH (
msg.value) to USDC, usingSafeERC20.safeTransferFromfor safe token transfers and recording the fee in thee3Paymentsmapping.Also applies to: 223-225
353-353: Reward distribution triggered after plaintext output.The call to
_distributeRewardsis appropriately placed after successful plaintext output publication. However, see the comments on_distributeRewardsitself for critical issues that need addressing.
413-424: LGTM: Validation added tosetBondingRegistry.The setter now includes proper validation (lines 417-418) to prevent setting a zero address or redundantly setting the same address, addressing the concerns raised in past reviews.
426-436: LGTM: Validation added tosetUsdcToken.The setter now includes proper validation (line 430) to prevent setting a zero address or redundantly setting the same address, addressing the concerns raised in past reviews.
521-525: Placeholder pricing—implement dynamic fee calculation for production.The function returns a hardcoded 1 USDC fee and ignores the request parameters. This is fine for testing but will need a proper pricing model for production (e.g., based on duration, threshold, computation complexity).
Consider implementing dynamic pricing based on:
requestParams.durationrequestParams.threshold(committee size affects cost)- E3 program complexity
Track this as a TODO if not planned for this PR.
packages/enclave-contracts/scripts/deployEnclave.ts (4)
57-82: LGTM: Token and slashing manager deployment.The deployment sequence properly logs addresses and uses placeholder addresses (
addressOne) where cross-contract dependencies will be configured later in the script.
84-129: LGTM: Core contracts deployed in correct order.The deployment sequence handles circular dependencies by using placeholder addresses initially and configuring the actual addresses in the subsequent wiring section.
158-185: LGTM: Mock-specific configuration properly gated.The mock deployment logic is correctly gated by
shouldDeployMocksand sets up the necessary test infrastructure.
187-200: LGTM: Comprehensive deployment summary.The deployment summary provides a clear overview of all deployed contract addresses, which is helpful for testing and verification.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/enclave-contracts/tasks/ciphernode.ts (1)
106-106: Fix typo in console log.The message reads "Licensed bonded" but should be "License bonded".
Apply this diff:
- `Licensed bonded: ${ethers.formatEther(licenseBondAmountBigInt)} ENCL`, + `License bonded: ${ethers.formatEther(licenseBondAmountBigInt)} ENCL`,
🧹 Nitpick comments (1)
packages/enclave-contracts/tasks/ciphernode.ts (1)
456-458: Consider clarifying variable naming.The variable
ciphernodeSigner2suggests this is a second signer, but it's actually a fresh signer instance for the same address after re-impersonating. Consider naming it to reflect the re-impersonation context (e.g.,reimpersonatedCiphernodeSigneror similar) for better clarity.Example:
- const ciphernodeSigner2 = await ethers.getSigner(ciphernodeAddress); - const bondingRegistryAsCiphernode2 = - bondingRegistry.connect(ciphernodeSigner2); + const reimpersonatedCiphernodeSigner = await ethers.getSigner(ciphernodeAddress); + const bondingRegistryReimpersonated = + bondingRegistry.connect(reimpersonatedCiphernodeSigner);And update the subsequent usage:
- const mockUSDCAsCiphernode = mockUSDC.connect(ciphernodeSigner2); + const mockUSDCAsCiphernode = mockUSDC.connect(reimpersonatedCiphernodeSigner);- await bondingRegistryAsCiphernode2.addTicketBalance(ticketAmountWei); + await bondingRegistryReimpersonated.addTicketBalance(ticketAmountWei);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/enclave-contracts/tasks/ciphernode.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/enclave-contracts/tasks/ciphernode.ts (4)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
deployAndSaveBondingRegistry(36-140)packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
deployAndSaveEnclaveToken(28-109)packages/enclave-contracts/scripts/deployAndSave/enclaveTicketToken.ts (1)
deployAndSaveEnclaveTicketToken(30-101)packages/enclave-contracts/scripts/deployAndSave/mockStableToken.ts (1)
deployAndSaveMockStableToken(25-84)
⏰ 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)
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
🔇 Additional comments (6)
packages/enclave-contracts/tasks/ciphernode.ts (6)
1-10: LGTM!The imports are appropriate for the task file functionality.
11-155: Well-structured registration flow with comprehensive error handling.The ciphernodeAdd task correctly implements the multi-step registration process with appropriate balance checks, approvals, and detailed logging at each step.
157-210: LGTM!The deregistration flow is properly implemented with clear user messaging about the exit queue mechanism.
212-293: LGTM!The token minting task is correctly implemented for testing purposes with appropriate validation and balance reporting.
295-505: Well-implemented admin registration with proper impersonation handling.The task correctly orchestrates the complex flow of admin-initiated registration using Hardhat's account impersonation for testing. The two separate impersonation sessions are necessary because impersonation is stopped between operations.
507-539: LGTM!The siblings generation task correctly builds a Lean IMT and generates the merkle proof for the specified ciphernode address.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (1)
364-373: Fix inconsistent method name usage.Line 367 correctly uses the new
isEnabledmethod, but line 371 still references the oldisCiphernodeEligiblemethod. This inconsistency will cause test failures if the old method has been removed.Apply this fix:
- expect(await registry.isCiphernodeEligible(AddressThree)).to.be.false; + expect(await registry.isEnabled(AddressThree)).to.be.false;
♻️ Duplicate comments (2)
packages/enclave-contracts/test/Slashing/SlashingManager.spec.ts (1)
468-476: Fix invalid revert assertion syntax.Line 475 uses
.to.be.revert(ethers), which is not valid Chai-Ethers syntax. According to Hardhat v3 conventions, the correct syntax is.to.be.reverted(without parameters for general reverts) or.to.be.revertedWithCustomError()for specific errors.Based on learnings about Hardhat v3 chai matchers.
Apply this fix:
- ).to.be.revert(ethers); + ).to.be.reverted;Or more specifically, to match the access control pattern used elsewhere in the test:
- ).to.be.revert(ethers); + ).to.be.revertedWithCustomError(slashingManager, "AccessControlUnauthorizedAccount");packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
57-60: Usehredirectly and key bychainIdinstead of network name.The non-standard
hre.network.connect()remains. Accessignitionandethersdirectly fromhreand use numericchainIdas the storage key for deterministic deployments across providers.Based on learnings.
Apply this diff:
- const { ignition, ethers } = await hre.network.connect(); + const { ignition, ethers } = hre; const [signer] = await ethers.getSigners(); - const chain = (await signer.provider?.getNetwork())?.name ?? "localhost"; + const { chainId } = await ethers.provider.getNetwork(); + const chain = String(chainId);
🧹 Nitpick comments (2)
packages/enclave-contracts/test/Slashing/SlashingManager.spec.ts (1)
237-248: Verify the zero-address admin rejection.The test expects deployment to be rejected when admin is
ZeroAddress, but uses a generic.to.be.rejectedassertion. This doesn't validate the specific error that should be thrown by the constructor.Consider strengthening the assertion to check for the specific error:
- ).to.be.rejected; + ).to.be.revertedWithCustomError(slashingManager, "ZeroAddress");This will ensure the constructor fails for the correct reason and improve test precision.
packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (1)
375-384: Consider removing duplicate test block.Lines 364-373 test
isCiphernodeEligible()(which should be migrated toisEnabled), and lines 375-384 testisEnabled()with identical logic. These test blocks appear to be duplicates with one using the old method name.Consider removing the
isCiphernodeEligible()describe block (lines 364-373) entirely, as it's superseded by theisEnabled()tests at lines 375-384. This will eliminate redundancy and ensure tests only reference the current API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts(1 hunks)packages/enclave-contracts/test/Registry/BondingRegistry.spec.ts(1 hunks)packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts(3 hunks)packages/enclave-contracts/test/Registry/NaiveRegistryFilter.spec.ts(1 hunks)packages/enclave-contracts/test/Slashing/SlashingManager.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
PR: gnosisguild/enclave#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/Slashing/SlashingManager.spec.ts
📚 Learning: 2025-09-11T12:56:39.601Z
Learnt from: ctrlc03
PR: gnosisguild/enclave#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/Slashing/SlashingManager.spec.ts
📚 Learning: 2025-09-11T13:09:03.800Z
Learnt from: ctrlc03
PR: gnosisguild/enclave#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/scripts/deployAndSave/enclaveToken.ts
📚 Learning: 2025-09-11T13:09:03.800Z
Learnt from: ctrlc03
PR: gnosisguild/enclave#677
File: packages/enclave-contracts/scripts/deployAndSave/naiveRegistryFilter.ts:30-31
Timestamp: 2025-09-11T13:09:03.800Z
Learning: In Hardhat v3, hre.network.name is not available anymore. Use hre.globalOptions.network to get the network name instead.
Applied to files:
packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts
🧬 Code graph analysis (1)
packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (1)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(74-88)storeDeploymentArgs(34-66)
⏰ 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). (8)
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: build_sdk
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: Build & Push Image
🔇 Additional comments (69)
packages/enclave-contracts/test/Slashing/SlashingManager.spec.ts (42)
41-78: LGTM: Well-structured policy setup helper.The
setupPolicieshelper function provides clean test data for three different policy types (proof-based, evidence-based, and ban policy), which is reused across multiple test cases. The policy configurations are realistic and cover key variations.
80-209: LGTM: Comprehensive test fixture setup.The
setupfixture properly initializes all required contracts (USDC, EnclaveToken, TicketToken, BondingRegistry, SlashingManager, MockVerifier) with appropriate wiring and initial state. The use of Hardhat Ignition modules for deployment and proper contract interconnection demonstrates good integration testing practices.
211-227: LGTM: Constructor tests verify role assignment.Tests correctly verify that both
DEFAULT_ADMIN_ROLEandGOVERNANCE_ROLEare assigned to the owner during initialization.
252-275: LGTM: Policy setting with proper event verification.The test correctly validates policy creation with event emission and subsequent retrieval. Using
Object.values(policy)for event args is a clean pattern.
318-334: LGTM: Zero-reason validation.Test correctly verifies that policies cannot be created with a zero reason identifier.
336-352: LGTM: Disabled policy rejection.Test correctly verifies that disabled policies cannot be set, enforcing that only enabled policies can be created.
354-370: LGTM: No-penalty policy rejection.Test correctly validates that at least one penalty type (ticket or license) must be non-zero.
372-388: LGTM: Proof-without-verifier validation.Test correctly enforces that if
requiresProofis true, a verifier address must be provided.
390-406: LGTM: Proof with appeal window validation.Test correctly validates that proof-based policies (which are immediately executable) cannot have an appeal window.
408-424: LGTM: Evidence without appeal window validation.Test correctly enforces that evidence-based policies (without immediate proof verification) must have an appeal window.
428-446: LGTM: Role management tests.Tests correctly verify adding and removing slasher roles with proper role checks.
448-466: LGTM: Verifier role management.Tests correctly verify adding and removing verifier roles.
478-484: LGTM: Zero-address slasher validation.Test correctly prevents adding zero address as a slasher.
488-527: LGTM: Proof-based slash proposal.Test comprehensively validates the proposal flow with proof verification, including event emission with correct parameters and state checks.
529-569: LGTM: Evidence-based slash proposal.Test correctly validates proposals without proof verification, ensuring the appeal window is properly set and
proofVerifiedis false.
571-582: LGTM: Authorization check for proposals.Test correctly verifies that only slashers can propose slashes.
584-594: LGTM: Zero-address operator validation.Test correctly prevents proposing slashes against zero address.
596-607: LGTM: Disabled reason validation.Test correctly prevents proposing slashes for reasons without configured policies.
609-631: LGTM: Proof requirement validation.Test correctly enforces that empty proof cannot be provided when policy requires proof.
633-676: LGTM: Proposal counter test.Test correctly verifies that
totalProposalsincrements with each proposal.
680-715: LGTM: Immediate execution with proof.Test correctly validates that proof-verified proposals can be executed immediately with proper event emission and state updates.
717-741: LGTM: Appeal window enforcement.Test correctly validates that execution is blocked during the appeal window and allowed after it expires.
743-765: LGTM: Node banning on execution.Test correctly verifies that nodes are banned when the policy requires it, with proper event emission.
767-773: LGTM: Non-existent proposal validation.Test correctly rejects execution of non-existent proposals.
775-790: LGTM: Double execution prevention.Test correctly prevents executing the same proposal twice.
792-811: LGTM: Execution authorization.Test correctly verifies that only slashers can execute proposals.
815-842: LGTM: Appeal filing.Test correctly validates that operators can file appeals with proper event emission and state updates.
844-866: LGTM: Appeal authorization.Test correctly verifies that only the affected operator can file an appeal.
868-892: LGTM: Appeal window expiry.Test correctly prevents filing appeals after the appeal window has expired.
894-918: LGTM: Duplicate appeal prevention.Test correctly prevents filing multiple appeals for the same proposal.
920-958: LGTM: Appeal approval resolution.Test correctly validates governance's ability to approve appeals with proper event emission and state tracking.
960-988: LGTM: Appeal denial resolution.Test correctly validates governance's ability to deny appeals.
990-1015: LGTM: Pending appeal blocks execution.Test correctly verifies that unresolved appeals block execution even after the appeal window expires.
1017-1044: LGTM: Approved appeal blocks execution.Test correctly verifies that approved appeals permanently block execution.
1046-1074: LGTM: Denied appeal allows execution.Test correctly verifies that execution can proceed after an appeal is denied and the window expires.
1078-1091: LGTM: Manual node banning.Test correctly validates governance's ability to manually ban nodes with proper event emission.
1093-1107: LGTM: Node unbanning.Test correctly validates governance's ability to unban nodes.
1109-1118: LGTM: Ban authorization.Test correctly verifies that only governance can ban nodes.
1120-1139: LGTM: Banned node protection.Test correctly prevents proposing slashes against already-banned nodes.
1143-1167: LGTM: Policy retrieval.Test correctly validates that stored policies can be retrieved with all fields intact.
1169-1188: LGTM: Proposal retrieval.Test correctly validates that stored proposals can be retrieved with all fields intact, including the proof hash.
1190-1196: LGTM: Invalid proposal query.Test correctly rejects queries for non-existent proposals.
packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (3)
240-242: LGTM: Error type updated correctly.The test now correctly expects
NotOwnerOrBondingRegistryerror when an unauthorized caller attempts to add a ciphernode, aligning with the updated authorization model.
244-247: LGTM: Method renamed toisEnabled.The test correctly uses the renamed
isEnabledmethod instead of the deprecatedisCiphernodeEligible.
275-277: LGTM: Consistent error handling for removeCiphernode.The test correctly expects
NotOwnerOrBondingRegistryerror for unauthorized removal attempts.packages/enclave-contracts/test/Registry/NaiveRegistryFilter.spec.ts (7)
106-122: LGTM: Test updated for new three-argument API.The test correctly reflects the updated API where:
- Registry is set via
setRegistry- Committee is requested via
registry.requestCommittee(e3Id, filterAddress, threshold)with three arguments- Committee is published via
filter.publishCommittee(e3Id, nodes, publicKey)- Committee data is retrieved and validated
This matches the interface changes described in the AI summary.
134-146: LGTM: Owner authorization check.Test correctly verifies that only the owner can publish committees.
147-168: LGTM: Duplicate publication prevention.Test correctly prevents publishing the same committee twice, following the new API flow.
169-185: LGTM: Node storage validation.Test correctly validates that committee node addresses are stored and retrievable.
186-202: LGTM: Public key storage validation.Test correctly validates that the committee public key is hashed and stored properly.
203-237: LGTM: E3-specific committee data.Tests correctly verify that committees are stored and retrievable per E3 ID with correct nodes and public keys.
255-277: LGTM: Complete committee retrieval.Test correctly validates that all committee fields (threshold, nodes, publicKey) are properly stored and retrieved for a given E3 ID.
packages/enclave-contracts/test/Registry/BondingRegistry.spec.ts (13)
1-31: LGTM! Well-structured test imports and setup.The imports, hash helper using
poseidon2, and reason constants are correctly configured for the BondingRegistry test suite.
202-226: LGTM! Constructor tests verify all initial parameters.The test correctly validates that all configuration values are set during initialization.
228-303: LGTM! Comprehensive bondLicense tests.The tests cover successful bonding, zero amount validation, exit-in-progress blocking, and accumulation behavior. Event assertions and balance checks are properly implemented.
305-371: LGTM! Complete unbondLicense test coverage.The tests properly validate unbonding flows including zero amount checks, insufficient balance handling, and exit queue tracking.
373-441: LGTM! Thorough registerOperator tests.The tests validate the registration flow including license requirements, duplicate prevention, and exit clearing during re-registration. The distinction between
isRegisteredandisActivestates is properly tested.
443-507: LGTM! Complete deregisterOperator test coverage.The tests properly validate deregistration flows including state transitions, exit queuing, and timestamp calculations. The verification that both ticket and license balances are queued for exit is important.
509-603: LGTM! Complete addTicketBalance test coverage.The tests validate ticket balance additions including activation threshold behavior, registration requirements, and zero amount checks. The activation event tracking is properly tested.
605-728: LGTM! Thorough removeTicketBalance tests.The tests cover ticket removal including exit queuing, deactivation when falling below minimum balance, and insufficient balance handling. The state transition testing is well-implemented.
730-846: LGTM! Comprehensive claimExits tests.The tests validate exit claiming including time-based restrictions, full and partial claims, and balance updates. The partial claim test properly verifies remaining pending exits are tracked correctly.
848-876: LGTM! Proper isLicensed threshold tests.The tests correctly validate the license bond threshold at 80% (licenseActiveBps), testing both the minimum required amount and amounts just below the threshold.
878-913: LGTM! Correct availableTickets calculation tests.The tests validate ticket calculation logic including division by ticket price and zero price handling.
915-1016: LGTM! Complete admin function tests.The tests thoroughly validate all admin functions including parameter validation (zero checks, bounds checks), event emissions, and ownership restrictions. The basis points validation correctly enforces the 0-10000 range.
1018-1158: LGTM! Excellent edge case and lifecycle tests.The tests validate complex scenarios including:
- Operator state changes due to license reduction
- Multiple operators with different states operating independently
- Complete operator lifecycle from bonding through exit and re-registration
These integration-level tests ensure the contract handles real-world operator flows correctly.
packages/enclave-contracts/scripts/deployAndSave/enclaveToken.ts (4)
23-44: Good refactor: duplication eliminated.The helper function successfully addresses the duplicate transfer-restriction logic flagged in previous reviews. The implementation correctly checks the current state before toggling and handles errors gracefully.
61-77: LGTM: Reuse logic is sound.The path for reusing existing deployments correctly validates the presence of an address, connects to the contract, and uses the extracted helper for transfer restrictions.
79-103: LGTM: Deployment and persistence logic is correct.The new deployment path properly uses Ignition modules, captures deployment metadata (address, block number, constructor args), and persists the data for future reuse.
105-113: LGTM: Final setup is correct.The contract connection and transfer restriction handling (via the helper) are correctly implemented before returning the deployed contract.
hmzakhalid
requested review from
0xjei,
auryn-macmillan,
cedoor,
ctrlc03 and
ryardley
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
examples/CRISP/server/.env.example (1)
13-16: Address updates look good; E3_PROGRAM_ADDRESS matches client config.The updated contract addresses align with the new deployments. Confirmed that E3_PROGRAM_ADDRESS (0x1613...) is consistent with the client configuration.
Optionally, consider standardizing the quote style for consistency:
-ENCLAVE_ADDRESS="0x8A791620dd6260079BF849Dc5567aDC3F2FdC318" -CIPHERNODE_REGISTRY_ADDRESS="0xa513E6E4b8f2a923D98304ec87F64353C4D5C853" -NAIVE_REGISTRY_FILTER_ADDRESS="0x610178dA211FEF7D417bC0e6FeD39F05609AD788" -E3_PROGRAM_ADDRESS="0x1613beB3B2C4f22Ee086B2b38C1476A3cE7f78E8" # CRISPProgram Contract Address +ENCLAVE_ADDRESS=0x8A791620dd6260079BF849Dc5567aDC3F2FdC318 +CIPHERNODE_REGISTRY_ADDRESS=0xa513E6E4b8f2a923D98304ec87F64353C4D5C853 +NAIVE_REGISTRY_FILTER_ADDRESS=0x610178dA211FEF7D417bC0e6FeD39F05609AD788 +E3_PROGRAM_ADDRESS=0x1613beB3B2C4f22Ee086B2b38C1476A3cE7f78E8 # CRISPProgram Contract Addressexamples/CRISP/enclave.config.yaml (1)
5-7: Consider adding e3_program address for consistency.The
templates/default/enclave.config.yamlincludes ane3_programcontract address (line 5), but this CRISP example configuration does not. If the CRISP example is meant to be a full-featured configuration template, consider whethere3_programshould also be included here.examples/CRISP/deploy/config.toml (1)
17-17: enclaveAddress update is consistent and correct.The new address
0x8A791620dd6260079BF849Dc5567aDC3F2FdC318is used uniformly across deployment configs, templates, and integration tests. No mismatches detected. Consider refactoring to programmatically read the deployed address from your artifacts in the future to avoid manual hardcoding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/CRISP/Readme.md(4 hunks)examples/CRISP/client/.env.example(1 hunks)examples/CRISP/deploy/config.toml(1 hunks)examples/CRISP/enclave.config.yaml(1 hunks)examples/CRISP/server/.env.example(1 hunks)templates/default/enclave.config.yaml(1 hunks)tests/integration/enclave.config.yaml(1 hunks)tests/integration/fns.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/integration/enclave.config.yaml
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
examples/CRISP/server/.env.example
[warning] 13-13: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 14-14: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 14-14: [UnorderedKey] The CIPHERNODE_REGISTRY_ADDRESS key should go before the ENCLAVE_ADDRESS key
(UnorderedKey)
[warning] 15-15: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 16-16: [UnorderedKey] The E3_PROGRAM_ADDRESS key should go before the ENCLAVE_ADDRESS key
(UnorderedKey)
examples/CRISP/client/.env.example
[warning] 4-4: [UnorderedKey] The VITE_E3_PROGRAM_ADDRESS key should go before the VITE_ENCLAVE_API key
(UnorderedKey)
[warning] 4-4: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 5-5: [UnorderedKey] The VITE_SEMAPHORE_ADDRESS key should go before the VITE_TWITTER_SERVERLESS_API key
(UnorderedKey)
🪛 Shellcheck (0.11.0)
tests/integration/fns.sh
[warning] 24-24: ENCLAVE_CONTRACT appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 25-25: REGISTRY_CONTRACT appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 26-26: REGISTRY_FILTER_CONTRACT appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 27-27: INPUT_VALIDATOR_CONTRACT appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (8)
- GitHub Check: Build & Push Image
- GitHub Check: build_sdk
- GitHub Check: rust_unit
- GitHub Check: test_net
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
- GitHub Check: Build & Push Image
🔇 Additional comments (6)
examples/CRISP/client/.env.example (1)
4-5: LGTM! Address updates are consistent with new deployments.The updated default addresses for E3_PROGRAM and SEMAPHORE contracts align with the new economic contracts introduced in this PR.
examples/CRISP/server/.env.example (1)
20-27: LGTM! New E3 configuration variables have sensible defaults.The new E3 configuration extends functionality for thresholds, duration, and compute provider settings. All values are appropriate for local testing:
- Threshold range (1-2) suits minimal node setups
- Batch size of 4 satisfies the power-of-2 requirement
- RISC0 provider with serial processing is a safe default
examples/CRISP/enclave.config.yaml (1)
5-7: Contract addresses match template configuration.The addresses for
enclave,ciphernode_registry, andfilter_registrycorrectly match those intemplates/default/enclave.config.yaml, ensuring consistency across configurations.templates/default/enclave.config.yaml (1)
5-8: No address mismatches detected.The enclave, ciphernode_registry, and filter_registry values in templates/default/enclave.config.yaml align exactly with those in tests/integration/enclave.config.yaml and examples/CRISP/enclave.config.yaml.
examples/CRISP/Readme.md (2)
25-25: LGTM! Formatting improvements enhance readability.The added blank lines improve the visual structure of the documentation without altering any instructions or behavior.
Also applies to: 105-105
168-170: LGTM! Documentation formatting improved.The section header and note formatting changes improve readability without altering the instructions.
ctrlc03
left a comment
ctrlc03
left a comment
There was a problem hiding this comment.
Great work :) left some comment, will give it another pass tomorrow as there's some parts of the contracts I wanted to spend some more time on
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
examples/CRISP/client/.env.example (1)
4-4: Address update aligns with PR objectives; minor linting warnings to address.The E3 program address update reflects the new economic contract deployment from the PR. However, dotenv-linter flags two minor issues:
- Key ordering:
VITE_E3_PROGRAM_ADDRESSshould precedeVITE_ENCLAVE_APIalphabetically.- Value quoting: The address value should be quoted for consistency.
If your project enforces these linting standards, apply this diff:
-VITE_E3_PROGRAM_ADDRESS=0xc5a5C42992dECbae36851359345FE25997F5C42d # Default E3 program address from hardhat +VITE_E3_PROGRAM_ADDRESS="0xc5a5C42992dECbae36851359345FE25997F5C42d" # Default E3 program address from hardhatAnd reorder the keys alphabetically if required by your project standards.
examples/CRISP/server/.env.example (1)
15-19: Remove quote characters and sort keys alphabetically for consistency.The linter flags unnecessary quote characters on lines 16–17 and suggests alphabetizing keys. While quotes are functionally acceptable in env files, removing them and reordering alphabetically aligns with dotenv style conventions.
Apply this refactor to clean up the address configuration:
- # Based on Default Anvil Deployments (Only for testing) - ENCLAVE_ADDRESS="0x8A791620dd6260079BF849Dc5567aDC3F2FdC318" - CIPHERNODE_REGISTRY_ADDRESS="0x2279B7A0a67DB372996a5FaB50D91eAA73d2eBe6" - E3_PROGRAM_ADDRESS="0xc5a5C42992dECbae36851359345FE25997F5C42d" # CRISPProgram Contract Address - FEE_TOKEN_ADDRESS="0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0" + # Based on Default Anvil Deployments (Only for testing) + CIPHERNODE_REGISTRY_ADDRESS=0x2279B7A0a67DB372996a5FaB50D91eAA73d2eBe6 + E3_PROGRAM_ADDRESS=0xc5a5C42992dECbae36851359345FE25997F5C42d # CRISPProgram Contract Address + ENCLAVE_ADDRESS=0x8A791620dd6260079BF849Dc5567aDC3F2FdC318 + FEE_TOKEN_ADDRESS=0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (1)
34-41: Consider renamingvotetoencryptedDatafor clarity.The field
encryptedVote.votecontainsresult.encryptedData, which may be clearer if namedencryptedVote.encryptedDataor simplyencryptedVote.datato better reflect its contents.self.postMessage({ type: 'encrypt_vote', success: true, encryptedVote: { - vote: result.encryptedData, + encryptedData: result.encryptedData, proofData: result.proof, }, })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/CRISP/client/.env.example(1 hunks)examples/CRISP/client/libs/wasm/pkg/crisp_worker.js(1 hunks)examples/CRISP/server/.env.example(1 hunks)examples/CRISP/server/src/server/indexer.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
PR: gnosisguild/enclave#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/server/indexer.rs
📚 Learning: 2024-11-05T03:56:22.254Z
Learnt from: ryardley
PR: gnosisguild/enclave#170
File: packages/ciphernode/evm/tests/evm_reader.rs:63-63
Timestamp: 2024-11-05T03:56:22.254Z
Learning: In the Rust test function `test_logs` in `packages/ciphernode/evm/tests/evm_reader.rs`, a sleep duration of 1 millisecond is sufficient for reliable event processing, even in CI environments.
Applied to files:
examples/CRISP/server/src/server/indexer.rs
📚 Learning: 2024-10-22T03:30:03.606Z
Learnt from: ryardley
PR: gnosisguild/enclave#145
File: packages/ciphernode/enclave_node/src/shutdown.rs:21-26
Timestamp: 2024-10-22T03:30:03.606Z
Learning: In `packages/ciphernode/enclave_node/src/shutdown.rs`, the sleep duration after aborting the task is necessary to wait for other processes to complete during shutdown.
Applied to files:
examples/CRISP/server/src/server/indexer.rs
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
PR: gnosisguild/enclave#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:
examples/CRISP/server/.env.example
📚 Learning: 2025-10-10T12:56:40.538Z
Learnt from: 0xjei
PR: gnosisguild/enclave#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:
examples/CRISP/server/.env.exampleexamples/CRISP/client/.env.example
🧬 Code graph analysis (2)
examples/CRISP/server/src/server/indexer.rs (1)
crates/evm-helpers/src/listener.rs (2)
contract_address(99-99)create_contract_listener(97-104)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (1)
packages/enclave-sdk/src/enclave-sdk.ts (1)
EnclaveSDK(48-614)
🪛 dotenv-linter (4.0.0)
examples/CRISP/server/.env.example
[warning] 16-16: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 17-17: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 17-17: [UnorderedKey] The CIPHERNODE_REGISTRY_ADDRESS key should go before the ENCLAVE_ADDRESS key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The E3_PROGRAM_ADDRESS key should go before the ENCLAVE_ADDRESS key
(UnorderedKey)
examples/CRISP/client/.env.example
[warning] 4-4: [UnorderedKey] The VITE_E3_PROGRAM_ADDRESS key should go before the VITE_ENCLAVE_API key
(UnorderedKey)
[warning] 4-4: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🔇 Additional comments (10)
examples/CRISP/server/.env.example (1)
29-30: LGTM — sensible E3 compute provider defaults.The new
E3_COMPUTE_PROVIDER_PARALLELandE3_COMPUTE_PROVIDER_BATCH_SIZEsettings map to the PR's E3 compute provider configuration. The batch size of 4 is a valid power of 2, and the parallel flag defaults tofalsefor safe local testing. The inline comment about batch size being a power of 2 is helpful for developers.examples/CRISP/server/src/server/indexer.rs (5)
15-15: LGTM! Import changes support RPC-based timing.The addition of
ProviderandProviderBuilderenables RPC time retrieval, while replacingInstantwithtokio::time::sleepaligns with the async event-driven architecture.Also applies to: 35-36
331-339: Consider block timestamp granularity for timing-sensitive operations.The function correctly retrieves on-chain time, which is appropriate for blockchain coordination. However, note that:
- Block timestamps have limited granularity (updated only when new blocks are produced), potentially lagging real time by several seconds.
- A new provider connection is created on each call. For high-frequency usage, consider caching the provider instance.
For the current use case (activation and expiration timing), this approach is acceptable and correctly aligns with on-chain state.
157-168: LGTM! Expiration timing correctly uses RPC-based time.The wait duration calculation properly handles both future expirations and already-expired E3s. Using RPC time ensures alignment with on-chain state, which is appropriate for this blockchain-coordinated workflow.
299-319: Excellent! Activation timing now correctly respects the start window.This implementation properly addresses the previous review concern (✅ commit 9366fe6) by calculating the wait duration based on
e3.startWindow[0]and RPC time, replacing the problematic fixed 1-second sleep. The activation will now occur at the correct time, preventing premature activation attempts.The idempotency check (lines 294-297) and detailed logging ensure robust behavior.
344-344: LGTM! Parameter rename improves clarity.The rename from
registry_filter_addresstoregistry_addressbetter reflects the parameter's purpose and usage.Also applies to: 367-367
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (4)
7-8: LGTM: Import statements are correct.The imports correctly reference the updated SDK package and circuit configuration.
32-32: LGTM: Encryption call correctly uses updated SDK interface.The call to
encryptNumberAndGenProofcorrectly passes the required parameters and the result structure aligns with the SDK'sVerifiableEncryptionResulttype.
42-44: LGTM: Error handling is appropriate.The error handling correctly reports failures back to the main thread with clear error messages.
47-49: LGTM: Default case appropriately handles unknown message types.The console error for unknown message types provides helpful debugging information.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (1)
20-23: Contract address duplication issue persists.Both
enclaveandciphernodeRegistryshare the same address (0xc6e7DF5E7b4f2A278906862b61205850344D4e7d). Even in example code, distinct contracts should have unique addresses to avoid misleading developers.Apply this diff to use distinct placeholder addresses:
contracts: { - enclave: '0xc6e7DF5E7b4f2A278906862b61205850344D4e7d', - ciphernodeRegistry: '0xc6e7DF5E7b4f2A278906862b61205850344D4e7d', + enclave: '0x5FbDB2315678afecb367f032d93F642f64180aa3', + ciphernodeRegistry: '0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512', },
🧹 Nitpick comments (1)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (1)
27-27: Consider adding a comment clarifying this is for local development.The hardcoded Anvil private key is appropriate for example code. However, adding a brief comment clarifying this is only for local testing would make the intent more explicit.
Apply this diff:
- privateKey: '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80', + privateKey: '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80', // Anvil default key - local development only
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
PR: gnosisguild/enclave#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/client/libs/wasm/pkg/crisp_worker.js
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
PR: gnosisguild/enclave#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:
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js
🧬 Code graph analysis (1)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (2)
packages/enclave-sdk/src/index.ts (2)
EnclaveSDK(8-8)FheProtocol(47-47)packages/enclave-sdk/src/enclave-sdk.ts (1)
EnclaveSDK(48-614)
🪛 Gitleaks (8.28.0)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js
[high] 27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: build_e3_support_risc0
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
examples/CRISP/server/src/cli/commands.rs (2)
118-164: Fee quote uses stalestart_window— refresh invalidates the quote.The fee quote at lines 138-147 uses the
start_windowcalculated at lines 119-122 (based oncurrent_timestampfrom line 118). However, after token approval, the code refreshescurrent_timestamp(line 160) and recalculatesstart_window(lines 161-164). The subsequentrequest_e3call (lines 179-189) uses this newstart_window, which differs from the one used to obtain the quote.If the contract's fee calculation depends on
start_windowparameters, the approved amount may be insufficient or the quote may be rejected, causing the transaction to revert.Options to fix:
- Re-fetch the quote after refreshing the timestamp and before calling
request_e3:current_timestamp = get_current_timestamp().await?; start_window = [ U256::from(current_timestamp), U256::from(current_timestamp + CONFIG.e3_window_size as u64), ]; + + // Re-fetch quote with updated start_window + let updated_fee = contract + .get_e3_quote( + threshold, + start_window, + duration, + e3_program, + e3_params.clone(), + compute_provider_params_bytes.clone(), + ) + .await?; + + if updated_fee > fee_amount { + info!("Fee increased, approving additional tokens..."); + approve::approve_token( + &CONFIG.http_rpc_url, + &CONFIG.private_key, + &CONFIG.fee_token_address, + &CONFIG.enclave_address, + updated_fee, + ) + .await?; + }
- Do not refresh the timestamp/start_window after obtaining the quote — use the original values for
request_e3to maintain consistency.
61-70: Remove.unwrap()to prevent potential panic.Line 66 uses
.unwrap()on the provider result, which can cause a panic if the blockchain query fails. This breaks the error handling contract of the function.Apply this diff to properly propagate the error:
pub async fn get_current_timestamp() -> Result<u64, Box<dyn std::error::Error + Send + Sync>> { let provider = ProviderBuilder::new().connect(&CONFIG.http_rpc_url).await?; let block = provider .get_block_by_number(alloy::eips::BlockNumberOrTag::Latest) .await - .unwrap() - .ok_or_else(|| anyhow::anyhow!("Latest block not found"))?; + .map_err(|e| Box::new(e) as Box<dyn std::error::Error + Send + Sync>)? + .ok_or_else(|| anyhow::anyhow!("Latest block not found"))?; Ok(block.header.timestamp) }
🧹 Nitpick comments (1)
examples/CRISP/server/src/cli/commands.rs (1)
132-178: Consider usingdebug!ortrace!for verbose diagnostic logs.Lines 132-136 and 168-177 contain multiple diagnostic logs prefixed with "Debug -". These are currently logged at
info!level, which may clutter production logs.Consider changing these to
debug!ortrace!level:- info!("Debug Before Fee Quote - start_window: {:?}", start_window); + debug!("Before fee quote - start_window: {:?}", start_window); - info!( - "Debug Before Fee Quote - current timestamp: {:?}", - current_timestamp - ); + debug!("Before fee quote - current timestamp: {:?}", current_timestamp);And similarly for lines 168-177:
- info!("Debug - threshold: {:?}", threshold); - info!("Debug - start_window: {:?}", start_window); - info!("Debug - current timestamp: {:?}", current_timestamp); - info!("Debug - duration: {}", duration); - info!("Debug - e3_program: {}", e3_program); + debug!("Request parameters - threshold: {:?}, start_window: {:?}, timestamp: {:?}", threshold, start_window, current_timestamp); + debug!("Request parameters - duration: {}, e3_program: {}", duration, e3_program);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/CRISP/server/src/cli/commands.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-28T10:45:29.100Z
Learnt from: ryardley
PR: gnosisguild/enclave#156
File: packages/ciphernode/cipher/src/cipher.rs:164-164
Timestamp: 2024-10-28T10:45:29.100Z
Learning: In test code, it's acceptable to use `.unwrap()` instead of propagating errors with the `?` operator in Rust.
Applied to files:
examples/CRISP/server/src/cli/commands.rs
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
PR: gnosisguild/enclave#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
🧬 Code graph analysis (1)
examples/CRISP/server/src/cli/commands.rs (2)
crates/evm-helpers/src/contracts.rs (1)
new(195-201)examples/CRISP/server/src/cli/approve.rs (1)
approve_token(20-65)
⏰ 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_sdk
- GitHub Check: test_net
- GitHub Check: test_contracts
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: build_e3_support_risc0
- GitHub Check: Build & Push Image
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
examples/CRISP/test/crisp.spec.ts (1)
51-52: Replace fixed timeout with explicit wait for Committee publication.Hard-coded timeouts make E2E tests flaky. The 4-second wait may be too short under load or unnecessarily long in fast environments.
Consider waiting for a deterministic signal instead:
- Use
page.waitForSelector()for a DOM element that appears when Committee is published- Poll an API endpoint with
page.waitForFunction()to check publication status- Listen for a specific network request completion
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (1)
21-23: All three contracts still share the same address.This issue was previously flagged: all three contract addresses (enclave, ciphernodeRegistry, feeToken) are set to the same value
0xc6e7DF5E7b4f2A278906862b61205850344D4e7d. Even in example code, these should be distinct contract instances to avoid misleading developers.
🧹 Nitpick comments (2)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (1)
28-28: Consider adding a production warning comment.While this is clearly marked as the default Anvil test key, adding an explicit warning comment (e.g., "// WARNING: Never use this key in production") would help prevent accidental misuse if developers copy this example code.
examples/CRISP/server/src/server/indexer.rs (1)
333-341: Consider reusing the provider to avoid connection overhead.Creating a new provider on every call to
get_current_timestamp_rpc()incurs connection setup overhead (potentially including DNS resolution, TCP handshake, and TLS negotiation). Since this function is called in time-sensitive activation and expiration paths, the overhead could accumulate.Consider one of these approaches:
- Pass an existing provider as a parameter (preferred if a provider is already available in the calling context)
- Use a cached/shared provider (e.g.,
Arc<Provider>stored in a static or passed through the handler context)- Reuse the WebSocket provider already created in
start_indexerinstead of creating a separate HTTP providerExample of approach 1:
-pub async fn get_current_timestamp_rpc() -> eyre::Result<u64> { - let provider = ProviderBuilder::new().connect(&CONFIG.http_rpc_url).await?; +pub async fn get_current_timestamp_rpc<P: Provider>(provider: &P) -> eyre::Result<u64> { let block = provider .get_block_by_number(alloy::eips::BlockNumberOrTag::Latest) .await? .ok_or_else(|| eyre::eyre!("Latest block not found"))?; Ok(block.header.timestamp) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js(1 hunks)examples/CRISP/server/src/server/indexer.rs(6 hunks)examples/CRISP/test/crisp.spec.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
PR: gnosisguild/enclave#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/server/indexer.rs
📚 Learning: 2024-11-05T03:56:22.254Z
Learnt from: ryardley
PR: gnosisguild/enclave#170
File: packages/ciphernode/evm/tests/evm_reader.rs:63-63
Timestamp: 2024-11-05T03:56:22.254Z
Learning: In the Rust test function `test_logs` in `packages/ciphernode/evm/tests/evm_reader.rs`, a sleep duration of 1 millisecond is sufficient for reliable event processing, even in CI environments.
Applied to files:
examples/CRISP/server/src/server/indexer.rs
📚 Learning: 2024-10-22T03:30:03.606Z
Learnt from: ryardley
PR: gnosisguild/enclave#145
File: packages/ciphernode/enclave_node/src/shutdown.rs:21-26
Timestamp: 2024-10-22T03:30:03.606Z
Learning: In `packages/ciphernode/enclave_node/src/shutdown.rs`, the sleep duration after aborting the task is necessary to wait for other processes to complete during shutdown.
Applied to files:
examples/CRISP/server/src/server/indexer.rs
📚 Learning: 2024-09-26T04:59:28.043Z
Learnt from: ryardley
PR: gnosisguild/enclave#107
File: tests/basic_integration/test.sh:142-142
Timestamp: 2024-09-26T04:59:28.043Z
Learning: In the integration test script `tests/basic_integration/test.sh`, using a fixed sleep to wait for the input deadline is acceptable.
Applied to files:
examples/CRISP/test/crisp.spec.ts
📚 Learning: 2024-09-26T04:26:00.779Z
Learnt from: ryardley
PR: gnosisguild/enclave#107
File: tests/basic_integration/test.sh:142-142
Timestamp: 2024-09-26T04:26:00.779Z
Learning: In the `tests/basic_integration/test.sh` script, using fixed sleep durations is acceptable, and adding more complex waiting mechanisms may introduce unnecessary complexity for the test.
Applied to files:
examples/CRISP/test/crisp.spec.ts
📚 Learning: 2024-09-26T04:15:32.544Z
Learnt from: ryardley
PR: gnosisguild/enclave#107
File: tests/basic_integration/test.sh:81-83
Timestamp: 2024-09-26T04:15:32.544Z
Learning: In `tests/basic_integration/test.sh`, it's acceptable to wait indefinitely for the EVM node to start without a timeout, as it's unlikely to fail here.
Applied to files:
examples/CRISP/test/crisp.spec.ts
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
PR: gnosisguild/enclave#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/client/libs/wasm/pkg/crisp_worker.js
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
PR: gnosisguild/enclave#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:
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js
🧬 Code graph analysis (2)
examples/CRISP/server/src/server/indexer.rs (1)
crates/evm-helpers/src/listener.rs (2)
contract_address(99-99)create_contract_listener(97-104)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (1)
packages/enclave-sdk/src/enclave-sdk.ts (1)
EnclaveSDK(48-614)
🪛 Gitleaks (8.28.0)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js
[high] 23-23: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 28-28: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: integration_prebuild
- GitHub Check: test_net
- GitHub Check: build_e3_support_risc0
- GitHub Check: build_sdk
- GitHub Check: test_contracts
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (9)
examples/CRISP/test/crisp.spec.ts (1)
13-26: LGTM!The try/catch block with error logging improves observability for debugging test failures. Rethrowing the error ensures the test still fails appropriately.
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (4)
7-8: LGTM! Clean import refactoring.The import statements are properly formatted with consistent quote styles and trailing semicolons removed.
11-13: LGTM! Improved message handling structure.The refactored event handling using destructuring and a switch statement is cleaner and more maintainable than the previous approach.
32-44: LGTM! Result handling correctly updated for new SDK API.The code properly uses the updated SDK return shape (
result.encryptedDataandresult.proof) and includes appropriate error handling.
47-49: LGTM! Good defensive programming.The default case appropriately handles unknown message types with logging.
examples/CRISP/server/src/server/indexer.rs (4)
15-15: LGTM! Import changes support RPC-based timing.The shift from system time (
SystemTime,UNIX_EPOCH,Instant,sleep_until) to RPC-based timing (Provider,ProviderBuilder,sleep) is consistent with the refactored timing logic that now queries blockchain timestamps.Also applies to: 35-36
157-168: LGTM! Expiration timing logic is correct.The RPC-based timing approach properly calculates the wait duration until expiration, handles already-expired E3s with
Duration::ZERO, and includes helpful logging. Using blockchain time rather than system time is more accurate for blockchain-based deadlines.
299-321: LGTM! Activation timing now correctly respects the start window.The implementation properly calculates the wait duration based on
e3.startWindow[0]and current RPC time, addressing the concern from the previous review about premature activation. The verbose logging is helpful for debugging timing-related issues.
346-346: LGTM! Parameter rename improves clarity.Renaming
registry_filter_addresstoregistry_addressis more accurate since the parameter represents a contract address rather than a filter. The usage is updated consistently.Also applies to: 369-369
There was a problem hiding this comment.
Did a sweep over the ciphernode changes and have added some thoughts. Still need some more time with this to go over the logic and make sure I understand what is happening and maybe clear some of these comments up but I thought I would post these for now and come back in an hour or two.
|
I made an issue to capture the ciphernode architecture stuff: #939 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
crates/ciphernode-builder/src/ciphernode_builder.rs (1)
225-227: Comment still incorrectly says "writable".The comment claims this method sets up a "writable BondingRegistry," but the implementation at lines 343-354 only attaches a reader (uses
read_providerandbonding_registry_readerrepository).Apply this diff to correct the comment:
- /// Setup a writable BondingRegistry for every evm chain provided + /// Setup a BondingRegistry reader for every EVM chain provided pub fn with_contract_bonding_registry(mut self) -> Self {crates/tests/tests/integration.rs (1)
220-232: Derive the expected committee from the nodes under test.Hardcoding the five addresses couples the test to the current RNG output and sortition implementation; any minor change in node generation breaks the assertion. Reuse the nodes you just built (or the same selection helper used in production) to compute the committee instead of literal strings—this mirrors the earlier feedback on the same block.
- let committee: Vec<String> = vec![ - "0x8f32E487328F04927f20c4B14399e4F3123763df".to_string(), - "0x95b8a2b9b93aE9e0F13e215A49b8C53172c4f4ba".to_string(), - "0x8966a013047aef67Cac52Bc96eB77bC11B5D2572".to_string(), - "0x2B1eD59AC30f668B5b9EcF3D8718A44C15E0E479".to_string(), - "0x83A06c5Ac9E4207526C3eFA79812808428Dd5FaB".to_string(), - ]; + let committee: Vec<String> = nodes + .iter() + .map(|node| node.address()) + .take(threshold_n as usize) + .collect();crates/aggregator/src/committee_finalizer.rs (1)
107-125: Cancel existing scheduled finalizations before inserting the new handle.A repeated
CommitteeRequestedfor the samee3_idreplaces the map entry but leaves the oldSpawnHandleactive, so both timers fire and we emitCommitteeFinalizeRequestedtwice. Clean out and cancel the previous handle before scheduling the new one.- let handle = ctx.run_later( + if let Some(prev) = act + .pending_committees + .remove(&e3_id_for_async.to_string()) + { + ctx.cancel_future(prev); + } + + let handle = ctx.run_later( Duration::from_secs(seconds_until_deadline), move |act, _ctx| { info!(e3_id = %e3_id_clone, "Dispatching CommitteeFinalizeRequested event"); @@ - act.pending_committees - .insert(e3_id_for_async.to_string(), handle); + act.pending_committees + .insert(e3_id_for_async.to_string(), handle);crates/evm/src/ciphernode_registry_sol.rs (2)
439-459: CRITICAL: Manual nonce management still present despite past review.A previous review flagged manual nonce management as causing concurrency issues and was marked "Addressed in commit c5658c6", but the code still explicitly fetches and sets nonces (lines 447-456). Multiple concurrent calls will fetch the same pending nonce, causing "nonce too low" or "already used" errors.
Apply this diff to let the provider auto-fill nonces:
pub async fn submit_ticket_to_registry<P: Provider + WalletProvider + Clone>( provider: EthProvider<P>, contract_address: Address, e3_id: E3id, ticket_number: u64, ) -> Result<TransactionReceipt> { let e3_id: U256 = e3_id.try_into()?; let ticket_number = U256::from(ticket_number); - let from_address = provider.provider().default_signer_address(); - let current_nonce = provider - .provider() - .get_transaction_count(from_address) - .pending() - .await?; let contract = ICiphernodeRegistry::new(contract_address, provider.provider()); - let builder = contract + let receipt = contract .submitTicket(e3_id, ticket_number) - .nonce(current_nonce); - let receipt = builder.send().await?.get_receipt().await?; + .send() + .await? + .get_receipt() + .await?; Ok(receipt) }
479-504: CRITICAL: Manual nonce management still present; MAJOR: Silent address filtering drops invalid nodes.Two issues:
- Manual nonce management (lines 492-497, 501): Same concurrency hazard as other functions—marked addressed but unchanged.
- Silent address filtering (lines 488-491): Invalid node addresses are silently dropped via
filter_map(|node| node.parse().ok()). This can cause incomplete committee publication without any error indication, which is a correctness issue.Apply these diffs:
Fix manual nonce management:
pub async fn publish_committee_to_registry<P: Provider + WalletProvider + Clone>( provider: EthProvider<P>, contract_address: Address, e3_id: E3id, nodes: OrderedSet<String>, public_key: Vec<u8>, ) -> Result<TransactionReceipt> { let e3_id: U256 = e3_id.try_into()?; let public_key = Bytes::from(public_key); let nodes_vec: Vec<Address> = nodes .into_iter() - .filter_map(|node| node.parse().ok()) + .map(|node| { + node.parse::<Address>() + .map_err(|_| anyhow::anyhow!("Invalid node address: {}", node)) + }) - .collect(); - let from_address = provider.provider().default_signer_address(); - let current_nonce = provider - .provider() - .get_transaction_count(from_address) - .pending() - .await?; + .collect::<Result<Vec<_>>>()?; let contract = ICiphernodeRegistry::new(contract_address, provider.provider()); - let builder = contract + let receipt = contract .publishCommittee(e3_id, nodes_vec, public_key) - .nonce(current_nonce); - let receipt = builder.send().await?.get_receipt().await?; + .send() + .await? + .get_receipt() + .await?; Ok(receipt) }
🧹 Nitpick comments (1)
crates/evm/src/ciphernode_registry_sol.rs (1)
265-298: Document theis_aggregatorparameter.The
is_aggregatorflag controls whether the writer subscribes toPublicKeyAggregatedandCommitteeFinalizeRequestedevents, but there's no documentation explaining when this should betruevsfalse. Adding a doc comment would help callers understand the intended usage pattern.Consider adding:
+ /// Attach a writer to the event bus. + /// + /// # Arguments + /// * `is_aggregator` - Set to `true` for nodes that aggregate and publish public keys, + /// `false` for regular nodes that only submit tickets pub async fn attach( bus: &Addr<EventBus<EnclaveEvent>>, provider: EthProvider<P>, contract_address: &str, is_aggregator: bool, ) -> Result<Addr<CiphernodeRegistrySolWriter<P>>> {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/aggregator/src/committee_finalizer.rs(1 hunks)crates/ciphernode-builder/src/ciphernode_builder.rs(9 hunks)crates/events/src/enclave_event/committee_finalize_requested.rs(1 hunks)crates/events/src/enclave_event/mod.rs(6 hunks)crates/evm/src/ciphernode_registry_sol.rs(5 hunks)crates/evm/src/lib.rs(1 hunks)crates/tests/tests/integration.rs(6 hunks)crates/tests/tests/integration_legacy.rs(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/evm/src/lib.rs
🧰 Additional context used
🧠 Learnings (29)
📚 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/events/src/enclave_event/mod.rscrates/events/src/enclave_event/committee_finalize_requested.rscrates/tests/tests/integration.rscrates/tests/tests/integration_legacy.rscrates/evm/src/ciphernode_registry_sol.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/mod.rscrates/tests/tests/integration.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/tests/tests/integration_legacy.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-10T23:24:43.341Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 143
File: packages/ciphernode/sortition/src/sortition.rs:4-9
Timestamp: 2024-10-10T23:24:43.341Z
Learning: In the `Sortition` module (`packages/ciphernode/sortition/src/sortition.rs`), errors are sent to the event bus using `self.bus.err`, which handles logging and printing. Therefore, explicit use of the `tracing` crate for logging errors may not be necessary in this context.
Applied to files:
crates/events/src/enclave_event/mod.rscrates/tests/tests/integration.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/tests/tests/integration_legacy.rs
📚 Learning: 2024-11-05T06:51:46.701Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/evm/src/event_reader.rs:270-286
Timestamp: 2024-11-05T06:51:46.701Z
Learning: In `packages/ciphernode/evm/src/event_reader.rs`, the `state.ids` HashSet is intended to grow indefinitely to store all processed event IDs for deduplication purposes until an alternative like a bloom filter is implemented.
Applied to files:
crates/events/src/enclave_event/mod.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:
crates/events/src/enclave_event/mod.rscrates/tests/tests/integration.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/aggregator/src/committee_finalizer.rscrates/tests/tests/integration_legacy.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-28T12:00:09.010Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave/src/commands/wallet/set.rs:17-23
Timestamp: 2024-10-28T12:00:09.010Z
Learning: In the `enclave` package of the `ciphernode` project, prefer using `println!` over logging macros like `error!` from the `tracing` crate for error output in CLI commands.
Applied to files:
crates/events/src/enclave_event/mod.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.rscrates/tests/tests/integration_legacy.rs
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/tests/tests/integration.rscrates/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.rscrates/tests/tests/integration_legacy.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:
crates/tests/tests/integration.rscrates/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: tests/basic_integration/test.sh:142-142
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In the `tests/basic_integration/test.sh` script, using fixed sleep durations is acceptable, and adding more complex waiting mechanisms may introduce unnecessary complexity for the test.
Applied to files:
crates/tests/tests/integration.rs
📚 Learning: 2024-11-05T03:56:22.254Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 170
File: packages/ciphernode/evm/tests/evm_reader.rs:63-63
Timestamp: 2024-11-05T03:56:22.254Z
Learning: In the Rust test function `test_logs` in `packages/ciphernode/evm/tests/evm_reader.rs`, a sleep duration of 1 millisecond is sufficient for reliable event processing, even in CI environments.
Applied to files:
crates/tests/tests/integration.rs
📚 Learning: 2024-09-26T04:59:28.043Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: tests/basic_integration/test.sh:142-142
Timestamp: 2024-09-26T04:59:28.043Z
Learning: In the integration test script `tests/basic_integration/test.sh`, using a fixed sleep to wait for the input deadline is acceptable.
Applied to files:
crates/tests/tests/integration.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.rscrates/tests/tests/integration_legacy.rs
📚 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:
crates/tests/tests/integration.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/tests/tests/integration_legacy.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-23T02:03:02.008Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/encryption.rs:45-45
Timestamp: 2024-10-23T02:03:02.008Z
Learning: In the `packages/ciphernode/keyshare/src/encryption.rs` file, the environment variable `CIPHERNODE_SECRET` is used for the encryption password. A secure secret management solution is not currently available, but may be considered in future iterations.
Applied to files:
crates/tests/tests/integration.rscrates/ciphernode-builder/src/ciphernode_builder.rs
📚 Learning: 2024-09-26T16:47:14.932Z
Learnt from: auryn-macmillan
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/core/src/logger.rs:45-46
Timestamp: 2024-09-26T16:47:14.932Z
Learning: The `seed` in the `E3Requested` event is a seed for RNG and not sensitive. Logging it is acceptable.
Applied to files:
crates/tests/tests/integration.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/ciphernode-builder/src/ciphernode_builder.rscrates/evm/src/ciphernode_registry_sol.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/ciphernode-builder/src/ciphernode_builder.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-22T03:30:45.478Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/src/repository.rs:58-60
Timestamp: 2024-10-22T03:30:45.478Z
Learning: In `packages/ciphernode/data/src/repository.rs`, the `write` method in `Repository<T>` must be synchronous and should not return errors.
Applied to files:
crates/ciphernode-builder/src/ciphernode_builder.rs
📚 Learning: 2024-10-29T00:05:52.278Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/config/src/app_config.rs:133-144
Timestamp: 2024-10-29T00:05:52.278Z
Learning: In the `load_config` function in `packages/ciphernode/config/src/app_config.rs`, Figment ensures correct types for the final configuration from defaults, reducing the need for additional validation after loading.
Applied to files:
crates/ciphernode-builder/src/ciphernode_builder.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/ciphernode-builder/src/ciphernode_builder.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-28T10:45:29.100Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/cipher/src/cipher.rs:164-164
Timestamp: 2024-10-28T10:45:29.100Z
Learning: In test code, it's acceptable to use `.unwrap()` instead of propagating errors with the `?` operator in Rust.
Applied to files:
crates/aggregator/src/committee_finalizer.rs
📚 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/aggregator/src/committee_finalizer.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/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-22T03:30:21.818Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/enclave_node/src/shutdown.rs:12-12
Timestamp: 2024-10-22T03:30:21.818Z
Learning: In shutdown code (e.g., in `packages/ciphernode/enclave_node/src/shutdown.rs`), if signal creation fails, it's acceptable to panic rather than handle the error gracefully.
Applied to files:
crates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-22T03:30:03.606Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/enclave_node/src/shutdown.rs:21-26
Timestamp: 2024-10-22T03:30:03.606Z
Learning: In `packages/ciphernode/enclave_node/src/shutdown.rs`, the sleep duration after aborting the task is necessary to wait for other processes to complete during shutdown.
Applied to files:
crates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-12T10:24:07.572Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/evm/src/ciphernode_registry_sol.rs:133-143
Timestamp: 2024-10-12T10:24:07.572Z
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-25T09:56:11.195Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 184
File: packages/ciphernode/net/src/network_relay.rs:0-0
Timestamp: 2024-11-25T09:56:11.195Z
Learning: In `NetworkPeer::new` in `packages/ciphernode/net/src/network_peer.rs`, peers are converted to multiaddrs, and validation of the address format occurs during this conversion. Duplicate addresses are managed as connections are deduplicated.
Applied to files:
crates/evm/src/ciphernode_registry_sol.rs
🧬 Code graph analysis (6)
crates/events/src/enclave_event/committee_finalize_requested.rs (1)
crates/events/src/enclave_event/mod.rs (1)
fmt(382-384)
crates/tests/tests/integration.rs (3)
crates/tests/tests/integration_legacy.rs (1)
setup_score_sortition_environment(118-153)crates/events/src/enclave_event/mod.rs (3)
from(240-269)try_from(365-367)try_from(372-378)crates/ciphernode-builder/src/ciphernode_builder.rs (2)
new(89-109)new(485-491)
crates/ciphernode-builder/src/ciphernode_builder.rs (7)
crates/sortition/src/backends.rs (1)
score(252-254)crates/sortition/src/repo.rs (2)
sortition(15-15)sortition(19-21)crates/evm/src/ciphernode_registry_sol.rs (4)
attach(217-242)attach(265-298)attach(510-531)attach_writer(533-546)crates/sortition/src/ciphernode_selector.rs (1)
attach(46-62)crates/sortition/src/sortition.rs (1)
attach(168-210)crates/evm/src/bonding_registry_sol.rs (2)
attach(144-169)attach(176-197)crates/ciphernode-builder/src/ciphernode.rs (1)
store(53-55)
crates/aggregator/src/committee_finalizer.rs (3)
crates/events/src/eventbus.rs (1)
error(92-96)crates/evm/src/helpers.rs (3)
new(69-75)new(111-113)get_current_timestamp(200-220)crates/events/src/enclave_event/mod.rs (1)
from(240-269)
crates/tests/tests/integration_legacy.rs (3)
crates/tests/tests/integration.rs (1)
setup_score_sortition_environment(35-70)crates/events/src/enclave_event/mod.rs (3)
from(240-269)try_from(365-367)try_from(372-378)crates/test-helpers/src/lib.rs (2)
new(167-173)get_common_setup(71-99)
crates/evm/src/ciphernode_registry_sol.rs (4)
crates/events/src/enclave_event/mod.rs (1)
from(240-269)crates/evm/src/bonding_registry_sol.rs (8)
from(30-38)from(42-45)from(51-68)from(77-83)from(87-90)from(94-97)attach(144-169)attach(176-197)crates/evm/src/helpers.rs (4)
new(69-75)new(111-113)chain_id(81-83)provider(77-79)crates/evm/src/enclave_sol_writer.rs (2)
new(39-49)attach(51-65)
⏰ 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: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: build_e3_support_dev
- GitHub Check: build_sdk
- GitHub Check: rust_integration
- GitHub Check: build_e3_support_risc0
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (6)
crates/ciphernode-builder/src/ciphernode_builder.rs (3)
207-211: LGTM: Score-based sortition builder method.The implementation is clean and follows the builder pattern correctly. Good practice to provide an explicit configuration method even though score is the default.
289-301: LGTM: Repository-based sortition setup.The sortition initialization correctly uses the new repository-based approach with
node_state,finalized_committees, and the configured backend. TheCiphernodeSelectorattachment also correctly includes the store reference.
367-391: LGTM: Robust writer attachment with proper error handling.The error handling now captures and logs error details instead of silently swallowing them, addressing the previous review concern. The conditional attachment of
CommitteeFinalizerfor score sortition with public key aggregation is appropriately logged and aligns with the score-based committee finalization flow described in the PR objectives.crates/evm/src/ciphernode_registry_sol.rs (3)
89-154: LGTM! Event wrappers follow established patterns.The new event wrapper structs (CommitteeRequestedWithChainId, CommitteeFinalizedWithChainId, TicketSubmittedWithChainId) and their From implementations correctly translate Solidity events to Rust event types with chain-id context, consistent with the existing CiphernodeAdded and CiphernodeRemoved wrappers.
176-202: LGTM! Event extractor properly handles new event types.The additions to the extractor function correctly decode CommitteeRequested, CommitteeFinalized, and TicketSubmitted events, with appropriate error logging and chain-id wrapping consistent with the existing pattern.
335-371: The review comment is incorrect.The
TicketIdenum has only one variant:Score(u64). Since this is the only variant, the match statement is exhaustive by design—there are no other variants to handle. The Rust compiler guarantees this is safe, and no wildcard arm is necessary.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (1)
21-22: Both contracts share the same address.Lines 21-22 configure
enclaveandciphernodeRegistrywith identical addresses. Even in example code, these should be distinct contract instances. This configuration is logically incorrect and may mislead developers.Consider using distinct placeholder addresses:
contracts: { - enclave: '0xc6e7DF5E7b4f2A278906862b61205850344D4e7d', - ciphernodeRegistry: '0xc6e7DF5E7b4f2A278906862b61205850344D4e7d', + enclave: '0x5FbDB2315678afecb367f032d93F642f64180aa3', + ciphernodeRegistry: '0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512', },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/client/libs/wasm/pkg/crisp_worker.js
📚 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:
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js
🧬 Code graph analysis (1)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (1)
packages/enclave-sdk/src/index.ts (2)
EnclaveSDK(8-8)FheProtocol(47-47)
🪛 Gitleaks (8.28.0)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js
[high] 27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (4)
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: build_e3_support_risc0
- GitHub Check: Build & Push Image
🔇 Additional comments (4)
examples/CRISP/client/libs/wasm/pkg/crisp_worker.js (4)
7-8: LGTM! Clean import simplification.The import statements are properly formatted and the simplified circuit import path improves readability.
10-12: LGTM! Clear message handling pattern.The destructuring and switch-based routing is clean and maintainable.
31-31: LGTM! Encryption method call is correct.The call to
encryptNumberAndGenProofwith the updated parameters aligns with the new SDK API.
33-43: LGTM! Result handling is well-structured.The post-message payload correctly handles both success and error cases with clear field naming.
5 participants
The Enclave system uses a score-based sortition mechanism to select a committee of ciphernodes to perform threshold homomorphic encryption operations. The flow involves:
Complete System Flow
sequenceDiagram participant Operator participant BondingRegistry participant CiphernodeRegistry participant EventBus participant NodeStateManager participant Sortition participant CiphernodeSelector participant Keyshare participant PublicKeyAggregator participant PlaintextAggregator Note over Operator,BondingRegistry: Phase 1: Operator Setup & Registration Operator->>BondingRegistry: bondLicense(amount) BondingRegistry->>BondingRegistry: Transfer ENCL tokens BondingRegistry->>EventBus: LicenseBondUpdated Operator->>BondingRegistry: registerOperator() BondingRegistry->>BondingRegistry: Check isLicensed() BondingRegistry->>CiphernodeRegistry: addCiphernode(operator) CiphernodeRegistry->>EventBus: CiphernodeAdded(operator, index, numNodes, chainId) EventBus->>NodeStateManager: CiphernodeAdded NodeStateManager->>NodeStateManager: Register operator in nodes HashMap Operator->>BondingRegistry: addTicketBalance(amount) BondingRegistry->>BondingRegistry: Mint ticket tokens BondingRegistry->>EventBus: TicketBalanceUpdated(operator, delta, newBalance, chainId) EventBus->>NodeStateManager: TicketBalanceUpdated NodeStateManager->>NodeStateManager: Update operator ticket balance BondingRegistry->>BondingRegistry: Check if balance >= minTicketBalance BondingRegistry->>EventBus: OperatorActivationChanged(operator, active=true, chainId) EventBus->>NodeStateManager: OperatorActivationChanged NodeStateManager->>NodeStateManager: Set operator active status Note over Operator,PlaintextAggregator: Phase 2: E3 Request & Sortition Operator->>EventBus: E3Requested(e3Id, thresholdM, thresholdN, seed, params, chainId) EventBus->>Sortition: E3Requested Sortition->>NodeStateManager: GetNodeState(chainId) NodeStateManager-->>Sortition: NodeStateStore { nodes, ticketPrice } Sortition->>Sortition: Build sortition list from active nodes Sortition->>Sortition: Run score sortition algorithm Sortition->>Sortition: Generate tickets for selected nodes loop For each selected node Sortition->>EventBus: TicketGenerated(e3Id, node, ticketId, chainId) end Note over CiphernodeRegistry,EventBus: Phase 3: On-Chain Ticket Submission EventBus->>CiphernodeRegistry: TicketGenerated (if ticketId != 0) CiphernodeRegistry->>CiphernodeRegistry: Submit ticket to contract CiphernodeRegistry->>CiphernodeRegistry: Wait for threshold tickets CiphernodeRegistry->>CiphernodeRegistry: Call finalizeCommittee() CiphernodeRegistry->>EventBus: CommitteeFinalized(e3Id, committee[], chainId) Note over EventBus,Sortition: Phase 4: Committee Storage EventBus->>Sortition: CommitteeFinalized Sortition->>Sortition: Store committee in finalized_committees HashMap Sortition->>Sortition: Persist to disk Note over EventBus,CiphernodeSelector: Phase 5: Node Selection EventBus->>CiphernodeSelector: CommitteeFinalized CiphernodeSelector->>CiphernodeSelector: Check if node address in committee alt Node is in committee CiphernodeSelector->>EventBus: CiphernodeSelected(e3Id, node, chainId) end Note over EventBus,Keyshare: Phase 6: Keyshare Generation EventBus->>Keyshare: CiphernodeSelected Keyshare->>Keyshare: fhe.generate_keyshare() Keyshare->>Keyshare: Generate secret key (random) Keyshare->>Keyshare: Generate public key share (sk + CRP) Keyshare->>Keyshare: Persist secret key Keyshare->>EventBus: KeyshareCreated(e3Id, node, pubkey, chainId) Note over EventBus,PublicKeyAggregator: Phase 7: Public Key Aggregation EventBus->>PublicKeyAggregator: KeyshareCreated PublicKeyAggregator->>Sortition: GetNodesForE3(e3Id, chainId) Sortition-->>PublicKeyAggregator: committee[] PublicKeyAggregator->>PublicKeyAggregator: Verify node in committee PublicKeyAggregator->>PublicKeyAggregator: Add keyshare to OrderedSet alt Threshold reached (all keyshares collected) PublicKeyAggregator->>PublicKeyAggregator: fhe.get_aggregate_public_key(keyshares) PublicKeyAggregator->>PublicKeyAggregator: Aggregate public key shares PublicKeyAggregator->>EventBus: PublicKeyAggregated(e3Id, pubkey, nodes, chainId) PublicKeyAggregator->>CiphernodeRegistry: publishPublicKey(e3Id, pubkey, nodes) end Note over Operator,PlaintextAggregator: Phase 8: Encryption & Computation Operator->>Operator: Encrypt input data with aggregated pubkey Operator->>EventBus: CiphertextOutputPublished(e3Id, ciphertext, chainId) Note over EventBus,PlaintextAggregator: Phase 9: Threshold Decryption EventBus->>Keyshare: CiphertextOutputPublished Keyshare->>Keyshare: Load secret key from storage Keyshare->>Keyshare: fhe.decrypt_ciphertext(secret, ciphertext) Keyshare->>Keyshare: Generate decryption share Keyshare->>EventBus: DecryptionshareCreated(e3Id, node, decryptionShare, chainId) EventBus->>PlaintextAggregator: DecryptionshareCreated PlaintextAggregator->>Sortition: GetNodesForE3(e3Id, chainId) Sortition-->>PlaintextAggregator: committee[] PlaintextAggregator->>PlaintextAggregator: Verify node in committee PlaintextAggregator->>PlaintextAggregator: Add decryption share to OrderedSet alt Threshold reached (all shares collected) PlaintextAggregator->>PlaintextAggregator: fhe.get_aggregate_plaintext(shares, ciphertext) PlaintextAggregator->>PlaintextAggregator: Aggregate decryption shares PlaintextAggregator->>PlaintextAggregator: Decode plaintext PlaintextAggregator->>EventBus: PlaintextAggregated(e3Id, plaintext, nodes, chainId) endState Diagram: Node Lifecycle
stateDiagram-v2 [*] --> Unbonded Unbonded --> Licensed: bondLicense(amount >= requiredBond) Licensed --> Registered: registerOperator() Registered --> Active: addTicketBalance(balance >= minBalance) Active --> Inactive: removeTicketBalance() OR unbondLicense() Inactive --> Active: addTicketBalance() OR bondLicense() Active --> ExitPending: deregisterOperator() Inactive --> ExitPending: deregisterOperator() Registered --> ExitPending: deregisterOperator() ExitPending --> [*]: claimExits() after exitDelay ExitPending --> Registered: registerOperator() (cancels exit)Sortition Data Flow
flowchart TD A[E3Requested Event] --> B{Chain ID Match?} B -->|No| Z[Ignore] B -->|Yes| C[NodeStateManager: Get Active Nodes] C --> D[Filter: ticket_balance > 0 AND active=true] D --> E[Score Sortition: Build Weighted List] E --> F[Calculate Total Ticket Weight] F --> G{threshold_n nodes available?} G -->|No| H[Error: Insufficient Nodes] G -->|Yes| I[Select Top N Nodes by Weight] I --> J[Generate Ticket IDs] J --> K[Emit TicketGenerated Events] K --> L[EVM: Submit Tickets On-Chain] L --> M{Threshold Tickets Submitted?} M -->|No| L M -->|Yes| N[Contract: finalizeCommittee] N --> O[Emit CommitteeFinalized Event] O --> P[Sortition: Store Committee] P --> Q[CiphernodeSelector: Check Membership] Q --> R[Emit CiphernodeSelected] R --> S[Keyshare Generation Starts]Committee Finalization Flow
flowchart LR A[TicketGenerated] --> B{ticket_id == 0?} B -->|Yes| C[Distance Sortition - Skip Contract] B -->|No| D[Score Sortition - Submit to Contract] D --> E[Contract: Collect Tickets] E --> F{Threshold Met?} F -->|No| E F -->|Yes| G[Contract: finalizeCommittee] G --> H[Freeze Committee List] H --> I[Emit CommitteeFinalized Event] C --> J[Manual CommitteeFinalized Emission] I --> K[All Nodes: Store Committee] J --> K K --> L[CiphernodeSelector: Process]Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements
Configuration Changes