docs: update and expand V2 contract specifications#2261
Open
blaisebuidl wants to merge 2 commits intokleros:devfrom
Open
docs: update and expand V2 contract specifications#2261blaisebuidl wants to merge 2 commits intokleros:devfrom
blaisebuidl wants to merge 2 commits intokleros:devfrom
Conversation
Updated specifications: - arbitrator.md: fix execute/executeRuling separation, correct function signatures, add error conditions table, separate public/internal sections - sortition-module.md: add missing 'generating' phase, fix tree navigation formula, add phase transition conditions, add error conditions - courts.md: remove hallucinated DEFAULT_K constant, document __gap storage slots - dispute-kit-classic.md: clarify base class validation, add error conditions - evidence-format.md: add off-chain protocol requirement markers - All files: add versioning headers New specifications: - dispute-kit-overview.md: dispute kit architecture and interface - dispute-kit-shutter.md: Shutter threshold encryption dispute kit - dispute-kit-gated.md: gated dispute kit (token/NFT requirements) - dispute-kit-sybil-resistant.md: sybil-resistant dispute kit (PoH) Reviewed by: automated bad cop (2 rounds, 0 critical issues remaining)
👷 Deploy request for kleros-v2-testnet pending review.Visit the deploys page to approve it
|
👷 Deploy request for kleros-v2-neo pending review.Visit the deploys page to approve it
|
👷 Deploy request for kleros-v2-testnet-devtools pending review.Visit the deploys page to approve it
|
Contributor
WalkthroughRegistry-backed, template-driven expansion of Kleros V2 specifications: adds a template system and template-backed dispute flow, introduces three new dispute-kit specs (Gated, Shutter, Sybil-Resistant), replaces/extends sortition/staking with K-ary trees and autostaking, and enriches courts, arbitrator, evidence, and policy docs with governance, eligibility, and security guidance. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant TemplateRegistry
participant KlerosCore
participant Arbitrator
User->>UI: submit dispute (templateTag + data)
UI->>TemplateRegistry: resolve templateTag -> templateId
TemplateRegistry-->>UI: templateId, templateDataMappings
UI->>KlerosCore: createDisputeForTemplate(templateId, extraData)
KlerosCore->>Arbitrator: create dispute on arbitrator (arbitratorExtraData)
Arbitrator-->>KlerosCore: arbitratorDisputeID
KlerosCore-->>UI: DisputeRequest(arbitrator, arbitratorDisputeID, templateId)
Arbitrator->>KlerosCore: rule(arbitratorDisputeID, ruling)
KlerosCore-->>UI: Ruling(disputeID, ruling, templateContext)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/specifications/arbitrator.md`:
- Line 603: The heading "#### _getCompatibleNextRoundSettings" is at H4
immediately after an H2 ("## Internal Mechanics"), breaking the heading
hierarchy; change it to an H3 ("### _getCompatibleNextRoundSettings") so heading
levels progress correctly, and scan nearby headings in this section (e.g., other
headings under "Internal Mechanics") to ensure consistent H3/H4 usage.
- Around line 192-193: The doc incorrectly states the fallback for invalid DK
output as “current court + Classic DK”; update both occurrences that describe
the fallback for "Invalid Settings" (the diagram/text around the "Fallback"
description and the paragraph at the second occurrence) to say the fallback is
"current court + current dispute kit + default nbVotes", and add a clarifying
note that Classic DK is only chosen if court/dispute kit compatibility still
fails after that fallback; ensure the two spots that currently mention "Classic
DK" are replaced with this corrected fallback text.
- Around line 222-229: The spec example calls
disputeKits[disputeKitID].getNextRoundSettings with the wrong argument for the
jump-threshold: it passes _round.jurorsForCourtJump but the implementation
expects the court-level field _court.jurorsForCourtJump; update the example call
to pass _court.jurorsForCourtJump (replace the 4th argument) so
getNextRoundSettings receives the correct court-level jump threshold and
preserves the intended semantics of disputeKitID/newCourtID/newRoundNbVotes
resolution.
In `@contracts/specifications/courts.md`:
- Around line 197-199: Update the specification bullet that lists token
standards so it no longer includes ERC-20; specifically replace the line
"Requires specific token holdings (ERC20/721/1155)" with wording that only
references ERC-721 and ERC-1155 to match the gated DK implementation and avoid
implying ERC-20 support, and ensure related bullets like "Configurable token
requirements per court" and "Enables specialized expert juries" remain
consistent with ERC-721/ERC-1155 gating semantics.
- Around line 191-194: The description for "Sybil-Resistant Dispute Kit (ID: 2)"
incorrectly claims "equal drawing chances regardless of stake"; update the
specification text to state that juror selection uses stake-weighted sortition
while still requiring Proof of Humanity registration (or being optional for
courts that require human identity verification), e.g., replace the "equal
drawing chances..." bullet with a clear statement that drawing chances are
weighted by stake per the V2 sortition model and keep the Proof of Humanity
requirement/optional note consistent with the rest of the V2 dispute-kit
documentation (refer to the "Sybil-Resistant Dispute Kit (ID: 2)" block and the
stake-weighted sortition wording elsewhere).
- Around line 264-265: The spec incorrectly states that eligibility is checked
in SortitionModule.validateStake(); update the documentation to match the
current API by either (a) removing or changing the sentence to reflect that
validateStake() currently does not accept an eligibility contract and therefore
does not perform that check, or (b) explicitly mark the eligibility check as
"future work" and note where it should be performed once implemented (e.g., a
future SortitionModule.validateStake(eligibilityContract) or external
eligibility contract call). Mention the exact symbol
SortitionModule.validateStake in the edit so reviewers can trace the claim to
the current API.
In `@contracts/specifications/dispute-kit-classic.md`:
- Line 29: The TOC link currently written as [🏗️ Architecture](`#-architecture`)
does not match the actual heading anchor for the heading "## 🏗️ Architecture";
update the fragment part of the link to exactly match the generated anchor for
that heading (replace "(`#-architecture`)" in the TOC line with the correct slug
produced by your markdown renderer/host so the link resolves), then verify the
link by rendering or using markdownlint/anchor generation to confirm it points
to the "## 🏗️ Architecture" section.
- Around line 102-103: The spec currently claims draws are "Deterministic" for
the same dispute + nonce but the RNG example uses block.timestamp, creating a
contradiction; update the spec to either remove time-dependent RNG (replace
block.timestamp with a deterministic seed derived solely from dispute ID and
nonce, e.g., dispute ID || nonce || blockhash(block.number - 1) or a canonical
on-chain seed) or change the determinism claim to "Time-dependent" and
explicitly document the use of block.timestamp; reference the terms
"Deterministic", "nonce", and the example using "block.timestamp" (and any
example RNG formula) when making the change so the language and example are
consistent.
In `@contracts/specifications/dispute-kit-overview.md`:
- Around line 170-174: The pseudocode incorrectly uses block.timestamp as the
randomness source; replace that with the protocol RNG output and show hashing of
disputeID and nonce with that RNG output. Update the lines referencing
randomNumber, hash(disputeID, nonce, block.timestamp) and targetPosition to
instead derive randomNumber from hash(disputeID, nonce, protocolRngOutput) (or
equivalent named protocol RNG value used elsewhere) and then compute
targetPosition = randomNumber % totalStaked so the doc matches the actual
protocol RNG-based draw.
In `@contracts/specifications/evidence-format.md`:
- Around line 203-206: The spec currently presents JSON validity, required
fields, and fileURI format as mandatory but they are not enforced by the
on-chain EvidenceModule; update the text in evidence-format.md to explicitly
state these are off-chain protocol/implementation conventions (e.g.,
frontend/indexer requirements) and not contract-enforced rules, change wording
from mandatory to "Off-chain requirement" or "Recommendation", and add a short
note referencing EvidenceModule to clarify that enforcement must be implemented
outside the smart contract.
- Around line 225-230: The access-control bullet is incorrect:
EvidenceModule.submitEvidence is permissionless and on-chain enforcement is not
performed; update the specification to state that submission is permissionless
on-chain (the contract emits an event with msg.sender) and that any access
restrictions must be enforced off-chain (UI/backend policy) rather than by the
smart contract; reference EvidenceModule.submitEvidence and msg.sender in the
revised text so readers know the contract records submitter identity but does
not restrict who can call it.
In `@contracts/specifications/policy-format.md`:
- Around line 168-170: The PolicyUpdate event in the spec is missing the
_courtName field and must exactly match the contract's event shape; update the
documented event signature for PolicyUpdate to include the _courtName parameter
(same type/order as in the contract) so indexers/integrations align with the
contract's emitted event.
- Around line 160-165: The spec's setPolicy declaration is missing the
_courtName parameter used by PolicyRegistry; update the function signature for
setPolicy to include string calldata _courtName (matching PolicyRegistry) and
ensure any related emit (e.g., PolicyUpdate) and uses inside setPolicy reference
_courtName along with _courtID and _policy so the spec API matches the contract
(look for setPolicy, PolicyRegistry, _courtID, _courtName, _policy, and
PolicyUpdate to update).
In `@contracts/specifications/sortition-module.md`:
- Around line 585-586: The spec line that says K defaults to DEFAULT_K conflicts
with the rest of the spec which requires K to be explicitly provided; update the
sentence referencing `_extraData`/`DEFAULT_K` so it no longer implies a default
— state that K must be supplied in `_extraData` (or, if you prefer defaults,
update all places that reference explicit K), and ensure related references
(e.g., `DEFAULT_K`, `K`, `_extraData`, and the key derivation
`CourtID.wrap(_courtID).toTreeKey()`) are consistent with the chosen rule.
- Around line 422-427: The DelayedStake struct is missing the alreadyTransferred
boolean referenced elsewhere (delayedStake.alreadyTransferred); update the
struct definition (DelayedStake) to include a bool alreadyTransferred field and
ensure any code that sets or reads delayedStake.alreadyTransferred uses that
field consistently (e.g., when creating/clearing delayed stakes in functions
handling delayedStake and any arrays/mappings that store DelayedStake
instances).
- Around line 591-597: The spec's validateStake signature is out of sync with
the implementation: remove the undocumented _eligibility parameter (or
alternatively add it to the implementation) so the specification matches the
actual function signature in SortitionModule.sol; specifically, update the
documentation's validateStake declaration to mirror the validateStake(...)
definition in contracts/src/arbitration/SortitionModule.sol (same parameters,
return values and modifiers such as onlyByCore) to avoid misleading integrators.
- Around line 119-154: In the set function the "direction" used when calling
_updateParents is computed after nodes[treeIndex] is overwritten so it is always
false; fix by capture the old value (e.g., uint256 old = nodes[treeIndex])
before assigning nodes[treeIndex] = _value, compute difference and the direction
flag using old (direction = _value > old), then call _updateParents(treeIndex,
difference, direction); ensure stack handling and deletes remain as-is around
the zero-stake branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19080eff-8b4b-4d0f-bc39-ba83bb435f18
📒 Files selected for processing (12)
contracts/specifications/arbitrable.mdcontracts/specifications/arbitrator.mdcontracts/specifications/courts.mdcontracts/specifications/dispute-kit-classic.mdcontracts/specifications/dispute-kit-gated.mdcontracts/specifications/dispute-kit-overview.mdcontracts/specifications/dispute-kit-shutter.mdcontracts/specifications/dispute-kit-sybil-resistant.mdcontracts/specifications/evidence-format.mdcontracts/specifications/policy-format.mdcontracts/specifications/sortition-module.mdcontracts/specifications/specifications.md
Comment on lines
+192
to
+193
| D -->|Invalid Settings| F[Fallback:<br/>Current Court + Classic DK<br/>+ Default nbVotes] | ||
|
|
Contributor
There was a problem hiding this comment.
Fallback behavior is documented incorrectly for invalid DK output.
At Line 192 and Line 242, invalid settings are described as falling back to “current court + Classic DK”. In implementation, invalid settings first fall back to current court + current dispute kit + default nbVotes; Classic DK is only forced if court/DK compatibility still fails.
🛠️ Suggested doc fix
- D -->|Invalid Settings| F[Fallback:<br/>Current Court + Classic DK<br/>+ Default nbVotes]
+ D -->|Invalid Settings| F[Fallback:<br/>Current Court + Current DK<br/>+ Default nbVotes]- - If DK returns invalid settings → fallback to current court + Classic DK
+ - If DK returns invalid settings → fallback to current court + current dispute kit (with default nbVotes)
- If parent court doesn't support new DK → force Classic DKAlso applies to: 242-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/arbitrator.md` around lines 192 - 193, The doc
incorrectly states the fallback for invalid DK output as “current court +
Classic DK”; update both occurrences that describe the fallback for "Invalid
Settings" (the diagram/text around the "Fallback" description and the paragraph
at the second occurrence) to say the fallback is "current court + current
dispute kit + default nbVotes", and add a clarifying note that Classic DK is
only chosen if court/dispute kit compatibility still fails after that fallback;
ensure the two spots that currently mention "Classic DK" are replaced with this
corrected fallback text.
Comment on lines
+222
to
+229
| (newCourtID, newDisputeKitID, newRoundNbVotes) = disputeKits[disputeKitID].getNextRoundSettings( | ||
| _disputeID, | ||
| _dispute.courtID, | ||
| _court.parent, | ||
| _round.jurorsForCourtJump, | ||
| disputeKitID, | ||
| _round.nbVotes | ||
| ); |
Contributor
There was a problem hiding this comment.
Incorrect getNextRoundSettings argument in the spec example.
At Line 226, the 4th argument is documented as _round.jurorsForCourtJump, but implementation uses _court.jurorsForCourtJump. This changes jump-threshold semantics in the spec.
🛠️ Suggested doc fix
(newCourtID, newDisputeKitID, newRoundNbVotes) = disputeKits[disputeKitID].getNextRoundSettings(
_disputeID,
_dispute.courtID,
_court.parent,
- _round.jurorsForCourtJump,
+ _court.jurorsForCourtJump,
disputeKitID,
_round.nbVotes
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (newCourtID, newDisputeKitID, newRoundNbVotes) = disputeKits[disputeKitID].getNextRoundSettings( | |
| _disputeID, | |
| _dispute.courtID, | |
| _court.parent, | |
| _round.jurorsForCourtJump, | |
| disputeKitID, | |
| _round.nbVotes | |
| ); | |
| (newCourtID, newDisputeKitID, newRoundNbVotes) = disputeKits[disputeKitID].getNextRoundSettings( | |
| _disputeID, | |
| _dispute.courtID, | |
| _court.parent, | |
| _court.jurorsForCourtJump, | |
| disputeKitID, | |
| _round.nbVotes | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/arbitrator.md` around lines 222 - 229, The spec
example calls disputeKits[disputeKitID].getNextRoundSettings with the wrong
argument for the jump-threshold: it passes _round.jurorsForCourtJump but the
implementation expects the court-level field _court.jurorsForCourtJump; update
the example call to pass _court.jurorsForCourtJump (replace the 4th argument) so
getNextRoundSettings receives the correct court-level jump threshold and
preserves the intended semantics of disputeKitID/newCourtID/newRoundNbVotes
resolution.
|
|
||
| The following are internal functions and implementation details that are subject to change. Do not depend on these externally: | ||
|
|
||
| #### _getCompatibleNextRoundSettings |
Contributor
There was a problem hiding this comment.
Heading level jumps from H2 to H4.
Line 603 uses a level-4 heading immediately after a level-2 section (## Internal Mechanics), which triggers MD001 and breaks heading hierarchy.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 603-603: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/arbitrator.md` at line 603, The heading "####
_getCompatibleNextRoundSettings" is at H4 immediately after an H2 ("## Internal
Mechanics"), breaking the heading hierarchy; change it to an H3 ("###
_getCompatibleNextRoundSettings") so heading levels progress correctly, and scan
nearby headings in this section (e.g., other headings under "Internal
Mechanics") to ensure consistent H3/H4 usage.
Comment on lines
+191
to
+194
| **Sybil-Resistant Dispute Kit (ID: 2)**: | ||
| - Requires Proof of Humanity registration | ||
| - Equal drawing chances regardless of stake | ||
| - Optional for courts requiring human identity verification |
Contributor
There was a problem hiding this comment.
Sybil-resistant drawing behavior is misstated.
Line 193 says “equal drawing chances regardless of stake,” which conflicts with the stake-weighted sortition model documented elsewhere for V2 dispute kits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/courts.md` around lines 191 - 194, The description
for "Sybil-Resistant Dispute Kit (ID: 2)" incorrectly claims "equal drawing
chances regardless of stake"; update the specification text to state that juror
selection uses stake-weighted sortition while still requiring Proof of Humanity
registration (or being optional for courts that require human identity
verification), e.g., replace the "equal drawing chances..." bullet with a clear
statement that drawing chances are weighted by stake per the V2 sortition model
and keep the Proof of Humanity requirement/optional note consistent with the
rest of the V2 dispute-kit documentation (refer to the "Sybil-Resistant Dispute
Kit (ID: 2)" block and the stake-weighted sortition wording elsewhere).
Comment on lines
+197
to
+199
| - Requires specific token holdings (ERC20/721/1155) | ||
| - Configurable token requirements per court | ||
| - Enables specialized expert juries |
Contributor
There was a problem hiding this comment.
Gated DK token standards are over-specified.
Line 197 includes ERC-20, but the gated spec and implementation focus on ERC-721/ERC-1155 gating. This should be narrowed to avoid incorrect integration assumptions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/courts.md` around lines 197 - 199, Update the
specification bullet that lists token standards so it no longer includes ERC-20;
specifically replace the line "Requires specific token holdings
(ERC20/721/1155)" with wording that only references ERC-721 and ERC-1155 to
match the gated DK implementation and avoid implying ERC-20 support, and ensure
related bullets like "Configurable token requirements per court" and "Enables
specialized expert juries" remain consistent with ERC-721/ERC-1155 gating
semantics.
Comment on lines
+168
to
+170
| ```solidity | ||
| event PolicyUpdate(uint96 indexed _courtID, string _policy); | ||
| ``` |
Contributor
There was a problem hiding this comment.
PolicyUpdate event signature is incomplete.
The documented event omits the _courtName field. It should match the contract event shape to keep indexers/integrations accurate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/policy-format.md` around lines 168 - 170, The
PolicyUpdate event in the spec is missing the _courtName field and must exactly
match the contract's event shape; update the documented event signature for
PolicyUpdate to include the _courtName parameter (same type/order as in the
contract) so indexers/integrations align with the contract's emitted event.
Comment on lines
+119
to
+154
| function set(bytes32 _key, uint256 _value, bytes32 _ID) internal { | ||
| // Find or create node position | ||
| uint256 treeIndex = IDsToNodeIndexes[_ID]; | ||
| if (treeIndex == 0) { | ||
| if (_value == 0) return; // Skip zero stakes for new entries | ||
|
|
||
| // Reuse vacant position or expand tree | ||
| if (stack.length > 0) { | ||
| treeIndex = stack[stack.length - 1]; | ||
| stack.pop(); | ||
| } else { | ||
| treeIndex = nodes.length; | ||
| nodes.push(0); | ||
| } | ||
|
|
||
| IDsToNodeIndexes[_ID] = treeIndex; | ||
| nodeIndexesToIDs[treeIndex] = _ID; | ||
| } | ||
|
|
||
| // Update node value and propagate to ancestors | ||
| uint256 difference = _value > nodes[treeIndex] | ||
| ? _value - nodes[treeIndex] | ||
| : nodes[treeIndex] - _value; | ||
|
|
||
| nodes[treeIndex] = _value; | ||
|
|
||
| // If stake is zero, mark position as vacant | ||
| if (_value == 0) { | ||
| stack.push(treeIndex); | ||
| delete IDsToNodeIndexes[_ID]; | ||
| delete nodeIndexesToIDs[treeIndex]; | ||
| } | ||
|
|
||
| // Propagate changes up the tree | ||
| _updateParents(treeIndex, difference, _value > nodes[treeIndex]); | ||
| } |
Contributor
There was a problem hiding this comment.
Tree update pseudocode computes the direction flag incorrectly.
At Line 153, _value > nodes[treeIndex] is evaluated after nodes[treeIndex] was set at Line 143, so it is always false. This breaks the described parent-update behavior.
Proposed doc fix
- uint256 difference = _value > nodes[treeIndex]
- ? _value - nodes[treeIndex]
- : nodes[treeIndex] - _value;
+ uint256 oldValue = nodes[treeIndex];
+ bool isIncrease = _value > oldValue;
+ uint256 difference = isIncrease ? _value - oldValue : oldValue - _value;
@@
- _updateParents(treeIndex, difference, _value > nodes[treeIndex]);
+ _updateParents(treeIndex, difference, isIncrease);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/sortition-module.md` around lines 119 - 154, In the
set function the "direction" used when calling _updateParents is computed after
nodes[treeIndex] is overwritten so it is always false; fix by capture the old
value (e.g., uint256 old = nodes[treeIndex]) before assigning nodes[treeIndex] =
_value, compute difference and the direction flag using old (direction = _value
> old), then call _updateParents(treeIndex, difference, direction); ensure stack
handling and deletes remain as-is around the zero-stake branch.
Comment on lines
422
to
427
| ```solidity | ||
| struct DelayedStake { | ||
| address account; // The juror's address | ||
| uint96 courtID; // The court ID | ||
| uint256 stake; // The new stake amount | ||
| bool alreadyTransferred; // Whether tokens were already transferred | ||
| address account; // The juror's address | ||
| uint96 courtID; // The court ID | ||
| uint256 stake; // The new stake amount | ||
| } |
Contributor
There was a problem hiding this comment.
DelayedStake struct and usage are internally inconsistent.
Line 436 uses delayedStake.alreadyTransferred, but the struct at Line 423 does not define alreadyTransferred. The spec snippets should compile conceptually as-is.
Proposed doc fix
struct DelayedStake {
address account; // The juror's address
uint96 courtID; // The court ID
uint256 stake; // The new stake amount
+ bool alreadyTransferred; // Whether deposit transfer already happened during delay
}Also applies to: 434-439
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/sortition-module.md` around lines 422 - 427, The
DelayedStake struct is missing the alreadyTransferred boolean referenced
elsewhere (delayedStake.alreadyTransferred); update the struct definition
(DelayedStake) to include a bool alreadyTransferred field and ensure any code
that sets or reads delayedStake.alreadyTransferred uses that field consistently
(e.g., when creating/clearing delayed stakes in functions handling delayedStake
and any arrays/mappings that store DelayedStake instances).
Comment on lines
+585
to
+586
| - Extracts K value from `_extraData` (default: `DEFAULT_K`) | ||
| - Key derived from court ID using `CourtID.wrap(_courtID).toTreeKey()` |
Contributor
There was a problem hiding this comment.
DEFAULT_K reference conflicts with the rest of the updated spec set.
Line 585 says K defaults to DEFAULT_K, but the docs elsewhere state K must be explicitly provided. Keep one rule to avoid configuration ambiguity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/sortition-module.md` around lines 585 - 586, The
spec line that says K defaults to DEFAULT_K conflicts with the rest of the spec
which requires K to be explicitly provided; update the sentence referencing
`_extraData`/`DEFAULT_K` so it no longer implies a default — state that K must
be supplied in `_extraData` (or, if you prefer defaults, update all places that
reference explicit K), and ensure related references (e.g., `DEFAULT_K`, `K`,
`_extraData`, and the key derivation `CourtID.wrap(_courtID).toTreeKey()`) are
consistent with the chosen rule.
Comment on lines
+591
to
+597
| function validateStake( | ||
| address _account, | ||
| uint96 _courtID, | ||
| uint256 _newStake, | ||
| bool _noDelay, | ||
| ICourtEligibility _eligibility | ||
| ) external override onlyByCore returns (uint256 pnkDeposit, uint256 pnkWithdrawal, StakingResult stakingResult) |
Contributor
There was a problem hiding this comment.
validateStake signature is out of sync with the contract.
Line 591 documents an _eligibility parameter, but the current contracts/src/arbitration/SortitionModule.sol validateStake signature does not include it. This will mislead integrators about the callable interface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/sortition-module.md` around lines 591 - 597, The
spec's validateStake signature is out of sync with the implementation: remove
the undocumented _eligibility parameter (or alternatively add it to the
implementation) so the specification matches the actual function signature in
SortitionModule.sol; specifically, update the documentation's validateStake
declaration to mirror the validateStake(...) definition in
contracts/src/arbitration/SortitionModule.sol (same parameters, return values
and modifiers such as onlyByCore) to avoid misleading integrators.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/specifications/arbitrable.md`:
- Around line 145-149: The example security checks use incorrect error
identifiers; update the revert calls so they match the contract's actual errors
by replacing OnlyArbitrator() with ArbitratorOnly() and InvalidRuling() with
RulingOutOfBounds() in the snippet that checks msg.sender, _ruling against
numberOfRulingOptions, and disputes[externalDisputeID].isRuled; keep the other
check (DisputeAlreadyRuled()) unchanged so the three-condition block uses
ArbitratorOnly(), RulingOutOfBounds(), and DisputeAlreadyRuled() respectively.
- Line 23: The table of contents entry "Direct URI References" points to a
non-existent fragment "#direct-uri-references"; locate the TOC entry in
contracts/specifications/arbitrable.md and either remove the "- [Direct URI
References](`#direct-uri-references`)" line or add a matching section header
(e.g., "## Direct URI References") with the referenced content so the link
resolves; ensure the header text exactly matches the fragment casing and
hyphenation to fix the broken link.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d7b7c11-331b-4a9e-89e3-eeeb46f8372d
📒 Files selected for processing (1)
contracts/specifications/arbitrable.md
| - [Ruling Flow](#ruling-flow) | ||
| 4. [🎯 Template System Integration](#-template-system-integration) | ||
| - [Template Registry](#template-registry) | ||
| - [Direct URI References](#direct-uri-references) |
Contributor
There was a problem hiding this comment.
Fix broken link fragment.
The link to #direct-uri-references does not point to any section in the document. Either remove this TOC entry or add the corresponding section.
🔗 Proposed fix to remove the broken link
4. [🎯 Template System Integration](`#-template-system-integration`)
- [Template Registry](`#template-registry`)
- - [Direct URI References](`#direct-uri-references`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - [Direct URI References](#direct-uri-references) | |
| 4. [🎯 Template System Integration](`#-template-system-integration`) | |
| - [Template Registry](`#template-registry`) |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 23-23: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/arbitrable.md` at line 23, The table of contents
entry "Direct URI References" points to a non-existent fragment
"#direct-uri-references"; locate the TOC entry in
contracts/specifications/arbitrable.md and either remove the "- [Direct URI
References](`#direct-uri-references`)" line or add a matching section header
(e.g., "## Direct URI References") with the referenced content so the link
resolves; ensure the header text exactly matches the fragment casing and
hyphenation to fix the broken link.
Comment on lines
+145
to
+149
| ```solidity | ||
| if (msg.sender != address(arbitrator)) revert OnlyArbitrator(); | ||
| if (_ruling > numberOfRulingOptions) revert InvalidRuling(); | ||
| if (disputes[externalDisputeID].isRuled) revert DisputeAlreadyRuled(); | ||
| ``` |
Contributor
There was a problem hiding this comment.
Update error names to match actual implementation.
The security checks example uses error names that don't match the actual implementation:
- Line 146: Should be
ArbitratorOnly()notOnlyArbitrator() - Line 147: Should be
RulingOutOfBounds()notInvalidRuling()
📝 Proposed fix to match actual error names
**Security Checks**:
```solidity
-if (msg.sender != address(arbitrator)) revert OnlyArbitrator();
-if (_ruling > numberOfRulingOptions) revert InvalidRuling();
+if (msg.sender != address(arbitrator)) revert ArbitratorOnly();
+if (_ruling > numberOfRulingOptions) revert RulingOutOfBounds();
if (disputes[externalDisputeID].isRuled) revert DisputeAlreadyRuled();
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/specifications/arbitrable.md` around lines 145 - 149, The example
security checks use incorrect error identifiers; update the revert calls so they
match the contract's actual errors by replacing OnlyArbitrator() with
ArbitratorOnly() and InvalidRuling() with RulingOutOfBounds() in the snippet
that checks msg.sender, _ruling against numberOfRulingOptions, and
disputes[externalDisputeID].isRuled; keep the other check
(DisputeAlreadyRuled()) unchanged so the three-condition block uses
ArbitratorOnly(), RulingOutOfBounds(), and DisputeAlreadyRuled() respectively.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Updates and expands the contract specifications in
contracts/specifications/to accurately reflect the current V2 implementation.Updated specifications (8 files)
_getCompatibleNextRoundSettingssignature, add error conditions table, separate public/internal interface sectionsgeneratingphase (was only documenting 2 of 3 phases), fix tree navigation formula accuracy, document all phase transition conditions, add error conditionsDEFAULT_Kconstant (K must be explicitly provided), document__gapstorage slots for upgradabilitysingleDrawPerJuror, add error conditions table, separate public/internal sectionsNew specifications (4 files)
All files
Last updated: 2026-03-14)Review process
PR-Codex overview
This PR updates the Kleros V2 specifications, enhancing the evidence submission and court policy formats, and introducing new features for dispute resolution mechanisms, including improved authentication and file management processes.
Detailed summary
EvidenceModulecontract reference in evidence format.Summary by CodeRabbit