feat(DK): pass commit and appeal quicker #1955
Conversation
WalkthroughThis pull request enhances the arbitration contracts by tightening the appeal period conditions. In the KlerosCoreBase contract, the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant KlerosCore as KlerosCoreBase
participant DisputeKit
Caller->>KlerosCore: call passPeriod(disputeID)
KlerosCore->>KlerosCore: check elapsed appeal period time
KlerosCore->>DisputeKit: call isAppealFunded(disputeID)
DisputeKit-->>KlerosCore: return appeal funding status (bool)
alt Appeal period not passed and appeal not funded
KlerosCore->>Caller: revert AppealPeriodNotPassed
else Conditions met
KlerosCore->>Caller: transition period and emit NewPeriod event
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
![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 (3)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
534-546: Good implementation of appeal funding check logic.The implementation of
appealFundingIsFinishedcorrectly:
- Retrieves the appeal period's start and end times
- Checks for funded choices
- Determines if funding is finished based on elapsed time and the absence of any funded choices
The time check uses
LOSER_APPEAL_PERIOD_MULTIPLIER(5000 basis points = 50%) to allow for quicker transitions after half the appeal period has elapsed if no funding has occurred.However, consider expanding this implementation to also handle the case where only one side has funded. Currently, it only returns true when no choices are funded at all, but there could be scenarios where only the winning side funded and it's clear the losing side won't fund.
function appealFundingIsFinished(uint256 _coreDisputeID) external view override returns (bool) { (uint256 appealPeriodStart, uint256 appealPeriodEnd) = core.appealPeriod(_coreDisputeID); uint256[] memory fundedChoices = getFundedChoices(_coreDisputeID); - return (fundedChoices.length == 0 && + // If no choices are funded, check if loser's time window has passed + return ((fundedChoices.length == 0 && block.timestamp - appealPeriodStart >= - ((appealPeriodEnd - appealPeriodStart) * LOSER_APPEAL_PERIOD_MULTIPLIER) / ONE_BASIS_POINT); + ((appealPeriodEnd - appealPeriodStart) * LOSER_APPEAL_PERIOD_MULTIPLIER) / ONE_BASIS_POINT) + // OR if only one choice is funded and loser's time window has passed + || (fundedChoices.length == 1 && + block.timestamp - appealPeriodStart >= + ((appealPeriodEnd - appealPeriodStart) * LOSER_APPEAL_PERIOD_MULTIPLIER) / ONE_BASIS_POINT)); }contracts/test/foundry/KlerosCore.t.sol (2)
1880-1936: Test for quick period transition when all committed votes are castThis test verifies a new feature where the voting period can be passed to appeal phase when all committed votes are cast, even if not all jurors have voted. This aligns with the PR objective of passing periods quicker.
Consider adding an additional test for the edge case where some votes are committed but not cast, to ensure the period cannot be passed in that scenario.
2248-2280: Test for quick appeal period transitionThis test validates that the appeal period can be passed to execution without waiting for the full duration, meeting the PR objective of accelerating period transitions.
Consider adding a test for the edge case where an appeal is funded on one side but not the other, to ensure the behavior is correct in that scenario as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/src/arbitration/KlerosCoreBase.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(1 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)contracts/test/foundry/KlerosCore.t.sol(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-neo
- 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: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
contracts/src/arbitration/interfaces/IDisputeKit.sol (1)
95-98: Good addition of theappealFundingIsFinishedmethod to the interface.This new method provides a clean way to check if appeal funding is completed prematurely. The documentation clearly explains its purpose and parameters.
contracts/src/arbitration/KlerosCoreBase.sol (1)
578-581: Good enhancement to the appeal period check logic.This modification to the
passPeriodfunction now checks both time elapsed AND appeal funding status before preventing a period change. This allows for quicker transitions through the appeal period when funding won't be completed.The logical operator changed from a simple time check to a more complex condition that enables skipping the remaining appeal period when appropriate.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
520-521: Good documentation clarification.Adding this note helps clarify that the function is intended for direct use by the core contract rather than off-chain usage, which is important context for maintainers.
527-531: Good improvement to vote counting logic to handle hidden votes.The function now correctly differentiates between hidden and non-hidden votes when determining if all votes have been cast. For hidden votes, it checks against
totalCommitted, while for non-hidden votes, it usesvotes.length.This will lead to more accurate vote counting, especially in courts with different voting visibility settings.
contracts/test/foundry/KlerosCore.t.sol (1)
1804-1805: Test ensures voting period can't be passed prematurelyGood addition to verify that the contract correctly prevents passing the period before all voting conditions are met.
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jaybuidl
added
Type: Feature🗿
Package: Contracts
|
Code Climate has analyzed commit 5f8e25c and detected 0 issues on this pull request. View more on Code Climate. |
|
PR-Codex overview
This PR focuses on enhancing the appeal process in the
KlerosCoreBasecontract by adding checks for appeal funding and modifying voting mechanisms in dispute kits.Detailed summary
KlerosCoreBaseto ensure appeal funding is completed before moving to the execution period.isAppealFundedfunction inIDisputeKitto verify if appeal funding is finished.areVotesAllCastinDisputeKitClassicBaseto account for hidden votes.isAppealFundedto check if the appeal period has ended.KlerosCoreTestfor quick passing of periods during voting and appeal phases.Summary by CodeRabbit
New Features
Tests