Contracts review feedback - round 1

[jaybuidl]

PR-Codex overview

This PR focuses on improving error handling and documentation clarity in the KlerosCore and related contracts, along with renaming errors for consistency. It also modifies some function parameters and improves comments for better understanding.

Detailed summary

• Renamed InvalidDisputKitParent to InvalidDisputeKitParent.
• Changed error handling for min stake checks.
• Updated documentation for functions regarding dispute kits and penalties.
• Added a new error MinStakeHigherThanChildCourt.
• Improved comments for clarity in various functions.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Summary by CodeRabbit

• Refactor
  • Standardized and renamed error messages for clearer appeal and court-validation reporting; one validation now includes the affected child court ID for clearer feedback.
• Tests
  • Updated tests to expect the renamed error signals.
• Documentation
  • Clarified ERC20 fee handling and stake-penalty wording; minor doc fixes.
• New Features
  • Governance functions now enforce owner-only access and accept a dispute-template registry at initialization.
• Chores
  • Minor staking/penalty calculation tidy; no user-facing behavior changes.

[coderabbitai]

Caution

Review failed

The pull request is closed.

Walkthrough

Renamed several custom error identifiers across arbitration contracts and tests; added MinStakeHigherThanChildCourt(uint256) error; tweaked SortitionModule.setStakePenalty to use the computed newCourtStake instead of re-reading stake; added owner-only modifier and template registry constructor parameter to DisputeResolver; minor documentation fixes.

Changes

Cohort / File(s)	Summary
Dispute kit error renames contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol	Renamed AppealPeriodIsOver() → NotAppealPeriod() and AppealPeriodIsOverForLoser() → NotAppealPeriodForLoser() and updated all revert sites.
Core error rename & new error contracts/src/arbitration/KlerosCore.sol	Renamed exported error InvalidDisputKitParent() → InvalidDisputeKitParent() and added error MinStakeHigherThanChildCourt(uint256 _childCourtID); updated usages and related messaging.
University mirror rename contracts/src/arbitration/university/KlerosCoreUniversity.sol	Renamed InvalidDisputKitParent → InvalidDisputeKitParent to match core rename.
Tests updated for error renames contracts/test/foundry/KlerosCore_Appeals.t.sol, contracts/test/foundry/KlerosCore_Governance.t.sol	Updated expected revert selectors to use NotAppealPeriod / NotAppealPeriodForLoser; governance test now expects MinStakeHigherThanChildCourt(childID) encoded revert when increasing parent min stake beyond a child.
Sortition stake logic tweak contracts/src/arbitration/SortitionModule.sol	In setStakePenalty, derive currentStake from newCourtStake (already computed) instead of calling _stakeOf(...), and set stake using newCourtStake. Updated comments clarifying cross-court staking limits.
DisputeResolver access control & ctor update contracts/src/arbitration/arbitrables/DisputeResolver.sol	Added onlyByOwner modifier (reverts OwnerOnly()), used it on governance functions, and added _templateRegistry constructor parameter assigned to templateRegistry.
Interface / docs fixes contracts/src/arbitration/interfaces/IArbitratorV2.sol, contracts/src/arbitration/interfaces/ISortitionModule.sol, contracts/src/arbitration/interfaces/IEvidence.sol	Doc/comment fixes: clarify ERC20 createDispute cost inputs, change setStakePenalty notice text to describe a PNK penalty, and fix a typo in IEvidence docs.

Sequence Diagram(s)

sequenceDiagram
  participant Caller as Caller
  participant SortMod as SortitionModule
  participant StakeStore as Stake storage

  rect #E6F8E6
  Note over Caller,SortMod: New flow (this PR)
  Caller->>SortMod: setStakePenalty(courtID, account, ...)
  SortMod->>SortMod: compute availablePenalty & newCourtStake
  SortMod->>StakeStore: _setStake(account, courtID, newCourtStake)
  SortMod-->>Caller: return (uses newCourtStake as current)
  end

  rect #FAEBE6
  Note over Caller,SortMod: Previous flow (before PR)
  Caller->>SortMod: setStakePenalty(courtID, account, ...)
  SortMod->>SortMod: compute availablePenalty & newCourtStake
  SortMod->>StakeStore: _setStake(account, courtID, newCourtStake)
  SortMod->>StakeStore: _stakeOf(account, courtID)  -- re-read stake
  SortMod-->>Caller: return (used re-read stake)
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• No more Neo contract distinction #2126 — Touches DisputeKit appeal-period identifiers and related tests; overlaps with the error-name and test updates in this PR.

Suggested labels

Compatibility: ABI change 🗯

Suggested reviewers

• unknownunknown1
• kemuru

Poem

Hop hop, I nudge the names anew,
Errors hopped to labels true.
Tests hopped through to catch the cue,
Stakes trimmed neat, constructor grew.
A rabbit cheers — the code looks new 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title Check	❓ Inconclusive	The title “Contracts review feedback - round 1” is too generic and does not highlight the central changes in this PR, such as renaming error identifiers, updating documentation, and adjusting appeal-period reverts; it reads more like an internal note than a summary of substantive modifications. As a result, reviewers scanning the history will not immediately grasp the primary scope of the changes. A concise, descriptive title would better convey the essence of the update.	Please revise the title to clearly reflect the main modifications, for example “Rename error identifiers and update appeal-period reverts” or “Refine contract error names and documentation in arbitration modules,” so that teammates can immediately understand the key changes.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45bcd0f and 06cccaa.

📒 Files selected for processing (1)

• contracts/src/arbitration/arbitrables/DisputeResolver.sol (2 hunks)

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[netlify]

❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →

Name	Link
🔨 Latest commit	06cccaa
🔍 Latest deploy log	https://app.netlify.com/projects/kleros-v2-testnet-devtools/deploys/68e3adc202cf0700089deb22

[netlify]

✅ Deploy Preview for kleros-v2-testnet ready!

Name	Link
🔨 Latest commit	06cccaa
🔍 Latest deploy log	https://app.netlify.com/projects/kleros-v2-testnet/deploys/68e3adc24303040008ae93ec
😎 Deploy Preview	https://deploy-preview-2154--kleros-v2-testnet.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[netlify]

✅ Deploy Preview for kleros-v2-neo ready!

Name	Link
🔨 Latest commit	06cccaa
🔍 Latest deploy log	https://app.netlify.com/projects/kleros-v2-neo/deploys/68e3adc2efb012000823812a
😎 Deploy Preview	https://deploy-preview-2154--kleros-v2-neo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud