Refactor the Dispute Kit checks: active dispute and isJumped #2174
Conversation
…Rewards(), DK errors renamed
jaybuidl
added
Type: Enhancement ✨
Compatibility: ABI change 🗯
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces an Active struct for dispute tracking (dispute + currentRound), replaces per-dispute jumped flag and notJumped modifier with isActive, updates createDispute signature (adds _nbVotes), aggregates withdrawal logic across rounds and simplifies Withdrawal event, and removes notJumped guards from Shutter castCommit functions; tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Core as KlerosCore
participant DK as DisputeKitClassicBase
participant DKs as DisputeKitShutter*
participant Juror
participant Funder as Crowdfunder
Note over Core,DK: Dispute creation (new API)
Core->>DK: createDispute(coreDisputeID, choices, extraData, nbVotes)
DK->>DK: set coreDisputeIDToActive[coreDisputeID] = Active{dispute=true, currentRound=true}
DK-->>Core: local dispute initialized / reused
Note over Core,DK: Voting / Commit / Draw
Juror->>DKs: castCommitShutter(coreDisputeID, ...)
DKs->>DK: _castCommit(...) -> DK.isActive(coreDisputeID)? (dispute && currentRound)
DK-->>Juror: accept/reject commit
Note over Core,DK: Appeal funding and jumping
Funder->>Core: fundAppeal(coreDisputeID,...)
Core->>DK: fundAppeal(...)
DK->>DK: If jump -> coreDisputeIDToActive[coreDisputeID].currentRound = false
DK-->>Core: appeal result / jump noted
Note over DK,Funder: Withdrawal (aggregated)
Funder->>DK: withdrawFeesAndRewards(disputeID, recipient, round)
DK->>DK: iterate rounds, compute reimbursements, clear contributions
DK-->>Funder: emit Withdrawal(disputeID, round, recipient, amount)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
jaybuidl
changed the title
jaybuidl
changed the title
jaybuidl
changed the title
jaybuidl
force-pushed
the
feat/recurring-dk-test
branch
from
1e7d185 to
536e61c
Compare
![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)
56-75: Blocking: newActivelayout corrupts existing stateUpgrading the proxy with
coreDisputeIDToActivenow storingActive(two bools) instead of a single bool is not storage-compatible. All legacy entries were0x01(meaning “true”). After the upgrade, that same slot decodes todispute = truebutcurrentRound = false. BecauseisActivenow requires both flags, every dispute that was mid-round before the upgrade will immediately revert withDisputeJumpedToAnotherDisputeKit, preventing draws, commits, votes, and appeals. There is no way to backfillcurrentRoundfor those mappings on-chain.Please preserve the old storage contract (e.g., keep the original bool, or derive
currentRoundlazily while distinguishing real jumps) or provide a safe migration path that setscurrentRoundfor all active disputes before enforcing the new check. As written, this change bricks live disputes post-upgrade.
🧹 Nitpick comments (2)
contracts/test/foundry/KlerosCore_Appeals.t.sol (2)
586-589: Initial active/currentRound semantics — LGTMGood use of (disputeActive, currentRound) to assert both dispute visibility and round ownership.
Consider extracting a tiny helper in the test base to assert (active,current) pairs to reduce repetition across tests.
716-719: Fix misleading comment: argument is a core dispute ID, not a local oneThe function coreDisputeIDToActive expects a core dispute ID. The inline comment says “local dispute id”.
Apply:
- (disputeActive, currentRound) = disputeKit3.coreDisputeIDToActive(0); // local dispute id + (disputeActive, currentRound) = disputeKit3.coreDisputeIDToActive(0); // core dispute id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(12 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(1 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(15 hunks)contracts/test/foundry/KlerosCore_Disputes.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(2 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(2 hunks)
🔇 Additional comments (16)
contracts/test/foundry/KlerosCore_Appeals.t.sol (16)
301-302: Active-state check on jumped DK — LGTMCorrectly asserts currentRound is false on the DK the dispute jumped from.
315-316: Active DK currentRound assertion — LGTMCorrectly asserts the new DK has currentRound = true.
320-320: Renamed error selector usage — LGTMUpdated to DisputeJumpedToAnotherDisputeKit as per refactor.
439-440: Post-jump active-state on dk3 — LGTMAsserting currentRound = false on the old DK is correct.
453-454: Active DK switched — LGTMCorrect currentRound = true on the destination DK.
458-458: Error rename on draw guard — LGTMExpecting DisputeJumpedToAnotherDisputeKit is correct after the jump.
624-627: After first jump: dispute active but round jumped — LGTMAsserting active=true, currentRound=false is correct for the source DK.
643-646: Destination DK active/current — LGTMAsserting active=true, currentRound=true is correct on the new DK.
652-652: Draw from outdated DK reverts — LGTMCorrectly guarded by DisputeJumpedToAnotherDisputeKit.
665-665: castVote on outdated DK reverts — LGTMCorrect revert selector used.
674-675: fundAppeal on outdated DK reverts — LGTMMatches the new unknown/jumped semantics.
692-695: After second jump: dk2 round jumped — LGTMactive=true, currentRound=false is correct.
730-730: Draw from outdated DK reverts — LGTMSelector update is consistent.
743-743: castVote from outdated DK reverts — LGTMSelector update is consistent.
757-775: Withdrawal flow: signature change and error rename — looks right
- Updated withdrawFeesAndRewards(disputeID, recipient, choice) usage is correct.
- Withdrawal amounts and DK targeting align with active/currentRound semantics.
- Reverts with DisputeUnknownInThisDisputeKit on an unrelated DK are correct.
If handy, add quick asserts around DK balances post-withdrawals to ensure funds moved as expected and no dust remains due to rounding:
// e.g., after the two withdrawals: assertEq(address(disputeKit3).balance, /* expected */); assertEq(address(disputeKit2).balance, /* expected */);Optionally, also assert no event is emitted for zero-withdrawal paths to lock event semantics.
826-826: Updated disputes() return shape — LGTMTuple destructuring matches the new getter.
|
2 participants
Made the distinction more explicit between
Removed the parameter
_coreRoundIDfromwithdrawFeesAndRewards()which was responsible for several edge cases as seen in #2159 and in a tentative fix 1e7d185.Renamed the errors for clarity:
PR-Codex overview
This PR enhances the
DisputeKitClassicBasecontract and its derived contracts by improving the handling of dispute states and active rounds. It introduces a newActivestruct for tracking dispute activity and modifies several functions to ensure correct dispute state management.Detailed summary
Activestruct to track dispute and current round status.jumpedboolean withActivestruct incoreDisputeIDToActivemapping.notJumpedtoisActiveto check dispute status.Withdrawalparameters for clarity.Summary by CodeRabbit
New Features
Refactor
Documentation
Tests