fix(DK): multiple commit fix #2145
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. |
WalkthroughAdjusts commit accounting in DisputeKitClassicBase.castCommit to increment totals only for previously uncommitted voteIDs and adds tests validating recommit behavior without inflating totalCommitted while updating stored commit hashes. Changes
Sequence Diagram(s)sequenceDiagram autonumber actor Juror participant DK as DisputeKitClassicBase participant Storage as Vote Storage Note over Juror,DK: castCommit(voteIDs[], commit) Juror->>DK: castCommit(voteIDs, commit) activate DK DK->>DK: commitCount = 0 loop For each voteID DK->>Storage: read commit[voteID] alt not previously committed (commit == 0) DK->>Storage: set commit[voteID] = newCommit DK->>DK: commitCount++ else already committed DK->>Storage: update commit[voteID] = newCommit end end DK->>Storage: totalCommitted += commitCount deactivate DK DK-->>Juror: return Note over DK: Recommit updates hash but not totalCommitted Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
253-257: Update NatSpec to reflect new single-commit semanticsDoc says commits can be overridden, but code now reverts on re-commit. Align the comment with behavior.
Apply:
- /// @dev It can be called multiple times during the commit period, each call overrides the commits of the previous one. + /// @dev Each vote can be committed at most once per round. Attempts to re-commit an already committed vote revert with AlreadyCommittedThisVote().
🧹 Nitpick comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
277-281: Keep revert semantics predictable and save an sload per iterationCheck ownership before the “already committed” guard to avoid revealing commit status to non-owners and use a local Vote storage reference.
Apply:
- for (uint256 i = 0; i < _voteIDs.length; i++) { - if (round.votes[_voteIDs[i]].commit != bytes32(0)) revert AlreadyCommittedThisVote(); - if (round.votes[_voteIDs[i]].account != msg.sender) revert JurorHasToOwnTheVote(); - round.votes[_voteIDs[i]].commit = _commit; - } + for (uint256 i = 0; i < _voteIDs.length; i++) { + Vote storage v = round.votes[_voteIDs[i]]; + if (v.account != msg.sender) revert JurorHasToOwnTheVote(); + if (v.commit != bytes32(0)) revert AlreadyCommittedThisVote(); + v.commit = _commit; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(2 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(1 hunks)
⏰ 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: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (2)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
763-764: LGTM: new error improves clarityError declaration is appropriate and non-breaking for storage.
contracts/test/foundry/KlerosCore_Voting.t.sol (1)
100-103: LGTM: asserts double-commit reverts as intendedTest correctly guards against re-commit attempts.
Consider adding one more check to ensure atomicity on mixed arrays (one already committed, one fresh) reverts and does not partially commit the fresh vote ID.
| @coderabbitai create an issue for this |
| Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 47 @jaybuidl I've created issue #2151 to track this fix: #2151 The issue documents the bug where multiple commits could be submitted for the same vote and the solution implemented in this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
281-283: Potential logic issue: Re-commit with same hash not properly handled.The current logic only increments
commitCountifcommit == bytes32(0). However, if a juror callscastCommittwice with the same commit hash, the second call will:
- Not increment
commitCount(correct)- Overwrite
commitwith the same value (redundant but harmless)- Emit
CommitCastevent again (potentially confusing)While this doesn't break functionality, it allows redundant operations. Consider whether this is the desired behavior or if you should add an early check to prevent calling
castCommitwith an identical hash.If you want to optimize, you could add:
for (uint256 i = 0; i < _voteIDs.length; i++) { if (round.votes[_voteIDs[i]].account != msg.sender) revert JurorHasToOwnTheVote(); + if (round.votes[_voteIDs[i]].commit == _commit) continue; // Skip if committing the same value if (round.votes[_voteIDs[i]].commit == bytes32(0)) { commitCount++; } round.votes[_voteIDs[i]].commit = _commit; }
277-278: Consider adding inline documentation.The comment "Introduce a counter so we don't count a re-commited votes" is helpful. Consider expanding it to explain the invariant being maintained:
totalCommittedrepresents the number of unique jurors who have committed at least once, not the total number ofcastCommitcalls.- // Introduce a counter so we don't count a re-commited votes. + // Track how many votes are being committed for the first time in this call. + // totalCommitted represents unique votes committed, not total castCommit calls. uint256 commitCount;contracts/test/foundry/KlerosCore_Voting.t.sol (1)
83-92: Consider adding edge case test coverage.While the current test validates the core re-commit behavior, consider adding test cases for:
- Re-committing multiple voteIDs where some are new and some are re-commits
- Re-committing with the same hash (to verify idempotency)
- Verifying that
CommitCastevent is emitted correctly on re-commitExample test for mixed scenario:
// Test re-committing with a mix of new and already-committed voteIDs voteIDs = new uint256[](2); voteIDs[0] = 0; // Already committed voteIDs[1] = 1; // Not yet committed vm.prank(staker1); disputeKit.castCommit(disputeID, voteIDs, newCommit); (, , , totalCommited, , ) = disputeKit.getRoundInfo(disputeID, 0, 0); assertEq(totalCommited, 2, "totalCommited should be 2 (1 existing + 1 new)");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(1 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(2 hunks)
⏰ 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)
- GitHub Check: Redirect rules - kleros-v2-neo
- 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-neo
- 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-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: SonarCloud
🔇 Additional comments (2)
contracts/test/foundry/KlerosCore_Voting.t.sol (2)
38-38: LGTM: NO constant improves test readability.The introduction of the
NOconstant (value 0) alongside the existingYESconstant makes the test more readable and semantically clear, especially in the re-commit test scenario.
83-92: LGTM: Test correctly validates re-commit behavior.This test segment properly verifies the fix:
- ✓ Confirms
totalCommittedremains 1 after re-committing the same voteID- ✓ Validates that the stored commit value is updated to the new hash
- ✓ Demonstrates that jurors can change their commit during the commit period
The test aligns with the implementation in
DisputeKitClassicBase.soland provides good coverage for the re-commit scenario.
|
PR-Codex overview
This PR focuses on improving the voting mechanism in the
DisputeKitClassicBasecontract by introducing a counter to prevent double counting of committed votes. It also includes tests to verify that re-committing a vote does not increase the total committed count.Detailed summary
commitCountvariable to track unique committed votes inDisputeKitClassicBase.round.totalCommittedbycommitCountinstead of the length of_voteIDs.KlerosCore_VotingTestto ensure re-committing a vote does not increase the total committed count.Summary by CodeRabbit
Bug Fixes
Tests