Contracts: multi-dimensional coherence #2090
Conversation
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ 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. |
WalkthroughMigrates coherence calculations from ALPHA_DIVISOR to basis points via ONE_BASIS_POINT, splits single coherence into reward (PNK/fee) and penalty paths, updates core, university, and dispute kit logic and interfaces accordingly, removes legacy constants, and adjusts tests and minor naming. Public API changes occur in IDisputeKit and DisputeKitClassicBase. Changes
Sequence Diagram(s)sequenceDiagram
participant Core as KlerosCore/KlerosCoreUniversity
participant DK as DisputeKit (Classic)
participant Juror as Juror
Core->>DK: getDegreeOfCoherencePenalty(disputeID, roundID, voteID, feePerJuror, pnkAtStake)
DK-->>Core: pnkCoherence (0..10000)
Core->>Core: penalty = pnkAtStake * (10000 - coherence) / 10000
Core-->>Juror: Apply penalty
Core->>DK: getDegreeOfCoherenceReward(disputeID, roundID, voteID, feePerJuror, pnkAtStake)
DK-->>Core: (pnkCoherence, feeCoherence)
Core->>Core: pnkReward = ..., feeReward = ... using coherence/10000
Core-->>Juror: Distribute PNK and fees
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
795-801: Avoid.sendand unchecked return when transferring ETH; use.calland bubble errors.ETH sends to
governorignore failure. Prefer.call{value: ...}("")and handle the boolean result to avoid silent loss of funds when governor is a contract with a reverting fallback.- if (round.feeToken == NATIVE_CURRENCY) { - // The dispute fees were paid in ETH - payable(governor).send(round.totalFeesForJurors); - } else { + if (round.feeToken == NATIVE_CURRENCY) { + // The dispute fees were paid in ETH + (bool ok, ) = payable(governor).call{value: round.totalFeesForJurors}(""); + if (!ok) revert TransferFailed(); + } else { @@ - if (round.feeToken == NATIVE_CURRENCY) { - // The dispute fees were paid in ETH - payable(governor).send(leftoverFeeReward); - } else { + if (round.feeToken == NATIVE_CURRENCY) { + // The dispute fees were paid in ETH + (bool ok, ) = payable(governor).call{value: leftoverFeeReward}(""); + if (!ok) revert TransferFailed(); + } else {Also applies to: 875-881
🧹 Nitpick comments (9)
contracts/src/arbitration/interfaces/IDisputeKit.sol (2)
70-79: Doc clarity: explicitly call out BPS and units for both return values.Minor doc nit to make units explicit and self-contained for integrators.
Apply this diff to tighten the comments:
- /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. - /// @return feeCoherence The degree of coherence in basis points for the dispute fee reward. + /// @return pnkCoherence Degree of coherence in basis points (0..10_000) for the PNK reward path. + /// @return feeCoherence Degree of coherence in basis points (0..10_000) for the fee reward path.
80-94: Penalty doc return mixes “reward” wording; fix to avoid ambiguity.The penalty path return is described as “for the dispute PNK reward,” which is misleading here.
- /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. + /// @return pnkCoherence Degree of coherence in basis points (0..10_000) used by the PNK penalty path.contracts/src/libraries/Constants.sol (1)
23-25: BPS denominator looks good; add a clarifying inline comment.Tiny readability nit: call out that 10_000 equals 100% to avoid occasional confusion between “bps” vs “percent”.
-// Units -uint256 constant ONE_BASIS_POINT = 10000; +// Units +// Basis points denominator: 10_000 == 100% (1 bps == 0.01%) +uint256 constant ONE_BASIS_POINT = 10_000;contracts/test/foundry/KlerosCore.t.sol (2)
2315-2340: Tests exercise the new split-API correctly.Assertions for both PNK and fee coherences on reward path plus penalty path are consistent with Classic DK’s binary coherence. LGTM.
If you want extra coverage for arithmetic/rounding and clamping, consider adding a DK or a fixture that yields partial coherence (e.g., 3333 bps) and assert rounding behavior for:
- pnkLocked, pnkReward, feeReward
- leftover reward distribution.
2425-2443: LGTM on “no coherence” branch.Split-API assertions for the zero-coherence scenario are aligned with execution semantics and leftover distribution path.
Optionally, add a check that results never exceed ONE_BASIS_POINT to enforce clamping at the API boundary.
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
821-847: Optional: extract a small helper to DRY coherence application.You can mirror Core’s
_applyCoherence(amount, coherenceBps)to reduce repeated divisions and improve readability.Example:
function _applyCoherence(uint256 amount, uint256 coherenceBps) internal pure returns (uint256) { return (amount * coherenceBps) / ONE_BASIS_POINT; }Then:
- pnkLocked = _applyCoherence(round.pnkAtStakePerJuror, pnkCoherence)
- pnkReward = _applyCoherence(_params.pnkPenaltiesInRound / _params.coherentCount, pnkCoherence)
- feeReward = _applyCoherence(round.totalFeesForJurors / _params.coherentCount, feeCoherence)
contracts/src/arbitration/KlerosCoreBase.sol (2)
846-847: Rename local variable for clarity (unlock amount, not "locked")pnkLocked actually holds the amount to unlock (the coherent portion). Renaming improves readability and prevents confusion with “locked” semantics.
Apply this diff:
- uint256 pnkLocked = _applyCoherence(round.pnkAtStakePerJuror, pnkCoherence); + uint256 pnkToUnlock = _applyCoherence(round.pnkAtStakePerJuror, pnkCoherence); - // Release the rest of the PNKs of the juror for this round. - sortitionModule.unlockStake(account, pnkLocked); + // Release the coherent portion of the juror's locked PNK for this round. + sortitionModule.unlockStake(account, pnkToUnlock);
862-866: Event emits PNK coherence; fee coherence omission is acceptableGiven the existing TokenAndETHShift schema, emitting only pnkCoherence is consistent. If consumer tooling needs feeCoherence, consider a new event or indexed field in a follow-up.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
542-556: Docstring mismatch: says “reward” in a penalty getterThe return description mentions reward, but this is the penalty path. Update the comment to avoid confusion for integrators.
Apply this diff:
- /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. + /// @return pnkCoherence The degree of coherence in basis points for the PNK penalty path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
contracts/src/arbitration/KlerosCoreBase.sol(4 hunks)contracts/src/arbitration/SortitionModuleBase.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(2 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(7 hunks)contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol(0 hunks)contracts/src/libraries/Constants.sol(1 hunks)contracts/test/foundry/KlerosCore.t.sol(2 hunks)
💤 Files with no reviewable changes (1)
- contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`.
Applied to files:
contracts/src/arbitration/SortitionModuleBase.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
Applied to files:
contracts/src/arbitration/university/KlerosCoreUniversity.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). (14)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: contracts-testing
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Analyze (javascript)
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (15)
contracts/src/arbitration/SortitionModuleBase.sol (1)
347-355: Typo fix is correct; parent-walk logic remains intact.
currentCourtIDrename is consistent across the loop, the_setcall, and the parent lookup viacore.courts. Behavior unchanged. LGTM.contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
11-11: Importing ONE_BASIS_POINT centralizes units.Good move to rely on shared constants instead of per-file divisors.
821-835: Reward/penalty split and clamping are implemented correctly.
- Clamping to ONE_BASIS_POINT is defensive and prevents accidental overweight rewards.
- Separate coherence for PNK and fees is applied consistently to pnkLocked, pnkReward, and feeReward.
- Event payload now reflects PNK coherence, which matches the PNK path. LGTM.
Also applies to: 839-847, 860-864
contracts/src/arbitration/KlerosCoreBase.sol (9)
769-777: Good migration to penalty-specific coherence APISwitched to disputeKit.getDegreeOfCoherencePenalty with the extended signature. Passing both feePerJurorInRound and pnkAtStakePerJurorInRound aligns with the new interface and preserves extensibility across DKs.
778-781: Clamping coherence to ONE_BASIS_POINT is correctDefensive cap avoids accidental over-1 basis-point values leaking from DKs. This prevents negative penalties and over-rewards.
784-784: Penalty formula matches basis-point semanticsUsing (stake * (ONE_BASIS_POINT - coherence)) / ONE_BASIS_POINT keeps penalties proportional and consistent with reward-side coherence, with rounding handled symmetrically by the leftover path.
797-801: Event coherence value is consistent with the penalty pathEmitting the coherence (not the penalty) keeps the event payload semantically aligned with the reward-side event. This helps off-chain consumers interpret shifts uniformly.
829-836: Good split of reward coherences (PNK vs fees)Using disputeKit.getDegreeOfCoherenceReward to obtain separate pnkCoherence and feeCoherence values future-proofs the design for multi-dimensional DKs.
838-844: Clamping both PNK and fee coherences is appropriateThe explicit guard ensures downstream math stays within expected bounds even if a DK misreports.
852-855: Reward computation matches the two-pass model and leftover handling
- PNK reward is proportional to coherence over the per-capita penalty pool.
- Fee reward is proportional to coherence over the per-capita fee pool.
Leftovers are handled at the end of the second pass, avoiding rounding drift accumulation.
1065-1069: _applyCoherence migrated to basis points cleanlySimple and explicit basis-point application improves clarity vs ALPHA_DIVISOR and centralizes the scaling factor through ONE_BASIS_POINT.
1076-1077: _calculatePnkAtStake uses ONE_BASIS_POINT as intendedMatches the new constant and keeps math consistent across the codebase.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
9-9: Centralizing ONE_BASIS_POINT import is the right moveExplicit import from Constants.sol removes duplication and keeps the scale factor canonical.
531-541: Reward coherence getter implementation is sound
- Returns identical values for PNK and fee coherence, which is correct for Classic DK.
- Uses the shared _getDegreeOfCoherence helper to avoid duplication.
557-573: Coherence helper is correct and minimalBinary coherence (0 or ONE_BASIS_POINT) when voted and either tied or matched winning choice matches Classic semantics. Keeps non-voters at 0.
This PR also includes a few minor unrelated changes
ALPHA_DIVISORandONE_BASIS_POINTSconstantsPR-Codex overview
This PR focuses on improving the handling of coherence calculations in the Kleros arbitration system. It introduces a new constant for basis points and refactors functions to enhance clarity and maintainability.
Detailed summary
ONE_BASIS_POINTconstant inConstants.sol.getDegreeOfCoherencetogetDegreeOfCoherenceReward.getDegreeOfCoherencePenalty.ALPHA_DIVISORto useONE_BASIS_POINT.Summary by CodeRabbit
New Features
Refactor
Documentation
Tests