fix(ShutterDK): tie recovery commit #2175
Conversation
✅ Deploy Preview for kleros-v2-neo 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-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughDerive commit on-chain from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant DK as DisputeKit(Shutter|Gated)
participant Core as Core/_castCommit
Note over Caller,DK: castCommitShutter(coreDisputeID, voteIDs, recoveryCommit, justification, identity, encryptedVote)
Caller->>DK: castCommitShutter(...)
DK->>DK: justificationHash = keccak256(bytes(justification))
DK->>DK: commit = keccak256(abi.encode(recoveryCommit, justificationHash))
DK->>Core: _castCommit(coreDisputeID, voteIDs, commit, identity, encryptedVote)
Core-->>DK: result
DK-->>Caller: emit CommitCastShutter(commit, recoveryCommit, ...)
alt non-juror reveal/hash path
Note over DK: voteHash = keccak256(abi.encode( keccak256(abi.encodePacked(choice, salt)), justificationHash ))
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-30T17:18:12.895ZApplied to files:
⏰ 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). (15)
🔇 Additional comments (3)
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
🧹 Nitpick comments (2)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
114-136: LGTM! On-chain commit derivation improves security.The change to derive the commit on-chain from
_recoveryCommitand_justificationensures that the commitment structure is always correctly formed. This prevents potential issues where an incorrectly constructed commit could be submitted.Consider using
string calldata _justificationinstead ofstring memoryfor better gas efficiency:function castCommitShutter( uint256 _coreDisputeID, uint256[] calldata _voteIDs, bytes32 _recoveryCommit, - string memory _justification, + string calldata _justification, bytes32 _identity, bytes calldata _encryptedVote ) external notJumped(_coreDisputeID) {While the conversion to
bytes(_justification)for hashing will copy to memory anyway, usingcalldatasignals intent and may provide minor gas savings in some scenarios.contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
98-120: LGTM! On-chain commit derivation improves security.The change to derive the commit on-chain from
_recoveryCommitand_justificationensures that the commitment structure is always correctly formed. This prevents potential issues where an incorrectly constructed commit could be submitted.Consider using
string calldata _justificationinstead ofstring memoryfor better gas efficiency:function castCommitShutter( uint256 _coreDisputeID, uint256[] calldata _voteIDs, bytes32 _recoveryCommit, - string memory _justification, + string calldata _justification, bytes32 _identity, bytes calldata _encryptedVote ) external notJumped(_coreDisputeID) {While the conversion to
bytes(_justification)for hashing will copy to memory anyway, usingcalldatasignals intent and may provide minor gas savings in some scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(3 hunks)contracts/test/arbitration/dispute-kit-shutter.ts(22 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/DisputeKitShutter.solcontracts/test/arbitration/dispute-kit-shutter.tscontracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.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
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (4)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
172-185: LGTM! Hash logic aligns with on-chain commit construction.The updated hash logic for non-jurors now correctly mirrors the commit structure by first hashing
(choice, salt)to produce the recovery commit, then encoding it with the justification hash. This ensures consistency between commit construction incastCommitShutterand vote validation incastVoteShutter.contracts/test/arbitration/dispute-kit-shutter.ts (2)
95-108: LGTM! Test helper correctly reflects on-chain commit structure.The
generateCommitmentshelper now properly constructs the full commit by encoding the recovery commit and justification hash as two separatebytes32values. This aligns with the contract's on-chain commit derivation logic.
623-637: LGTM! Hash computation test validates the new structure.The test correctly verifies that
hashVotefor non-jurors produces a hash matching the commit structure:keccak256(abi.encode(recoveryCommit, justificationHash))whererecoveryCommit = keccak256(abi.encode([choice, salt])).contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
156-169: LGTM! Hash logic aligns with on-chain commit construction.The updated hash logic for non-jurors now correctly mirrors the commit structure by first hashing
(choice, salt)to produce the recovery commit, then encoding it with the justification hash. This ensures consistency between commit construction incastCommitShutterand vote validation incastVoteShutter.
|
1 participant
PR-Codex overview
This PR focuses on updating the
castCommitShutterfunction in theDisputeKitGatedShutterandDisputeKitShuttercontracts to improve handling of justification and commitment hashes. It streamlines the parameters and updates the hash computation logic.Detailed summary
castCommitShutterparameters to includejustificationHashandidentity._commitand changed_recoveryCommitto be used with the new justification approach.recoveryCommitandjustificationHash.Summary by CodeRabbit
New Features
Tests