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. |
WalkthroughAdds a revert guard in DisputeKitClassicBase.castCommit to prevent re-committing already committed votes and declares a new error AlreadyCommittedThisVote(). Updates a Foundry test to assert the revert on double-commit attempts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Juror
participant DisputeKit as DisputeKitClassicBase
participant Storage as Vote Storage
rect rgb(235, 245, 255)
note over Juror,DisputeKit: Initial commit
Juror->>DisputeKit: castCommit(voteIDs[], commit)
DisputeKit->>Storage: Read votes
DisputeKit->>Storage: Store commit for each vote
DisputeKit-->>Juror: Success
end
rect rgb(255, 240, 240)
note over Juror,DisputeKit: Re-commit attempt (changed flow)
Juror->>DisputeKit: castCommit(same voteIDs[], same commit)
DisputeKit->>Storage: Read vote.commit
alt vote.commit != 0
DisputeKit-->>Juror: revert AlreadyCommittedThisVote()
else
DisputeKit->>Storage: Store commit
DisputeKit-->>Juror: Success
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
PR-Codex overview
This PR introduces a new error handling mechanism in the
DisputeKitClassicBasecontract to prevent users from committing votes multiple times. It also adds a test case to ensure that the proper error is thrown when a user attempts to commit a vote they have already committed.Detailed summary
castCommitfunction to revert withAlreadyCommittedThisVoteif a vote has already been committed.KlerosCore_Voting.t.solto verify that committing an already committed vote triggers the expected revert.Summary by CodeRabbit
Bug Fixes
Tests