feat: fault attribution slashing contracts [skip-line-limit]#1354
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a two‑lane SlashingManager and committee expulsion features, per‑E3 fee token routing and refund/accounting changes, extensive ABI/artifact updates, deployment/test rewiring, and several small React lint/memo refactors in CRISP examples. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Proposer/User
participant SlashingMgr as SlashingManager
participant CiphernodeReg as CiphernodeRegistry
participant BondingReg as BondingRegistry
participant Enclave as Enclave
rect rgba(100, 150, 200, 0.5)
Note over User,SlashingMgr: Lane A — Proof‑based (atomic)
User->>SlashingMgr: proposeSlash(e3Id, operator, reason, proof)
SlashingMgr->>CiphernodeReg: isCommitteeMemberActive(e3Id, operator)
CiphernodeReg-->>SlashingMgr: active?
SlashingMgr->>SlashingMgr: _verifyProofEvidence(proof)
SlashingMgr->>SlashingMgr: _executeSlash(proposalId)
SlashingMgr->>BondingReg: penalizeTicket(operator) / transfer penalties
SlashingMgr->>CiphernodeReg: expelCommitteeMember(e3Id, operator, reason)
CiphernodeReg-->>SlashingMgr: (activeCount, threshold)
alt activeCount < threshold
SlashingMgr->>Enclave: onE3Failed(e3Id, reason)
end
SlashingMgr-->>User: SlashExecuted event
end
sequenceDiagram
participant RewardDist as Reward Distributor
participant Enclave as Enclave
participant CiphernodeReg as CiphernodeRegistry
participant BondingReg as BondingRegistry
rect rgba(150, 200, 100, 0.5)
Note over RewardDist,Enclave: Rewards distribution using active committee and per‑E3 token
RewardDist->>Enclave: completeE3(e3Id)
Enclave->>CiphernodeReg: getActiveCommitteeNodes(e3Id)
CiphernodeReg-->>Enclave: activeNodes[]
alt no activeNodes
Enclave->>RewardDist: refund totalAmount to requester via feeToken
else
Enclave->>BondingReg: distributeRewards(activeNodes, amounts, paymentToken)
BondingReg->>BondingReg: transfer from msg.sender across recipients
end
Enclave-->>RewardDist: RewardsDistributed event
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
hmzakhalid
changed the title
hmzakhalid
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/enclave-contracts/contracts/token/EnclaveToken.sol (1)
157-168:⚠️ Potential issue | 🟡 Minor
batchMintAllocationszero-address guard is correct and consistent withmintAllocation.The placement (before amount/supply checks) matches the single-mint sibling, and the supply arithmetic (
MAX_SUPPLY - minted) remains safe under Solidity 0.8 checked arithmetic given thetotalMinted ≤ MAX_SUPPLYinvariant.However, the
@devNatSpec on line 139 still only documents two revert conditions and should be updated to mention the new one:📝 Proposed NatSpec update
- * Reverts if any amount is zero or if the cumulative minting would exceed MAX_SUPPLY. + * Reverts if any recipient is the zero address, if any amount is zero, or if the + * cumulative minting would exceed MAX_SUPPLY.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/token/EnclaveToken.sol` around lines 157 - 168, Update the `@dev` NatSpec for batchMintAllocations to document the additional revert condition for zero-address inputs: note that the function will revert with ZeroAddress() if any recipient in the recipients array is the zero address, in addition to the existing ZeroAmount() and ExceedsTotalSupply() reverts; mirror the wording used for mintAllocation to keep consistency with the single-mint sibling and reference the same error names (ZeroAddress, ZeroAmount, ExceedsTotalSupply) and their conditions.packages/enclave-contracts/.gitignore (1)
35-46:⚠️ Potential issue | 🟡 MinorDuplicate "Verifier contracts" section introduced — remove the stale copy.
Lines 35–39 (new) exactly duplicate lines 42–46 (pre-existing), including all four
!/artifacts/contracts/verifier/…rules and the section header.🔧 Suggested fix — remove the newly added duplicate block
-# Verifier contracts -!/artifacts/contracts/verifier/ -!/artifacts/contracts/verifier/DkgPkVerifier.sol/ -!/artifacts/contracts/verifier/DkgPkVerifier.sol/DkgPkVerifier.json -!/artifacts/contracts/verifier/DkgPkVerifier.sol/ZKTranscriptLib.json - - # Verifier contracts !/artifacts/contracts/verifier/ !/artifacts/contracts/verifier/DkgPkVerifier.sol/ !/artifacts/contracts/verifier/DkgPkVerifier.sol/DkgPkVerifier.json !/artifacts/contracts/verifier/DkgPkVerifier.sol/ZKTranscriptLib.jsonThe same pattern repeats for the
# Base Dirs(lines 6 vs 8) and# Interfaces(lines 14 vs 16) comment headers, which are also duplicated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/.gitignore` around lines 35 - 46, Remove the duplicate comment blocks and rules that were accidentally introduced: delete the newly added duplicate "# Verifier contracts" header and its four rules matching "!/artifacts/contracts/verifier/", "!/artifacts/contracts/verifier/DkgPkVerifier.sol/", "!/artifacts/contracts/verifier/DkgPkVerifier.sol/DkgPkVerifier.json", and "!/artifacts/contracts/verifier/DkgPkVerifier.sol/ZKTranscriptLib.json", and do the same for the duplicated "# Base Dirs" and "# Interfaces" comment headers so only one copy of each section remains (keep the original existing entries and remove the stale duplicates).packages/enclave-contracts/contracts/interfaces/ISlashingManager.sol (1)
66-87:⚠️ Potential issue | 🔴 CriticalABI artifact is missing snapshotted fields from
SlashProposalstruct.The
SlashProposalstruct definition includes 18 fields (banNode,affectsCommittee, andfailureReasonat lines 82-86), but the compiled artifactISlashingManager.jsononly includes 14 fields ending atproofVerified— the three snapshotted fields are absent from thegetSlashProposaloutput ABI.This will cause ABI encoding/decoding failures when clients or tests attempt to interact with proposals. Please regenerate the artifact to include the complete struct definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/interfaces/ISlashingManager.sol` around lines 66 - 87, The compiled ABI artifact ISlashingManager.json is missing the three snapshotted fields (banNode, affectsCommittee, failureReason) from the SlashProposal struct which causes encoding/decoding mismatches; regenerate the contract artifacts so the ABI for SlashProposal (used by getSlashProposal and any callers) includes all 18 fields as defined in the ISlashingManager.sol struct, then recompile/deploy or update the published JSON artifact so clients/tests consume the corrected ABI.packages/enclave-contracts/contracts/E3RefundManager.sol (1)
315-334:⚠️ Potential issue | 🟡 MinorToken transfer mechanism must be established before
routeSlashedFundsis integrated into the call chain.This function increases
requesterAmountandhonestNodeAmountwithout receiving any tokens. When integrated (currently not called in production), the calling code must transfer the correspondingamountof tokens to this contract before invokingrouteSlashedFunds, otherwise subsequent claims will revert with insufficient balance. Ensure the SlashingManager integration path or calling mechanism includes this token transfer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/E3RefundManager.sol` around lines 315 - 334, The routeSlashedFunds logic updates RefundDistribution (_distributions[e3Id]) balances (requesterAmount, honestNodeAmount, totalSlashed) without receiving tokens, so ensure the calling path or SlashingManager transfers the ERC20 tokens into this contract before calling routeSlashedFunds, or update routeSlashedFunds to atomically pull tokens: accept the token IERC20 address (or use the contract's configured token), call transferFrom(msg.sender, address(this), amount) and require its success prior to updating the RefundDistribution and emitting SlashedFundsRouted; reference the routeSlashedFunds function, RefundDistribution struct, _distributions mapping and SlashedFundsRouted event when making the change so claims won't later revert for insufficient balance.examples/CRISP/packages/crisp-contracts/deployed_contracts.json (1)
30-37:⚠️ Potential issue | 🟡 MinorSepolia
SlashingManagerconstructor args are stale — missingciphernodeRegistryandenclave.The sepolia entry still reflects the old constructor signature (only
admin+bondingRegistry), while the localhost entry correctly includes the newciphernodeRegistryandenclaveparams. This will need updating when sepolia is redeployed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/packages/crisp-contracts/deployed_contracts.json` around lines 30 - 37, Update the SlashingManager sepolia entry so its constructorArgs match the new constructor signature by adding the ciphernodeRegistry and enclave fields (same shape as the localhost entry); modify the "SlashingManager" object's constructorArgs to include "ciphernodeRegistry": "<address>" and "enclave": "<address>" (replace with the actual deployed addresses or placeholders until sepolia is redeployed) so the JSON reflects the current constructor parameters.
♻️ Duplicate comments (1)
packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json (1)
524-602:⚠️ Potential issue | 🔴 Critical
SlashProposalin artifact is missing snapshotted fields — artifact needs regeneration.The
getSlashProposaloutput struct (lines 524-594) includes fields only throughproofVerified. The interface'sSlashProposalstruct (inISlashingManager.sollines 82-86) also definesbanNode,affectsCommittee, andfailureReason, which are absent here.Additionally, the
ChainIdMismatcherror defined in the interface (line 163 ofISlashingManager.sol) appears to be missing from the artifact's error list.This artifact appears to be stale — please regenerate it from the current Solidity sources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json` around lines 524 - 602, The artifact for ISlashingManager is stale: the ABI for the getSlashProposal response (SlashProposal) is missing the snapshotted fields banNode, affectsCommittee, and failureReason and the ChainIdMismatch error is absent; regenerate the contract artifacts from the updated Solidity source (ISlashingManager.sol) so the ABI/JSON includes the full struct (SlashProposal) with banNode, affectsCommittee, failureReason and the ChainIdMismatch error entry, or if regenerating is not possible, update the ABI manually to add those fields to the getSlashProposal tuple and add the ChainIdMismatch error definition to the errors array.
🧹 Nitpick comments (10)
examples/CRISP/client/src/context/voteManagement/VoteManagement.context.tsx (1)
242-243: LGTM — targeted suppression is appropriate here.
createGenericContextreturns both a hook (useVoteManagementContext) and a provider component in a tuple, so the mixed export is structural and intentional. Thedisable-next-linescope keeps the suppression minimal rather than silencing the rule file-wide.The ideal long-term fix is to re-export
useVoteManagementContextthrough a thin barrel/index file so this file only exports the component, eliminating the need for the suppression entirely — but that's a non-trivial refactor for a separate PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/client/src/context/voteManagement/VoteManagement.context.tsx` around lines 242 - 243, The current targeted ESLint suppression before exporting both useVoteManagementContext and VoteManagementProvider is acceptable short-term; to remove the suppression, refactor so this file only exports VoteManagementProvider and move the hook export into a thin barrel (index) module: keep createGenericContext usage here to produce { useVoteManagementContext, VoteManagementProvider } but export only VoteManagementProvider from this file, then create/update a separate barrel file that imports useVoteManagementContext from this module and re-exports it (export { useVoteManagementContext } from './VoteManagement.context'), allowing you to remove the eslint-disable-next-line comment and keep exports structurally separated.examples/CRISP/client/src/components/CircularTiles.tsx (1)
15-17: Optional: preserve existing rotations whencountincreases.When
countgrows, the current implementation regenerates all slots (including already-stable tiles). For the common case of a fixedcountthis is a no-op, but ifcountever changes dynamically the tiles will flash with new random rotations.♻️ Incremental rotation update
- useEffect(() => { - setRotations(generateRotations(count)) - }, [count]) + useEffect(() => { + setRotations((prev) => { + if (count === prev.length) return prev + if (count > prev.length) return [...prev, ...generateRotations(count - prev.length)] + return prev.slice(0, count) + }) + }, [count])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/client/src/components/CircularTiles.tsx` around lines 15 - 17, The useEffect in the CircularTiles component currently calls setRotations(generateRotations(count)) unconditionally, which regenerates all rotations when count changes; instead, update rotations incrementally by using the state setter with the previous rotations: if count increases, keep prev rotations and append only generateRotations(newCount - prev.length) for the new slots; if count decreases, slice prev to the new length; if equal, return prev; reference setRotations, generateRotations, and the useEffect that depends on count when making this change.packages/enclave-contracts/contracts/token/EnclaveTicketToken.sol (2)
199-201:approveoverride blocks direct calls, butERC20Permit.permitstill routes through_approveinternally.The override is correct as a defence-in-depth measure, and there is no exploit (all
transferFromcalls revert via_update). Consider adding a NatSpec note to bothapproveandpermitso readers understand that permit-created allowances are unreachable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/token/EnclaveTicketToken.sol` around lines 199 - 201, The approve override currently reverts via TransferNotAllowed() which blocks direct approvals but ERC20Permit.permit still calls _approve internally, creating a potential confusion; add NatSpec comments to the approve function and the permit function explaining that approvals are intentionally disabled, that permit may appear to set allowances but those allowances are unreachable because transferFrom-related logic is prevented by _update, and reference TransferNotAllowed, _approve, and _update so readers understand this defense-in-depth behavior.
191-192: Use a custom error inpayoutinstead of a string revert message for consistency.All other guards in this contract use typed custom errors; this one-off string literal incurs extra gas and breaks the pattern.
🔧 Suggested fix
Add a new custom error (alongside
NotRegistry,TransferNotAllowed, etc.):+ /// `@notice` Thrown when payout amount exceeds the slashed payable balance + error ExceedsPayableBalance();Then use it in
payout:- require(amount <= payableBalance, "Exceeds payable balance"); + if (amount > payableBalance) revert ExceedsPayableBalance();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/token/EnclaveTicketToken.sol` around lines 191 - 192, Add a typed custom error (e.g., ExceedsPayableBalance() or InsufficientPayableBalance(uint256 requested, uint256 available)) alongside the other contract errors (like NotRegistry, TransferNotAllowed) and replace the require/string in the payout function (the require(amount <= payableBalance, "Exceeds payable balance");) with a revert using that custom error (providing any chosen params if you define them) and then decrement payableBalance as before.templates/default/enclave.config.yaml (1)
17-19: Nit: inconsistent quote style forslashing_manageraddress.All other contract entries use double quotes;
slashing_manager.addressuses single quotes.🔧 Suggested fix
- address: '0x0165878A594ca255338adfa4d48449f69242Eb8F' + address: "0x0165878A594ca255338adfa4d48449f69242Eb8F"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/enclave.config.yaml` around lines 17 - 19, The slashing_manager.address entry uses single quotes while other contract addresses use double quotes; update the slashing_manager.address value to use double quotes (change '0x0165878A594ca255338adfa4d48449f69242Eb8F' to "0x0165878A594ca255338adfa4d48449f69242Eb8F") so quote style is consistent across entries like slashing_manager.address and the other contract address fields.packages/enclave-contracts/contracts/interfaces/IEnclave.sol (1)
317-320: Considercalldatainstead ofmemoryfor the array parameter.
removeE3ProgramsParamsusesbytes[] memoryon anexternalfunction, which copies the array into memory unnecessarily.calldatais cheaper for external calls. This matches the pre-existing pattern insetE3ProgramsParams(Line 315), so consider fixing both at the same time for consistency.🛠️ Proposed fix
- function removeE3ProgramsParams(bytes[] memory _e3ProgramsParams) external; + function removeE3ProgramsParams(bytes[] calldata _e3ProgramsParams) external;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/interfaces/IEnclave.sol` around lines 317 - 320, The function declaration for removeE3ProgramsParams uses bytes[] memory in an external interface which causes unnecessary copying; change its parameter to bytes[] calldata to match the cheaper external calldata convention and align with the existing setE3ProgramsParams pattern — update the function signature for removeE3ProgramsParams (and, if any other external functions like setE3ProgramsParams are inconsistent, make them bytes[] calldata as well) so the external parameter uses calldata instead of memory.packages/enclave-contracts/contracts/Enclave.sol (2)
697-708:setSlashingManagermissing same-value guard, unlike sibling setters.
setCiphernodeRegistryandsetBondingRegistryboth reject calls where the new value equals the current value. This setter only checks foraddress(0).♻️ Proposed fix for consistency
function setSlashingManager( ISlashingManager _slashingManager ) public onlyOwner { require( - address(_slashingManager) != address(0), + address(_slashingManager) != address(0) && + _slashingManager != slashingManager, "Invalid SlashingManager address" ); slashingManager = _slashingManager; emit SlashingManagerSet(address(_slashingManager)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/Enclave.sol` around lines 697 - 708, The setSlashingManager function lacks the same-value guard present in sibling setters; update setSlashingManager (and its use of slashingManager and event SlashingManagerSet) to require that address(_slashingManager) != address(slashingManager) in addition to the non-zero check, so calls that try to set the same contract are rejected consistently.
786-789: Use a sentinel enum value or helper for upper-bound validation on FailureReason.The hardcoded check
reason <= uint8(FailureReason.VerificationFailed)couples validation to the current enum layout. If future failure reasons are appended afterVerificationFailed, they will be silently rejected without compile-time warning. Define a sentinelMAXenum value or extract validation to a helper function to make the bound automatically propagate with enum changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/Enclave.sol` around lines 786 - 789, The validation currently uses a hardcoded upper bound against uint8(FailureReason.VerificationFailed); update the FailureReason enum to include a sentinel like FailureReason.MAX (or LAST) as the final value, or add a helper function isValidFailureReason(uint8 reason) that checks reason > 0 && reason <= uint8(FailureReason.MAX), then replace the require(...) with a call to that helper (or compare against FailureReason.MAX) so future enum additions automatically pass validation; update references to FailureReason.VerificationFailed in this require check accordingly.packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
441-449: Optional: emit an event when updating the slashing manager.
This improves auditability and mirrors the existing Enclave/BondingRegistry setters.♻️ Suggested event emission
+ event SlashingManagerSet(address indexed slashingManager); ... function setSlashingManager( ISlashingManager _slashingManager ) public onlyOwner { require(address(_slashingManager) != address(0), ZeroAddress()); slashingManager = _slashingManager; + emit SlashingManagerSet(address(_slashingManager)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol` around lines 441 - 449, Add an event emission to setSlashingManager to improve auditability: declare a descriptive event (e.g., SlashingManagerUpdated(address indexed previous, address indexed current)) and emit it inside the setSlashingManager function after the require and assignment (reference function setSlashingManager and state variable slashingManager) so callers can track old and new slashing manager addresses; ensure event is added to the contract's events section and use address(slashingManager) for the previous value before overwriting.packages/enclave-contracts/contracts/registry/BondingRegistry.sol (1)
73-77: Consider zero‑address guard + event for distributor allowlist changes.
Admin allowlist mutations are security‑relevant; an event improves auditability and a zero‑address guard prevents accidental misconfiguration.♻️ Suggested events + zero‑address guard
+ event RewardDistributorAuthorizationUpdated( + address indexed distributor, + bool authorized + ); ... function setRewardDistributor( address newRewardDistributor ) public onlyOwner { - authorizedDistributors[newRewardDistributor] = true; + require(newRewardDistributor != address(0), ZeroAddress()); + authorizedDistributors[newRewardDistributor] = true; + emit RewardDistributorAuthorizationUpdated(newRewardDistributor, true); } ... function revokeRewardDistributor(address distributor) public onlyOwner { - authorizedDistributors[distributor] = false; + require(distributor != address(0), ZeroAddress()); + authorizedDistributors[distributor] = false; + emit RewardDistributorAuthorizationUpdated(distributor, false); }Also applies to: 696-709
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/registry/BondingRegistry.sol` around lines 73 - 77, Add an on‑chain event and a zero‑address check for all mutations of the authorizedDistributors allowlist: declare an event like DistributorAuthorizationChanged(address indexed distributor, bool authorized) and emit it whenever the mapping authorizedDistributors is updated; in each mutating function that touches this mapping (e.g., any authorizeDistributor/approveDistributor and revokeDistributor/removeAuthorizedDistributor functions that update authorizedDistributors) add require(distributor != address(0), "zero address not allowed") before state changes and then emit DistributorAuthorizationChanged(distributor, true/false). Apply the same pattern to the other analogous allowlist(s) referenced around lines 696–709 so all admin allowlist additions/removals have the zero‑address guard and emitted event for auditability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/contracts/interfaces/ICircuitVerifier.sol`:
- Around line 19-22: The interface ICircuitVerifier.sol declares function
verify(bytes calldata _proof, bytes32[] calldata _publicInputs) external returns
(bool) without the view modifier; update the declaration to be a read-only
function (external view returns (bool)) so callers can use STATICCALL and match
concrete implementations like DkgPkVerifier and MockCircuitVerifier—locate the
verify signature in ICircuitVerifier and add the view modifier to the function
declaration.
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 193-204: The deploy script never wires the SlashingManager into
the Enclave, so Enclave.onE3Failed (protected by
onlyCiphernodeRegistryOrSlashingManager) will fail because slashingManager is
still address(0); fix by adding a call to
enclave.setSlashingManager(slashingManagerAddress) in the deployment sequence
(after slashingManager and enclave are deployed and addresses available) so the
Enclave.slashingManager state is set and SlashingManager can call onE3Failed.
In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts`:
- Around line 75-79: The defaultTimeoutConfig object is missing
committeeFormationWindow which causes it to default to zero and trigger
immediate timeouts; update defaultTimeoutConfig to include a
committeeFormationWindow (e.g., set to ONE_DAY or the same value used elsewhere)
so tests use a nonzero formation timeout—modify the defaultTimeoutConfig
constant to add committeeFormationWindow: ONE_DAY (or the appropriate constant)
alongside dkgWindow, computeWindow, and decryptionWindow.
In `@templates/default/client/src/context/WizardContext.tsx`:
- Around line 110-116: When connection is lost the effect resets currentStep but
leaves stale e3State; update the effect in WizardContext.tsx so that when
isConnected is false you also clear e3State (e.g. call setE3State to its initial
empty/null fields) or, alternatively, when advancing to REQUEST_COMPUTATION
check that e3State.id is null before calling setCurrentStep; modify the
useEffect that references isConnected, sdk.isInitialized, currentStep to perform
one of these fixes (use setE3State in the disconnect branch or add a guard on
e3State.id in the reconnect branch) to prevent showing prior session E3 data.
---
Outside diff comments:
In `@examples/CRISP/packages/crisp-contracts/deployed_contracts.json`:
- Around line 30-37: Update the SlashingManager sepolia entry so its
constructorArgs match the new constructor signature by adding the
ciphernodeRegistry and enclave fields (same shape as the localhost entry);
modify the "SlashingManager" object's constructorArgs to include
"ciphernodeRegistry": "<address>" and "enclave": "<address>" (replace with the
actual deployed addresses or placeholders until sepolia is redeployed) so the
JSON reflects the current constructor parameters.
In `@packages/enclave-contracts/.gitignore`:
- Around line 35-46: Remove the duplicate comment blocks and rules that were
accidentally introduced: delete the newly added duplicate "# Verifier contracts"
header and its four rules matching "!/artifacts/contracts/verifier/",
"!/artifacts/contracts/verifier/DkgPkVerifier.sol/",
"!/artifacts/contracts/verifier/DkgPkVerifier.sol/DkgPkVerifier.json", and
"!/artifacts/contracts/verifier/DkgPkVerifier.sol/ZKTranscriptLib.json", and do
the same for the duplicated "# Base Dirs" and "# Interfaces" comment headers so
only one copy of each section remains (keep the original existing entries and
remove the stale duplicates).
In `@packages/enclave-contracts/contracts/E3RefundManager.sol`:
- Around line 315-334: The routeSlashedFunds logic updates RefundDistribution
(_distributions[e3Id]) balances (requesterAmount, honestNodeAmount,
totalSlashed) without receiving tokens, so ensure the calling path or
SlashingManager transfers the ERC20 tokens into this contract before calling
routeSlashedFunds, or update routeSlashedFunds to atomically pull tokens: accept
the token IERC20 address (or use the contract's configured token), call
transferFrom(msg.sender, address(this), amount) and require its success prior to
updating the RefundDistribution and emitting SlashedFundsRouted; reference the
routeSlashedFunds function, RefundDistribution struct, _distributions mapping
and SlashedFundsRouted event when making the change so claims won't later revert
for insufficient balance.
In `@packages/enclave-contracts/contracts/interfaces/ISlashingManager.sol`:
- Around line 66-87: The compiled ABI artifact ISlashingManager.json is missing
the three snapshotted fields (banNode, affectsCommittee, failureReason) from the
SlashProposal struct which causes encoding/decoding mismatches; regenerate the
contract artifacts so the ABI for SlashProposal (used by getSlashProposal and
any callers) includes all 18 fields as defined in the ISlashingManager.sol
struct, then recompile/deploy or update the published JSON artifact so
clients/tests consume the corrected ABI.
In `@packages/enclave-contracts/contracts/token/EnclaveToken.sol`:
- Around line 157-168: Update the `@dev` NatSpec for batchMintAllocations to
document the additional revert condition for zero-address inputs: note that the
function will revert with ZeroAddress() if any recipient in the recipients array
is the zero address, in addition to the existing ZeroAmount() and
ExceedsTotalSupply() reverts; mirror the wording used for mintAllocation to keep
consistency with the single-mint sibling and reference the same error names
(ZeroAddress, ZeroAmount, ExceedsTotalSupply) and their conditions.
---
Duplicate comments:
In
`@packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json`:
- Around line 524-602: The artifact for ISlashingManager is stale: the ABI for
the getSlashProposal response (SlashProposal) is missing the snapshotted fields
banNode, affectsCommittee, and failureReason and the ChainIdMismatch error is
absent; regenerate the contract artifacts from the updated Solidity source
(ISlashingManager.sol) so the ABI/JSON includes the full struct (SlashProposal)
with banNode, affectsCommittee, failureReason and the ChainIdMismatch error
entry, or if regenerating is not possible, update the ABI manually to add those
fields to the getSlashProposal tuple and add the ChainIdMismatch error
definition to the errors array.
---
Nitpick comments:
In `@examples/CRISP/client/src/components/CircularTiles.tsx`:
- Around line 15-17: The useEffect in the CircularTiles component currently
calls setRotations(generateRotations(count)) unconditionally, which regenerates
all rotations when count changes; instead, update rotations incrementally by
using the state setter with the previous rotations: if count increases, keep
prev rotations and append only generateRotations(newCount - prev.length) for the
new slots; if count decreases, slice prev to the new length; if equal, return
prev; reference setRotations, generateRotations, and the useEffect that depends
on count when making this change.
In `@examples/CRISP/client/src/context/voteManagement/VoteManagement.context.tsx`:
- Around line 242-243: The current targeted ESLint suppression before exporting
both useVoteManagementContext and VoteManagementProvider is acceptable
short-term; to remove the suppression, refactor so this file only exports
VoteManagementProvider and move the hook export into a thin barrel (index)
module: keep createGenericContext usage here to produce {
useVoteManagementContext, VoteManagementProvider } but export only
VoteManagementProvider from this file, then create/update a separate barrel file
that imports useVoteManagementContext from this module and re-exports it (export
{ useVoteManagementContext } from './VoteManagement.context'), allowing you to
remove the eslint-disable-next-line comment and keep exports structurally
separated.
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 697-708: The setSlashingManager function lacks the same-value
guard present in sibling setters; update setSlashingManager (and its use of
slashingManager and event SlashingManagerSet) to require that
address(_slashingManager) != address(slashingManager) in addition to the
non-zero check, so calls that try to set the same contract are rejected
consistently.
- Around line 786-789: The validation currently uses a hardcoded upper bound
against uint8(FailureReason.VerificationFailed); update the FailureReason enum
to include a sentinel like FailureReason.MAX (or LAST) as the final value, or
add a helper function isValidFailureReason(uint8 reason) that checks reason > 0
&& reason <= uint8(FailureReason.MAX), then replace the require(...) with a call
to that helper (or compare against FailureReason.MAX) so future enum additions
automatically pass validation; update references to
FailureReason.VerificationFailed in this require check accordingly.
In `@packages/enclave-contracts/contracts/interfaces/IEnclave.sol`:
- Around line 317-320: The function declaration for removeE3ProgramsParams uses
bytes[] memory in an external interface which causes unnecessary copying; change
its parameter to bytes[] calldata to match the cheaper external calldata
convention and align with the existing setE3ProgramsParams pattern — update the
function signature for removeE3ProgramsParams (and, if any other external
functions like setE3ProgramsParams are inconsistent, make them bytes[] calldata
as well) so the external parameter uses calldata instead of memory.
In `@packages/enclave-contracts/contracts/registry/BondingRegistry.sol`:
- Around line 73-77: Add an on‑chain event and a zero‑address check for all
mutations of the authorizedDistributors allowlist: declare an event like
DistributorAuthorizationChanged(address indexed distributor, bool authorized)
and emit it whenever the mapping authorizedDistributors is updated; in each
mutating function that touches this mapping (e.g., any
authorizeDistributor/approveDistributor and
revokeDistributor/removeAuthorizedDistributor functions that update
authorizedDistributors) add require(distributor != address(0), "zero address not
allowed") before state changes and then emit
DistributorAuthorizationChanged(distributor, true/false). Apply the same pattern
to the other analogous allowlist(s) referenced around lines 696–709 so all admin
allowlist additions/removals have the zero‑address guard and emitted event for
auditability.
In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Around line 441-449: Add an event emission to setSlashingManager to improve
auditability: declare a descriptive event (e.g., SlashingManagerUpdated(address
indexed previous, address indexed current)) and emit it inside the
setSlashingManager function after the require and assignment (reference function
setSlashingManager and state variable slashingManager) so callers can track old
and new slashing manager addresses; ensure event is added to the contract's
events section and use address(slashingManager) for the previous value before
overwriting.
In `@packages/enclave-contracts/contracts/token/EnclaveTicketToken.sol`:
- Around line 199-201: The approve override currently reverts via
TransferNotAllowed() which blocks direct approvals but ERC20Permit.permit still
calls _approve internally, creating a potential confusion; add NatSpec comments
to the approve function and the permit function explaining that approvals are
intentionally disabled, that permit may appear to set allowances but those
allowances are unreachable because transferFrom-related logic is prevented by
_update, and reference TransferNotAllowed, _approve, and _update so readers
understand this defense-in-depth behavior.
- Around line 191-192: Add a typed custom error (e.g., ExceedsPayableBalance()
or InsufficientPayableBalance(uint256 requested, uint256 available)) alongside
the other contract errors (like NotRegistry, TransferNotAllowed) and replace the
require/string in the payout function (the require(amount <= payableBalance,
"Exceeds payable balance");) with a revert using that custom error (providing
any chosen params if you define them) and then decrement payableBalance as
before.
In `@templates/default/enclave.config.yaml`:
- Around line 17-19: The slashing_manager.address entry uses single quotes while
other contract addresses use double quotes; update the slashing_manager.address
value to use double quotes (change '0x0165878A594ca255338adfa4d48449f69242Eb8F'
to "0x0165878A594ca255338adfa4d48449f69242Eb8F") so quote style is consistent
across entries like slashing_manager.address and the other contract address
fields.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (54)
examples/CRISP/client/src/App.tsxexamples/CRISP/client/src/components/Cards/Card.tsxexamples/CRISP/client/src/components/Cards/PollCard.tsxexamples/CRISP/client/src/components/CircularTiles.tsxexamples/CRISP/client/src/components/NavMenu.tsxexamples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsxexamples/CRISP/client/src/context/voteManagement/VoteManagement.context.tsxexamples/CRISP/client/src/pages/DailyPoll/components/ConfirmVote.tsxexamples/CRISP/client/src/pages/PollResult/PollResult.tsxexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/deploy/deploy.tsexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonexamples/CRISP/server/.env.examplepackages/enclave-contracts/.gitignorepackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/token/EnclaveTicketToken.sol/EnclaveTicketToken.jsonpackages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/DkgPkVerifier.jsonpackages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/ZKTranscriptLib.jsonpackages/enclave-contracts/contracts/E3RefundManager.solpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IBondingRegistry.solpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/interfaces/ICircuitVerifier.solpackages/enclave-contracts/contracts/interfaces/IE3RefundManager.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/contracts/interfaces/ISlashVerifier.solpackages/enclave-contracts/contracts/interfaces/ISlashingManager.solpackages/enclave-contracts/contracts/registry/BondingRegistry.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/slashing/SlashingManager.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/contracts/test/MockSlashingVerifier.solpackages/enclave-contracts/contracts/token/EnclaveTicketToken.solpackages/enclave-contracts/contracts/token/EnclaveToken.solpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/ignition/modules/enclave.tspackages/enclave-contracts/ignition/modules/mockSlashingVerifier.tspackages/enclave-contracts/ignition/modules/slashingManager.tspackages/enclave-contracts/scripts/deployAndSave/enclave.tspackages/enclave-contracts/scripts/deployAndSave/slashingManager.tspackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/scripts/utils.tspackages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Registry/BondingRegistry.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tspackages/enclave-contracts/test/Slashing/SlashingManager.spec.tstemplates/default/client/src/context/WizardContext.tsxtemplates/default/client/src/pages/steps/RequestComputation.tsxtemplates/default/enclave.config.yaml
💤 Files with no reviewable changes (3)
- packages/enclave-contracts/scripts/deployAndSave/enclave.ts
- packages/enclave-contracts/contracts/interfaces/ISlashVerifier.sol
- packages/enclave-contracts/ignition/modules/enclave.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-contracts/contracts/slashing/SlashingManager.sol (1)
547-568:⚠️ Potential issue | 🟡 MinorUnresolved appeal permanently blocks slash execution — consider adding a governance override or timeout.
If an appeal is filed (
p.appealed = true) but governance never callsresolveAppeal,executeSlashis blocked indefinitely (line 366 requiresp.resolved). A malicious or negligent governance delay could prevent a valid slash from executing.Consider adding a fallback mechanism — e.g., auto-resolve after a configurable deadline, or allow
DEFAULT_ADMIN_ROLEto force-execute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/slashing/SlashingManager.sol` around lines 547 - 568, The unresolved-appeal issue: add a fallback so appeals cannot block executeSlash forever by (1) extending SlashProposal with an appealExpiry timestamp set when p.appealed is true (e.g., in the function that sets p.appealed), (2) update resolveAppeal to clear/set appealExpiry as appropriate, and (3) modify executeSlash to allow proceeding if p.resolved==true OR block.timestamp > p.appealExpiry (auto-expire) or if msg.sender has DEFAULT_ADMIN_ROLE (force-execute). Also add a governance/admin function (e.g., forceResolveAppeal) to let DEFAULT_ADMIN_ROLE set p.resolved and appealUpheld manually; ensure events (AppealResolved or a new ForceAppealResolved) are emitted and that totalProposals/InvalidProposal/AlreadyResolved checks remain consistent.
♻️ Duplicate comments (3)
packages/enclave-contracts/contracts/interfaces/ICircuitVerifier.sol (1)
19-22:viewmodifier is present — past concern resolved. LGTM.The
verifyfunction is correctly declaredexternal view returns (bool), matching concrete implementations and enablingSTATICCALLfrom callers dispatching through this interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/interfaces/ICircuitVerifier.sol` around lines 19 - 22, The interface declaration for function verify already correctly uses the external view signature; no changes required—leave function verify(bytes calldata _proof, bytes32[] calldata _publicInputs) external view returns (bool) as-is to match implementations and allow STATICCALL from callers.packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts (1)
75-80: Past issue resolved:committeeFormationWindowis now included.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts` around lines 75 - 80, The test's previous bug (missing committeeFormationWindow) has already been fixed by adding committeeFormationWindow to defaultTimeoutConfig; ensure defaultTimeoutConfig still contains committeeFormationWindow: ONE_DAY and remove any leftover duplicate/approval review annotations or stray comments related to that past issue so the test file (CommitteeExpulsion.spec.ts) contains only the clean config object and no duplicate review markers.packages/enclave-contracts/scripts/deployEnclave.ts (1)
193-207: LGTM — Cross-contract wiring is now complete.The previously missing
enclave.setSlashingManager(slashingManagerAddress)is now present (line 200), and the new dependencies (setCiphernodeRegistry,setEnclaveon SlashingManager, andsetSlashingManageron CiphernodeRegistry) are all properly wired. Deployment ordering correctly usesaddressOneplaceholders followed by post-deployment configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/scripts/deployEnclave.ts` around lines 193 - 207, Cross-contract wiring is complete—slashingManager.setCiphernodeRegistry, slashingManager.setEnclave, enclave.setSlashingManager, bondingRegistry.setSlashingManager, and ciphernodeRegistry.setSlashingManager are all present and correctly ordered—so there are no code changes required; simply resolve/close the duplicate review comment and mark the PR as approved.
🧹 Nitpick comments (5)
packages/enclave-contracts/contracts/interfaces/ICircuitVerifier.sol (1)
6-6: Consider tightening the pragma upper bound.
>=0.8.27with no ceiling permits compilation against future breaking versions (0.9.x, 1.0.x, etc.). Using a caret range is safer.🔧 Proposed change
-pragma solidity >=0.8.27; +pragma solidity ^0.8.27;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/interfaces/ICircuitVerifier.sol` at line 6, The pragma directive in ICircuitVerifier.sol is too loose (pragma solidity >=0.8.27) and should be tightened to prevent accidental compilation with future breaking versions; change it to a bounded range such as a caret range ^0.8.27 or a maximum-bound range >=0.8.27 <0.9.0 in the ICircuitVerifier.sol file so the interface compiles only with the intended 0.8.x compiler series.templates/default/client/src/context/WizardContext.tsx (1)
160-173: Extractsdkproperties used in context to avoid unstable object reference in deps.
useEnclaveSDKreturns a new object on every render (line 100). Since the entire return object is included in thecontextValueuseMemo deps array (line 172),contextValuebecomes a new reference on every render, causing all context consumers to re-render unnecessarily.Only
sdk.isInitializedis actually used from the sdk object within WizardContext (lines 125, 132). Consider either:
- Memoize the
useEnclaveSDKreturn in WizardContext:const memoizedSdk = useMemo(() => useEnclaveSDK(sdkConfig), [sdkConfig]), then usememoizedSdkin deps and context.- Extract and depend only on the specific properties needed: replace
sdkin deps withsdk.isInitialized, and pass only necessary properties to context instead of the full sdk object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/client/src/context/WizardContext.tsx` around lines 160 - 173, The contextValue useMemo is unstable because useEnclaveSDK returns a new sdk object each render; update WizardContext to avoid depending on the whole sdk object: either memoize the SDK return (call useEnclaveSDK once and wrap it in useMemo, e.g., memoizedSdk = useMemo(() => useEnclaveSDK(sdkConfig), [sdkConfig]) and use memoizedSdk in the contextValue deps) or, preferably, extract only the used property (sdk.isInitialized) and replace sdk in the contextValue and deps with that primitive (and pass only the needed properties into the context instead of the full sdk object) so contextValue and consumers no longer re-render on every render.packages/enclave-contracts/contracts/registry/BondingRegistry.sol (2)
689-703: Missing zero-address validation and events for distributor authorization changes.
setRewardDistributordoesn't check foraddress(0), allowing a no-op authorization. Also, neithersetRewardDistributornorrevokeRewardDistributoremit events, which makes it hard to audit authorization changes off-chain.♻️ Proposed fix
function setRewardDistributor( address newRewardDistributor ) public onlyOwner { + require(newRewardDistributor != address(0), ZeroAddress()); authorizedDistributors[newRewardDistributor] = true; + emit RewardDistributorUpdated(newRewardDistributor, true); } function revokeRewardDistributor(address distributor) public onlyOwner { authorizedDistributors[distributor] = false; + emit RewardDistributorUpdated(distributor, false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/registry/BondingRegistry.sol` around lines 689 - 703, Add zero-address validation and events for distributor changes: in setRewardDistributor(address newRewardDistributor) and revokeRewardDistributor(address distributor) check that the address is not address(0) and revert with a clear message if it is; declare and emit events (e.g., RewardDistributorSet(address indexed distributor) and RewardDistributorRevoked(address indexed distributor)) inside setRewardDistributor and revokeRewardDistributor respectively when updating authorizedDistributors mapping to true/false; reference the authorizedDistributors mapping, setRewardDistributor, and revokeRewardDistributor when making these changes so off-chain consumers can track authorization changes.
29-33: Acknowledged:max-states-countlint suppression.The growing number of storage variables (now with
authorizedDistributors) triggers the lint. The suppression is reasonable for now, but if more state is added, consider refactoring related state into structs or a library.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/registry/BondingRegistry.sol` around lines 29 - 33, The file suppresses solhint max-states-count on the BondingRegistry contract because of added storage like authorizedDistributors; to fix future lint pressure, refactor by grouping related storage variables into a single struct (e.g., BondingState) or move them into a library/child storage contract, then replace the individual state declarations in BondingRegistry with one BondingState state variable and update references in functions/initializers (including any uses of authorizedDistributors) to access via state.authorizedDistributors; keep the current suppression only temporarily until the refactor is applied.packages/enclave-contracts/contracts/E3RefundManager.sol (1)
34-36: Remove unused state variablesfeeTokenandbondingRegistry.Both variables are assigned during
initialize()(lines 91–92) but never referenced again. All payment logic uses the per-E3dist.feeTokenfrom theRefundDistributionstruct instead. These storage variables are dead code that should be removed, along with the correspondingIBondingRegistryimport on line 16.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/E3RefundManager.sol` around lines 34 - 36, Remove the dead storage and import: delete the IERC20 public feeToken and IBondingRegistry public bondingRegistry state variables, remove the IBondingRegistry import, and remove the assignments to feeToken and bondingRegistry inside initialize(); ensure all payment logic continues to use RefundDistribution.dist.feeToken (and other dist.* fields) and run a quick search to confirm no other references to feeToken or bondingRegistry remain before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/enclave-contracts/contracts/slashing/SlashingManager.sol`:
- Around line 547-568: The unresolved-appeal issue: add a fallback so appeals
cannot block executeSlash forever by (1) extending SlashProposal with an
appealExpiry timestamp set when p.appealed is true (e.g., in the function that
sets p.appealed), (2) update resolveAppeal to clear/set appealExpiry as
appropriate, and (3) modify executeSlash to allow proceeding if p.resolved==true
OR block.timestamp > p.appealExpiry (auto-expire) or if msg.sender has
DEFAULT_ADMIN_ROLE (force-execute). Also add a governance/admin function (e.g.,
forceResolveAppeal) to let DEFAULT_ADMIN_ROLE set p.resolved and appealUpheld
manually; ensure events (AppealResolved or a new ForceAppealResolved) are
emitted and that totalProposals/InvalidProposal/AlreadyResolved checks remain
consistent.
---
Duplicate comments:
In `@packages/enclave-contracts/contracts/interfaces/ICircuitVerifier.sol`:
- Around line 19-22: The interface declaration for function verify already
correctly uses the external view signature; no changes required—leave function
verify(bytes calldata _proof, bytes32[] calldata _publicInputs) external view
returns (bool) as-is to match implementations and allow STATICCALL from callers.
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 193-207: Cross-contract wiring is
complete—slashingManager.setCiphernodeRegistry, slashingManager.setEnclave,
enclave.setSlashingManager, bondingRegistry.setSlashingManager, and
ciphernodeRegistry.setSlashingManager are all present and correctly ordered—so
there are no code changes required; simply resolve/close the duplicate review
comment and mark the PR as approved.
In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts`:
- Around line 75-80: The test's previous bug (missing committeeFormationWindow)
has already been fixed by adding committeeFormationWindow to
defaultTimeoutConfig; ensure defaultTimeoutConfig still contains
committeeFormationWindow: ONE_DAY and remove any leftover duplicate/approval
review annotations or stray comments related to that past issue so the test file
(CommitteeExpulsion.spec.ts) contains only the clean config object and no
duplicate review markers.
---
Nitpick comments:
In `@packages/enclave-contracts/contracts/E3RefundManager.sol`:
- Around line 34-36: Remove the dead storage and import: delete the IERC20
public feeToken and IBondingRegistry public bondingRegistry state variables,
remove the IBondingRegistry import, and remove the assignments to feeToken and
bondingRegistry inside initialize(); ensure all payment logic continues to use
RefundDistribution.dist.feeToken (and other dist.* fields) and run a quick
search to confirm no other references to feeToken or bondingRegistry remain
before committing.
In `@packages/enclave-contracts/contracts/interfaces/ICircuitVerifier.sol`:
- Line 6: The pragma directive in ICircuitVerifier.sol is too loose (pragma
solidity >=0.8.27) and should be tightened to prevent accidental compilation
with future breaking versions; change it to a bounded range such as a caret
range ^0.8.27 or a maximum-bound range >=0.8.27 <0.9.0 in the
ICircuitVerifier.sol file so the interface compiles only with the intended 0.8.x
compiler series.
In `@packages/enclave-contracts/contracts/registry/BondingRegistry.sol`:
- Around line 689-703: Add zero-address validation and events for distributor
changes: in setRewardDistributor(address newRewardDistributor) and
revokeRewardDistributor(address distributor) check that the address is not
address(0) and revert with a clear message if it is; declare and emit events
(e.g., RewardDistributorSet(address indexed distributor) and
RewardDistributorRevoked(address indexed distributor)) inside
setRewardDistributor and revokeRewardDistributor respectively when updating
authorizedDistributors mapping to true/false; reference the
authorizedDistributors mapping, setRewardDistributor, and
revokeRewardDistributor when making these changes so off-chain consumers can
track authorization changes.
- Around line 29-33: The file suppresses solhint max-states-count on the
BondingRegistry contract because of added storage like authorizedDistributors;
to fix future lint pressure, refactor by grouping related storage variables into
a single struct (e.g., BondingState) or move them into a library/child storage
contract, then replace the individual state declarations in BondingRegistry with
one BondingState state variable and update references in functions/initializers
(including any uses of authorizedDistributors) to access via
state.authorizedDistributors; keep the current suppression only temporarily
until the refactor is applied.
In `@templates/default/client/src/context/WizardContext.tsx`:
- Around line 160-173: The contextValue useMemo is unstable because
useEnclaveSDK returns a new sdk object each render; update WizardContext to
avoid depending on the whole sdk object: either memoize the SDK return (call
useEnclaveSDK once and wrap it in useMemo, e.g., memoizedSdk = useMemo(() =>
useEnclaveSDK(sdkConfig), [sdkConfig]) and use memoizedSdk in the contextValue
deps) or, preferably, extract only the used property (sdk.isInitialized) and
replace sdk in the contextValue and deps with that primitive (and pass only the
needed properties into the context instead of the full sdk object) so
contextValue and consumers no longer re-render on every render.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/enclave-contracts/contracts/E3RefundManager.solpackages/enclave-contracts/contracts/interfaces/ICircuitVerifier.solpackages/enclave-contracts/contracts/registry/BondingRegistry.solpackages/enclave-contracts/contracts/slashing/SlashingManager.solpackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tstemplates/default/client/src/context/WizardContext.tsx
ctrlc03
reviewed
Feb 26, 2026
This was referenced Mar 13, 2026
This was referenced May 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Configuration
Tests