fix(ShutterDK): replace recovery with justification commit #2177
Conversation
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughReplaces per-vote hash-return hook with a batch verification hook for hidden-vote commitments, renames recovery→justification commitments, splits choice and justification commits in commit flows, updates events/errors/function signatures, and adapts tests to the new justification-centric verification. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Voter as Voter/Relayer
participant DK as DisputeKitClassicBase
participant Impl as DisputeKitShutter/Gated
participant Store as Contract Storage
Note over DK,Impl: Hidden-vote reveal (batch verification)
Voter->>DK: castVote(_voteIDs, _choice, _justification, _salt, ...)
DK->>Impl: _verifyHiddenVoteCommitments(localDisputeID, localRoundID, _voteIDs, _choice, _justification, _salt)
alt all commitments match
Impl-->>DK: verified (void)
DK->>Store: record vote(s)
DK-->>Voter: success
else mismatch
Impl-->>DK: revert ChoiceCommitmentMismatch()/JustificationCommitmentMismatch()
DK-->>Voter: revert
end
sequenceDiagram
autonumber
participant Caller as Voter
participant Impl as DisputeKitShutter/Gated
participant Store as Contract Storage
Note over Impl: Commit flow (choice + justification)
Caller->>Impl: castCommitShutter(_voteIDs, _choiceCommit, _justificationCommit, ...)
alt _justificationCommit == 0
Impl-->>Caller: revert EmptyJustificationCommit()
else
Impl->>Store: justificationCommitments[dispute][round][voteID] = _justificationCommit
Impl-->>Caller: emit CommitCastShutter(..., _choiceCommit, _justificationCommit)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
151-166: Bug: Possible out-of-bounds read on empty _voteIDscastVoteShutter reads _voteIDs[0] before any length check. Passing an empty array will revert with a generic panic instead of EmptyVoteIDs(), diverging from base behavior.
Apply this guard before indexing:
function castVoteShutter( uint256 _coreDisputeID, uint256[] calldata _voteIDs, uint256 _choice, uint256 _salt, string memory _justification ) external { + if (_voteIDs.length == 0) revert EmptyVoteIDs(); Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; address juror = dispute.rounds[dispute.rounds.length - 1].votes[_voteIDs[0]].account;contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
141-150: Bug: Possible out-of-bounds read on empty _voteIDscastVoteShutter reads _voteIDs[0] before any length check. Empty arrays will revert with a generic panic instead of EmptyVoteIDs().
Apply this guard before indexing:
function castVoteShutter( uint256 _coreDisputeID, uint256[] calldata _voteIDs, uint256 _choice, uint256 _salt, string memory _justification ) external { + if (_voteIDs.length == 0) revert EmptyVoteIDs(); Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; address juror = dispute.rounds[dispute.rounds.length - 1].votes[_voteIDs[0]].account;
🧹 Nitpick comments (4)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
117-122: Reorder writes after commit verification to save gas and tighten semanticsCurrently, justificationCommitments are written before _castCommit checks period and ownership. These writes will revert anyway if checks fail, wasting gas. Move _castCommit before storing justification commits.
function castCommitShutter( uint256 _coreDisputeID, uint256[] calldata _voteIDs, bytes32 _choiceCommit, bytes32 _justificationCommit, bytes32 _identity, bytes calldata _encryptedVote ) external { if (_justificationCommit == bytes32(0)) revert EmptyJustificationCommit(); - uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; - Dispute storage dispute = disputes[localDisputeID]; - uint256 localRoundID = dispute.rounds.length - 1; - for (uint256 i = 0; i < _voteIDs.length; i++) { - justificationCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _justificationCommit; - } - // `_castCommit()` ensures that the caller owns the vote and that dispute is active _castCommit(_coreDisputeID, _voteIDs, _choiceCommit); + uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; + Dispute storage dispute = disputes[localDisputeID]; + uint256 localRoundID = dispute.rounds.length - 1; + for (uint256 i = 0; i < _voteIDs.length; i++) { + justificationCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _justificationCommit; + } emit CommitCastShutter( _coreDisputeID, msg.sender, _choiceCommit, _justificationCommit, _identity, _encryptedVote ); }Also applies to: 128-140
contracts/test/arbitration/dispute-kit-shutter.ts (2)
505-530: Test name contradicts behavior; rename for clarityThe test asserts that juror reveals bypass justification validation. Rename to reflect behavior (e.g., “Juror reveal bypasses justification validation”).
614-630: Incomplete test: no assertionsAdd concrete assertions or remove the test. Example: assert that juror reveal succeeds with different justification while non-juror with same data reverts on justification mismatch.
// Example assertions to add: await expect(disputeKitShutter.connect(juror1).castVoteShutter(disputeId, voteIDs, choice, salt, "X")) .to.emit(disputeKitShutter, "VoteCast"); await expect(disputeKitShutter.connect(attacker).castVoteShutter(disputeId, voteIDs, choice, salt, "X")) .to.be.revertedWithCustomError(disputeKitShutter, "WrongJustification");contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
101-107: Reorder writes after commit verification to save gas and tighten semanticsWrite justificationCommitments only after _castCommit validates period and ownership.
function castCommitShutter( uint256 _coreDisputeID, uint256[] calldata _voteIDs, bytes32 _choiceCommit, bytes32 _justificationCommit, bytes32 _identity, bytes calldata _encryptedVote ) external { if (_justificationCommit == bytes32(0)) revert EmptyJustificationCommit(); - uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; - Dispute storage dispute = disputes[localDisputeID]; - uint256 localRoundID = dispute.rounds.length - 1; - for (uint256 i = 0; i < _voteIDs.length; i++) { - justificationCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _justificationCommit; - } - // `_castCommit()` ensures that the caller owns the vote and that dispute is active _castCommit(_coreDisputeID, _voteIDs, _choiceCommit); + uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; + Dispute storage dispute = disputes[localDisputeID]; + uint256 localRoundID = dispute.rounds.length - 1; + for (uint256 i = 0; i < _voteIDs.length; i++) { + justificationCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _justificationCommit; + } emit CommitCastShutter( _coreDisputeID, msg.sender, _choiceCommit, _justificationCommit, _identity, _encryptedVote ); }Also applies to: 112-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(5 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(4 hunks)contracts/test/arbitration/dispute-kit-shutter.ts(20 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/test/arbitration/dispute-kit-shutter.ts
⏰ 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). (14)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (14)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (4)
37-38: Rename and public exposure LGTMSwitch to justificationCommitments and bytes32 type aligns with the new flow.
53-55: Event schema aligns with two-commit model_choiceCommit indexed and separate _justificationCommit is correct for querying and later verification.
Also applies to: 60-62
173-186: Justification check logic LGTM
- Non-jurors must match keccak256(abi.encode(salt, keccak256(bytes(justification)))).
- Jurors are allowed to bypass justification validation for recovery.
243-243: Error naming LGTMEmptyJustificationCommit matches the new invariant.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
344-346: Enforce justification via hook before recording voteThe _checkJustification hook + WrongJustification revert integrates cleanly into the voting loop.
729-739: Default-true _checkJustification keeps backward compatibilityVirtual hook returning true by default avoids breaking other DKs; derived DKs can opt-in.
789-789: New error type LGTMWrongJustification is specific and improves diagnosability.
contracts/test/arbitration/dispute-kit-shutter.ts (3)
96-108: Commit generation matches on-chain hashing
- choiceCommit = keccak256(abi.encode(choice, salt))
- justificationCommit = keccak256(abi.encode(salt, keccak256(bytes(justification))))
Aligned with solidity implementations.
211-228: Commit flow and storage assertions LGTMEvent args and stored justificationCommitment checks are correct and robust.
Also applies to: 231-237
440-441: Negative path for wrong justification LGTMExpecting WrongJustification for non-juror reveal with incorrect justification is correct.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (4)
21-22: Rename and public exposure LGTMPublic justificationCommitments mapping matches the new commit model.
37-39: Event schema aligns with two-commit modelIndexed _choiceCommit and separate _justificationCommit will aid indexing and verification.
Also applies to: 44-46
156-170: Justification check logic LGTM
- Non-jurors must match justificationCommit.
- Jurors can bypass for recovery.
176-176: Error naming LGTMEmptyJustificationCommit reflects the new invariant.
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)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
150-166: Add length validation for_voteIDsarray before accessing it at line 158.The array is accessed at line 158 without checking if it's empty. While the inherited
_castVotefunction validates non-empty arrays (DisputeKitClassicBase.sol:326), that validation occurs after this function has already dereferenced_voteIDs[0], causing an out-of-bounds revert. Add a check before line 158:if (_voteIDs.length == 0) revert EmptyVoteIDs();The same issue exists in DisputeKitShutter.sol (line 142).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(5 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (12)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (7)
172-178: LGTM! Secure double-hashing pattern for commit-reveal.The implementation correctly uses double hashing:
keccak256(bytes(_justification))converts the variable-length string to a fixed 32-byte hashkeccak256(abi.encode(_salt, ...))combines with salt for the commitment schemeThis prevents length extension attacks and is standard practice for commit-reveal schemes.
44-44: LGTM: Appropriate use of transient storage.The
transientkeyword (Solidity 0.8.24+) is correctly used for thecallerIsJurorflag, which only needs to persist within a single transaction. This is more gas-efficient than regular storage for temporary cross-function communication.Verify that the transient variable lifecycle is handled correctly in
castVoteShutter(lines 150-166):
- Set to
truebefore calling_castVote- Reset to
falseafter_castVotecompletes- Read in
_verifyHiddenVoteCommitmentsduring the_castVotecall chain
260-261: Error declarations updated to match new terminology.The error names have been updated from
EmptyRecoveryCommittoEmptyJustificationCommit, andWrongJustificationhas been added. These changes align with the shift from recovery-based to justification-based commitments.As suggested in the earlier comment on lines 185-203, consider adding a parameter to
WrongJustificationto indicate which vote ID failed verification.
114-125: _castCommit already checks for zero commit
The base implementation reverts withEmptyCommit()when the commit isbytes32(0), so no extra_choiceCommitcheck is needed here.Likely an incorrect or invalid review comment.
50-64: Breaking change to CommitCastShutter event
Renamed_commit→_choiceCommitand_recoveryCommit→_justificationCommit; update any off-chain listeners, subgraphs, frontends, and indexers to the new signature before deployment.
37-38: Verify storage layout after mapping renameNo occurrences of
recoveryCommitmentsfound; confirm off-chain integrations, migration scripts, and upgrade safety tooling reflect the newjustificationCommitmentsmapping.
185-203: Remove security concern — juror bypass is an intentional recovery mechanism
ThecallerIsJurorearly return matches the identical pattern in DisputeKitShutter, is vetted by prior PRs (e.g. “shutter-recovery-hash”), and doesn’t impact vote weight or rewards. Keeping parameterless errors aligns with existing code style; the suggested error-parameter refactor is optional.Likely an incorrect or invalid review comment.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (5)
98-107: Consider adding validation for_choiceCommit.Similar to
DisputeKitGatedShutter.sol, there's no check that_choiceCommitis non-zero while_justificationCommitis validated. Verify that the base contract's_castCommithandles this validation.
156-162: LGTM! Secure double-hashing pattern for commit-reveal.The implementation uses the same secure double-hashing pattern as in
DisputeKitGatedShutter.sol, correctly handling variable-length strings in the commit-reveal scheme.
134-150: Verify empty_voteIDsarray handling.Line 142 accesses
_voteIDs[0]without checking if the array is empty. Verify that_castVotevalidates array length before this access occurs.
21-22: Autogenerated getter renamed – update dependentsRenaming
recoveryCommitmentstojustificationCommitmentsdoesn’t affect storage layout but changes the public getter signature; update any contracts, scripts, or tests callingrecoveryCommitments.Likely an incorrect or invalid review comment.
169-187: Review Justification Bypass
ThecallerIsJurorcheck at lines 179–180 lets jurors skip justification verification. Confirm this bypass is intentional, covered by tests, and consistent with the protocol’s security model. Optionally, improve diagnostics by including the failingvoteIDin theWrongJustificationerror (note: this changes its signature and requires test updates).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
721-734: Consider improving error specificity for multi-vote scenarios.When a juror with multiple votes encounters a verification failure, the current error doesn't indicate which vote ID failed. This could complicate debugging.
Consider this enhancement:
function _verifyHiddenVoteCommitments( uint256 _localDisputeID, uint256 _localRoundID, uint256[] calldata _voteIDs, uint256 _choice, string memory _justification, uint256 _salt ) internal view virtual { bytes32 actualVoteHash = hashVote(_choice, _salt, _justification); for (uint256 i = 0; i < _voteIDs.length; i++) { - if (disputes[_localDisputeID].rounds[_localRoundID].votes[_voteIDs[i]].commit != actualVoteHash) + if (disputes[_localDisputeID].rounds[_localRoundID].votes[_voteIDs[i]].commit != actualVoteHash) { revert HashDoesNotMatchHiddenVoteCommitment(); + } } }Alternatively, if you want to provide the vote ID in the error:
error HashDoesNotMatchHiddenVoteCommitment(uint256 voteID);And update the revert:
revert HashDoesNotMatchHiddenVoteCommitment(_voteIDs[i]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
🔇 Additional comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
337-339: LGTM: Verification occurs before state changes.The verification call correctly precedes the vote-saving loop, ensuring atomicity—if any vote fails verification, no votes are saved. This fail-fast approach is appropriate for the commit-reveal scheme.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (3)
350-353: Update outdated comment.The comment still refers to "recovery commitment" but should say "justification commitment" to match the new terminology.
Apply this diff:
- // Verify recovery commitment was stored + // Verify justification commitment was stored const localDisputeId = await ctx.disputeKit.coreDisputeIDToLocal(disputeId); const storedRecoveryCommit = await ctx.disputeKit.justificationCommitments(localDisputeId, 0, voteIDs[0]);
384-386: Update outdated comment.The comment should refer to "justification commitment" instead of using the outdated "recovery" terminology.
Apply this diff:
- // Verify only the second commitment is stored + // Verify only the second justification commitment is stored const localDisputeId = await ctx.disputeKit.coreDisputeIDToLocal(disputeId); const storedRecoveryCommit = await ctx.disputeKit.justificationCommitments(localDisputeId, 0, voteIDs[0]);
626-651: Misleading test name - consider clarifying.The test name "Should validate against justification commitment when juror reveals" suggests that validation occurs, but the test actually demonstrates that jurors can provide ANY justification without validation (lines 641-650). This is the intended "recovery flow" behavior where Shutter fails and the juror reveals without justification verification.
Consider renaming to better reflect the actual behavior:
- it("Should validate against justification commitment when juror reveals", async () => { + it("Should NOT validate justification when juror reveals (recovery flow)", async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(5 hunks)contracts/test/arbitration/dispute-kit-gated-shutter.ts(0 hunks)contracts/test/arbitration/dispute-kit-shutter.ts(0 hunks)contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts(19 hunks)
💤 Files with no reviewable changes (2)
- contracts/test/arbitration/dispute-kit-shutter.ts
- contracts/test/arbitration/dispute-kit-gated-shutter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts
⏰ 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). (14)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: hardhat-tests
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Analyze (javascript)
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (9)
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (2)
74-88: LGTM! Commitment generation logic is correct.The function correctly separates choice and justification commitments:
choiceCommit= hash(choice, salt)justificationCommit= hash(salt, hash(justification))This matches the expected behavior described in the PR objectives.
736-851: LGTM! Edge case tests properly updated.The tests correctly verify the dual-path behavior:
- Normal flow: Bot reveals with full justification that must match commitment
- Recovery flow: Juror reveals without justification validation
- Security: Attackers cannot reveal without correct data
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (7)
40-41: LGTM! Storage mapping renamed appropriately.The rename from
recoveryCommitmentstojustificationCommitmentsaccurately reflects the commitment's purpose in the new verification flow.
56-67: LGTM! Event parameters updated consistently.Event parameters and documentation correctly reflect the split between choice and justification commitments.
142-169: LGTM! Function correctly implements split commitments.The function properly:
- Validates
_justificationCommitis non-empty- Stores justification commitments separately from choice commitments
- Maintains support for commitment updates during the commit period
Based on learnings: Jurors are allowed to re-submit commits during the commit period, which is handled correctly here.
200-206: LGTM! Hash function implementation is correct.The function correctly computes
hash(encode(salt, hash(justification))), which matches the test helper implementation and provides proper commitment security.
213-231: LGTM! Dual-path verification correctly implemented.The function properly implements two verification paths:
- Juror path (line 224): Allows recovery without justification verification when Shutter fails
- Non-juror path (lines 226-230): Requires exact justification match for bot reveals
The use of transient
callerIsJurorflag (set incastVoteShutterline 188) correctly distinguishes between these paths within a single transaction.
47-47: LGTM! Transient storage correctly declared.The
transientkeyword is properly used for thecallerIsJurorflag, which only needs to persist within a single transaction. This requires Solidity 0.8.24+, which is satisfied by the pragma.
289-290: LGTM! Error declarations are appropriate.Both errors accurately describe their failure conditions:
EmptyJustificationCommit: Prevents empty justification commitments during commit phaseWrongJustification: Indicates justification hash mismatch during non-juror reveals
|
jaybuidl
added
Type: Bug 🐛
Compatibility: ABI change 🗯
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
135-150: Guard against empty voteIDs before indexing.castVoteShutter reads _voteIDs[0] without checking length; empty input will panic with out-of-bounds instead of emitting EmptyVoteIDs.
function castVoteShutter( @@ - Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; + if (_voteIDs.length == 0) revert EmptyVoteIDs(); + Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]];contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (2)
3-3: Compiler version incompatible with transient storage.Contract uses
bool transient callerIsJuror;butpragma solidity ^0.8.24;will not compile with transient state vars. Bump to ≥0.8.28 (which fully supports transient state variables) or removetransient.-pragma solidity ^0.8.24; +pragma solidity ^0.8.28;Also applies to: 47-47
179-194: Guard against empty voteIDs before indexing.Add the same EmptyVoteIDs precheck here.
- Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; + if (_voteIDs.length == 0) revert EmptyVoteIDs(); + Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]];
♻️ Duplicate comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
61-67: Breaking event signature reminder.CommitCastShutter parameter changes break subscribers; coordinate subgraph/indexer updates before deploy.
#!/bin/bash # Find usages or listeners that still rely on old event args/names. rg -n --hidden -C2 'CommitCastShutter' || true rg -n --hidden -C2 'recoveryCommit' || true
🧹 Nitpick comments (9)
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (3)
350-354: Rename “recovery” terminology leftovers.Comment and variable storedRecoveryCommit still use “recovery”. Rename to “justification” to avoid confusion.
- // Verify recovery commitment was stored + // Verify justification commitment was stored - const storedRecoveryCommit = await ctx.disputeKit.justificationCommitments(localDisputeId, 0, voteIDs[0]); - expect(storedRecoveryCommit).to.equal(justificationCommit); + const storedJustificationCommit = await ctx.disputeKit.justificationCommitments(localDisputeId, 0, voteIDs[0]); + expect(storedJustificationCommit).to.equal(justificationCommit);
385-387: Consistent naming.Rename storedRecoveryCommit → storedJustificationCommit here too.
186-203: Avoid magic maxVotes=10 when collecting vote IDs.Use nbVoters from getRoundInfo to bound the loop.
- const maxVotes = 10; - for (let i = 0; i < maxVotes; i++) { + const [, , , , nbVoters] = await context.disputeKit.getRoundInfo(disputeId, 0, 0); + for (let i = 0; i < Number(nbVoters); i++) {contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
498-503: Make hashVote pure.It does not read state; marking pure saves gas and documents intent.
- ) public view virtual returns (bytes32) { + ) public pure virtual returns (bytes32) {
716-728: Parameter order inconsistency (salt/justification).castVote signature is (_choice, _salt, _justification) but _verifyHiddenVoteCommitments uses (_choice, _justification, _salt). Consider aligning for readability and to avoid call-site mistakes, or add a brief comment noting the order difference.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (2)
106-117: Write justification after period/ownership checks to reduce wasted SSTOREs on revert.Call _castCommit first, then persist justification commitments and emit event.
- uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; - Dispute storage dispute = disputes[localDisputeID]; - uint256 localRoundID = dispute.rounds.length - 1; - for (uint256 i = 0; i < _voteIDs.length; i++) { - justificationCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _justificationCommit; - } - - // `_castCommit()` ensures that the caller owns the vote and that dispute is active - _castCommit(_coreDisputeID, _voteIDs, _choiceCommit); + // `_castCommit()` ensures correct period, ownership and activity. + _castCommit(_coreDisputeID, _voteIDs, _choiceCommit); + + uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; + Dispute storage dispute = disputes[localDisputeID]; + uint256 localRoundID = dispute.rounds.length - 1; + for (uint256 i = 0; i < _voteIDs.length; i++) { + justificationCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _justificationCommit; + }Also applies to: 111-113, 117-125
34-48: Doc nit: event describes “vote is cast” but this is a commit event.Update to “when a commit is cast” for accuracy.
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (2)
143-151: Write justification after _castCommit to avoid wasted storage on revert.Same improvement as non-gated variant.
- uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; - Dispute storage dispute = disputes[localDisputeID]; - uint256 localRoundID = dispute.rounds.length - 1; - for (uint256 i = 0; i < _voteIDs.length; i++) { - justificationCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _justificationCommit; - } - - // `_castCommit()` ensures that the caller owns the vote and that dispute is active - _castCommit(_coreDisputeID, _voteIDs, _choiceCommit); + _castCommit(_coreDisputeID, _voteIDs, _choiceCommit); + + uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; + Dispute storage dispute = disputes[localDisputeID]; + uint256 localRoundID = dispute.rounds.length - 1; + for (uint256 i = 0; i < _voteIDs.length; i++) { + justificationCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _justificationCommit; + }Also applies to: 155-169, 160-166
53-67: Doc nit: event describes cast action as vote (it’s commit).Align wording with CommitCastShutter semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(5 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(4 hunks)contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts(24 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/test/arbitration/helpers/dispute-kit-shutter-common.tscontracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (5)
contracts/test/foundry/KlerosCore_Voting.t.sol (1)
117-117: Selector update is correct.Switching to ChoiceCommitmentMismatch matches the new batch verification path.
Also applies to: 121-121
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
337-340: Batch verification hook looks good.hidden votes now verified via a dedicated hook; revert path and error align with tests.
Also applies to: 714-734, 776-776
284-295: Re-commit counting preserved (good).CommitCount pattern prevents double-counting on recommit, matching prior guidance.
Based on learnings
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
168-187: Recovery carve‑out is clear and minimal.Transient flag gating justification checks for jurors is sound with super-call first verifying choice commit.
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
212-231: Recovery carve‑out mirrors non‑gated variant.Approach is consistent and minimal.
PR-Codex overview
This PR focuses on modifying the commitment and voting process in the
DisputeKitcontracts, specifically changing how commitments are handled, renaming functions, and updating error messages to reflect the new logic around justification commitments instead of recovery commitments.Detailed summary
testHashFunctionBehaviortests indispute-kit-shutteranddispute-kit-gated-shutter.recoveryCommitwithjustificationCommitinCommitCastShutterevents.hashVotetohashJustification.Summary by CodeRabbit
New Features
Refactor
Tests