Removal of externalDisputeID #2169
Conversation
jaybuidl
added
Type: Enhancement ✨
Compatibility: ABI change 🗯
WalkthroughThis PR removes the external/local dispute ID from events and APIs across arbitration, evidence, and gateway contracts. Event and function parameters are updated to use the arbitrator dispute ID where applicable. Interfaces and tests are aligned with the new event shapes. A Ruling event is added to IArbitrableV2. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant A as Arbitrable Contract
participant ARB as IArbitratorV2
participant U as User
U->>A: createDispute(..., templateId)
A->>ARB: createDispute(choices, extraData) [value: cost]
ARB-->>A: returns arbitratorDisputeID
A-->>A: update local state
A-->>U: emit DisputeRequest(arbitrator, arbitratorDisputeID, templateId)
U->>A: submitEvidence(arbitratorDisputeID, evidence)
A-->>U: emit Evidence(arbitratorDisputeID, user, evidence)
ARB-->>A: rule(disputeID, ruling)
A-->>U: emit Ruling(arbitrator, disputeID, ruling)
sequenceDiagram
autonumber
participant FG as Foreign Chain Arbitrable
participant HG as HomeGateway
participant ARB as Home Arbitrator
FG->>HG: relayCreateDispute(params: (blockHash, chainID, arbitrable, foreignDisputeID, templateId, choices, extraData))
HG->>ARB: createDispute(choices, extraData) [value: cost]
ARB-->>HG: returns disputeID
HG-->>FG: emit CrossChainDisputeIncoming(arbitrator, foreignChainID, foreignArbitrable, foreignDisputeID, disputeID, templateId)
HG-->>FG: emit DisputeRequest(arbitrator, disputeID, templateId)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (6)
contracts/test/evidence/index.ts (2)
265-268: Destructure only the emitted args (3) for DisputeRequest.The event now emits 3 params; drop the unused fourth binding for clarity.
- let [_arbitrator, _disputeID, _templateId, _templateUri] = getEmittedEvent("DisputeRequest", receipt).args; + let [_arbitrator, _disputeID, _templateId] = getEmittedEvent("DisputeRequest", receipt).args;
170-180: Rename test title to remove “external dispute id”.Terminology drift; suggest “same evidence twice for the same evidence group ID” or similar.
contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol (2)
73-84: Event rename is correct; add note about pre-dispute sentinel.ModeratedEvidence now uses _arbitratorDisputeID. Please extend the NatSpec to state that it can be 0 when emitted before a dispute exists.
217-218: Pre-dispute emission uses 0 as _arbitratorDisputeID.Intentional and matches tests. Consider documenting this behavior in the contract header to guide indexers/clients.
contracts/test/integration/index.ts (2)
115-115: Avoid magic number; assert with the decoded disputeId.Use the decoded disputeId variable for robustness.
- await expect(tx).to.emit(arbitrable, "DisputeRequest").withArgs(foreignGateway.target, 1, 0); + await expect(tx).to.emit(arbitrable, "DisputeRequest").withArgs(foreignGateway.target, disputeId, 0);
128-139: Strengthen the HomeGateway DisputeRequest assertion.Currently only checks that it emits. Also validate key args to catch regressions.
- ).to.emit(homeGateway, "DisputeRequest"); + ).to.emit(homeGateway, "DisputeRequest") + // Arbitrator should be the core; other args can be matched loosely if dynamic. + .withArgs(core.target, anyValue, anyValue);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
contracts/src/arbitration/KlerosGovernor.sol(1 hunks)contracts/src/arbitration/arbitrables/ArbitrableExample.sol(2 hunks)contracts/src/arbitration/arbitrables/DisputeResolver.sol(1 hunks)contracts/src/arbitration/devtools/DisputeResolverRuler.sol(1 hunks)contracts/src/arbitration/evidence/EvidenceModule.sol(1 hunks)contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol(3 hunks)contracts/src/arbitration/interfaces/IArbitrableV2.sol(1 hunks)contracts/src/arbitration/interfaces/IEvidence.sol(1 hunks)contracts/src/gateway/HomeGateway.sol(2 hunks)contracts/src/gateway/interfaces/IHomeGateway.sol(0 hunks)contracts/test/arbitration/draw.ts(1 hunks)contracts/test/arbitration/ruler.ts(3 hunks)contracts/test/evidence/index.ts(2 hunks)contracts/test/integration/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- contracts/src/gateway/interfaces/IHomeGateway.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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (15)
contracts/src/arbitration/interfaces/IEvidence.sol (1)
8-11: LGTM! Parameter rename improves clarity.The rename from
_externalDisputeIDto_arbitratorDisputeIDaccurately reflects that this is the internal arbitrator's dispute identifier, not an external/local ID. The documentation and event signature are updated consistently.contracts/src/gateway/HomeGateway.sol (1)
194-203: Consistent event signature update in the ERC20 fee overload.The same event signature changes are correctly applied in the ERC20 fee payment overload, maintaining consistency across both
relayCreateDisputevariants.contracts/src/arbitration/KlerosGovernor.sol (1)
306-306: LGTM! Event emission updated to match the new 3-parameter signature.The
DisputeRequestevent emission has been correctly updated to remove the session index parameter, aligning with the updatedIArbitrableV2interface.contracts/test/arbitration/ruler.ts (3)
100-102: LGTM! Test expectations correctly updated.The test correctly expects the new 3-parameter
DisputeRequestevent signature(arbitrator, disputeID, templateId), removing thelocalDisputeIDparameter.
124-126: LGTM! Consistent test update.Test expectations correctly updated to match the new event signature.
142-142: LGTM! Consistent test update.Test expectations correctly updated to match the new event signature.
contracts/test/arbitration/draw.ts (1)
157-169: LGTM! Test updated to match the new RelayCreateDisputeParams structure.The
relayCreateDisputecall has been correctly updated to use a 7-parameter tuple instead of 8, removing theexternalDisputeIDfield. The tuple structure now matches:
foreignBlockHash(bytes32)foreignChainID(uint256)foreignArbitrable(address)foreignDisputeID(uint256)templateId(uint256)choices(uint256)extraData(bytes)contracts/src/arbitration/arbitrables/ArbitrableExample.sol (2)
126-126: LGTM! Event emission updated to the new 3-parameter signature.The
DisputeRequestevent emission has been correctly updated to emit(arbitrator, disputeID, templateId)instead of the previous 4-parameter signature.
146-146: LGTM! Consistent update in the ERC20 fee overload.The event emission is correctly updated in the ERC20 fee payment variant as well, maintaining consistency across both
createDisputeoverloads.contracts/src/arbitration/devtools/DisputeResolverRuler.sol (1)
49-49: LGTM! Event emission updated correctly.The
DisputeRequestevent emission has been updated to the new 3-parameter signature(arbitrator, arbitratorDisputeID, templateId), removing thelocalDisputeIDparameter.contracts/src/arbitration/evidence/EvidenceModule.sol (1)
61-64: LGTM! Consistent parameter rename throughout the function.The parameter has been consistently renamed from
_externalDisputeIDto_arbitratorDisputeIDin the documentation, function signature, and event emission, aligning with the updatedIEvidenceinterface.contracts/src/arbitration/arbitrables/DisputeResolver.sol (1)
137-137: DisputeRequest arity aligned; looks good.Emission matches the updated IArbitrableV2 event (arbitrator, arbitratorDisputeID, templateId).
contracts/test/evidence/index.ts (1)
157-160: Assertion update is correct.Using _disputeID=0 for pre-dispute ModeratedEvidence aligns with the new event semantics.
If intended, please document that pre-dispute emissions use 0 as a sentinel for _arbitratorDisputeID, so indexers don’t misinterpret it as a real dispute ID.
contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol (1)
268-268: DisputeRequest emission updated; looks good.Three-argument payload (arbitrator, disputeID, templateId) aligns with IArbitrableV2.
contracts/src/arbitration/interfaces/IArbitrableV2.sol (1)
21-21: No leftover references to externalDisputeID/evidenceGroupID or old DisputeRequest signature All tests and contracts now use the updated 3-argument event.
✅ 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-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? →
|
1 participant
Resolves #2168
PR-Codex overview
This PR focuses on simplifying the
DisputeRequestevent by removing the_externalDisputeIDparameter across multiple contracts, improving code clarity and consistency while retaining the essential functionality.Detailed summary
_externalDisputeIDfromDisputeRequestinKlerosGovernor.sol,DisputeResolver.sol,DisputeResolverRuler.sol,ArbitrableExample.sol, andHomeGateway.sol.IArbitrableV2.solandIEvidence.solto reflect the removal of_externalDisputeID.submitEvidenceto use_arbitratorDisputeIDinstead of_externalDisputeID.draw.ts,ruler.ts, andindex.tsto align with the new event structure and removed references to_externalDisputeID.Summary by CodeRabbit