No more Neo contract distinction #2126
Conversation
✅ 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-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
WalkthroughRemoved Neo-specific contracts/proxies and replaced legacy base implementations with new upgradeable v2 Changes
Sequence Diagram(s)sequenceDiagram autonumber participant A as Arbitrable participant C as KlerosCore participant DK as DisputeKit participant S as SortitionModule Note over C: optional arbitrableWhitelistEnabled & jurorNft gating A->>C: createDispute(#choices, extraData, [fee]) alt arbitrableWhitelistEnabled C->>C: require(arbitrableWhitelist[A]) end C->>DK: init dispute & first round loop draw jurors C->>S: draw(disputeID, nonce) S-->>C: (jurorAddr, fromSubcourtID) C->>S: lockStake(jurorAddr, amount) C->>DK: postDrawHook(...) end C-->>A: emits DisputeCreation / NewPeriod sequenceDiagram autonumber participant J as Juror participant NFT as IERC721 participant C as KlerosCore participant S as SortitionModule J->>C: setStake(courtID, newStake) opt jurorNft != zero C->>NFT: balanceOf(J) alt balance == 0 C-->>J: revert NotEligibleForStaking end end C->>S: validateStake(J, courtID, newStake, noDelay) S-->>C: (pnkDeposit, pnkWithdrawal, StakingResult) C->>S: setStake(J, courtID, pnkDeposit, pnkWithdrawal, newStake) S-->>J: emits StakeSet or StakeDelayed Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ 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-neo failed. Why did it fail? →
|
![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
🧹 Nitpick comments (5)
contracts/src/arbitration/KlerosCoreNeo.sol (1)
101-104: Emit an event when toggling the global whitelist (and make the setter idempotent).For on-chain observability and cleaner re-runs, emit an event and skip state writes when the value is unchanged.
- function changeArbitrableWhitelistEnabled(bool _enabled) external onlyByOwner { - arbitrableWhitelistEnabled = _enabled; - } + function changeArbitrableWhitelistEnabled(bool _enabled) external onlyByOwner { + if (arbitrableWhitelistEnabled == _enabled) return; + arbitrableWhitelistEnabled = _enabled; + emit ArbitrableWhitelistEnabledChanged(_enabled); + }Add this event (outside the selected range, e.g., in the Events section):
event ArbitrableWhitelistEnabledChanged(bool enabled);contracts/deploy/00-home-chain-arbitration-neo.ts (1)
122-123: Make the toggle idempotent to avoid noisy or failing re-runs.Read before write so the script can be safely re-executed on already-initialized environments.
- console.log(`core.changeArbitrableWhitelistEnabled(true)`); - await core.changeArbitrableWhitelistEnabled(true); + console.log(`core.changeArbitrableWhitelistEnabled(true)`); + if (!(await core.arbitrableWhitelistEnabled())) { + await core.changeArbitrableWhitelistEnabled(true); + }contracts/test/arbitration/staking-neo.ts (3)
131-142: Strengthen the “disabled” test: prove the flag bypasses the mapping.Currently the resolver is whitelisted by the deploy script, so this test would pass even if the flag were ignored. Explicitly unwhitelist and assert flag state.
describe("When arbitrable whitelist is disabled", () => { before("Setup", async () => { await deployUnhappy(); - await core.changeArbitrableWhitelistEnabled(false); + await core.changeArbitrableWhitelistEnabled(false); + await core.changeArbitrableWhitelist(resolver.target, false); + expect(await core.arbitrableWhitelistEnabled()).to.equal(false); });
145-150: Assert the flag after enabling to make intent explicit.describe("When arbitrable whitelist is enabled", () => { before("Setup", async () => { await deployUnhappy(); await core.changeArbitrableWhitelistEnabled(true); + expect(await core.arbitrableWhitelistEnabled()).to.equal(true); });
164-175: Happy-path coverage looks good (whitelisted arbitrable under enabled flag).Optional: to reduce brittleness, derive the expected dispute ID from
await core.disputesCounter()(or equivalent) instead of hardcoding0.
📜 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 (3)
contracts/deploy/00-home-chain-arbitration-neo.ts(1 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(3 hunks)contracts/test/arbitration/staking-neo.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/test/arbitration/staking-neo.ts (1)
contracts/deploy/utils/index.ts (1)
ETH(41-41)
⏰ 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-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (3)
contracts/src/arbitration/KlerosCoreNeo.sol (2)
19-21: Global whitelist flag added — confirm upgrade/migration plan.Adding a new storage slot is fine for UUPS, but existing proxies will default this to
falsepost-upgrade. Please confirm your upgrade runbook includes toggling the flag to the intended value on live deployments to avoid behavior drift.
130-131: LGTM: Conditional whitelist check is correct and short-circuits when disabled.contracts/test/arbitration/staking-neo.ts (1)
151-162: Good negative-path coverage for non-whitelisted arbitrables under enabled flag.
jaybuidl force-pushed the feat/migration-no-more-neo branch from 7bfd927 to 262f940 Compare There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/test/arbitration/staking-neo.ts (1)
2-12: Name collision: value-levelPNKhelper shadows the importedPNKtypeThis can cause TS “Cannot redeclare block-scoped variable ‘PNK’” and runtime import bloat. Import contract types as type-only to keep the local
PNK()helper.-import { - PNK, - RandomizerRNG, - RandomizerMock, - SortitionModuleNeo, - KlerosCoreNeo, - TestERC721, - DisputeResolver, - ChainlinkRNG, - ChainlinkVRFCoordinatorV2Mock, -} from "../../typechain-types"; +import type { + PNK, + RandomizerRNG, + RandomizerMock, + SortitionModuleNeo, + KlerosCoreNeo, + TestERC721, + DisputeResolver, + ChainlinkRNG, + ChainlinkVRFCoordinatorV2Mock, +} from "../../typechain-types";(Keeping lines 32-34 as-is is then safe.)
Also applies to: 31-34
🧹 Nitpick comments (3)
contracts/src/arbitration/KlerosCoreNeo.sol (1)
97-101: Emit an event and no-op on unchanged value when toggling the global whitelistAdds observability and avoids needless writes.
Apply this diff to the function:
- function changeArbitrableWhitelistEnabled(bool _enabled) external onlyByOwner { - arbitrableWhitelistEnabled = _enabled; - } + function changeArbitrableWhitelistEnabled(bool _enabled) external onlyByOwner { + if (arbitrableWhitelistEnabled == _enabled) return; + arbitrableWhitelistEnabled = _enabled; + emit ArbitrableWhitelistEnabledSet(_enabled); + }Also add this event (outside the selected range, near your events section):
event ArbitrableWhitelistEnabledSet(bool enabled);Optional: emit an event in
changeArbitrableWhitelistfor parity.contracts/deploy/00-home-chain-arbitration-neo.ts (1)
122-123: Make the toggle idempotent and wait for miningAvoid an unnecessary tx on reruns and ensure ordering on non-autominers.
- console.log(`core.changeArbitrableWhitelistEnabled(true)`); - await core.changeArbitrableWhitelistEnabled(true); + if (!(await core.arbitrableWhitelistEnabled())) { + console.log(`core.changeArbitrableWhitelistEnabled(true)`); + await (await core.changeArbitrableWhitelistEnabled(true)).wait(); + }Optional: also
wait()the subsequentchangeArbitrableWhitelistcall for consistency.contracts/test/arbitration/staking-neo.ts (1)
186-191: Avoid double-awaiting tx objects in event assertionsUse the standard pattern to prevent subtle assertion issues; apply broadly in this file.
- await expect(await core.connect(juror).setStake(1, PNK(1000))) + await expect(core.connect(juror).setStake(1, PNK(1000))) .to.emit(sortition, "StakeSet") .withArgs(juror.address, 1, PNK(1000), PNK(1000));
📜 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 (3)
contracts/deploy/00-home-chain-arbitration-neo.ts(1 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(4 hunks)contracts/test/arbitration/staking-neo.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/test/arbitration/staking-neo.ts (1)
contracts/deploy/utils/index.ts (2)
ETH(41-41)PNK(40-40)
🔇 Additional comments (7)
contracts/src/arbitration/KlerosCoreNeo.sol (2)
112-114: NFT gating for staking looks correctThe check is now conditional on
jurorNftbeing set; view-call viabalanceOfis safe understaticcall.
126-128: Whitelist gate in dispute creation is correctConditionally reverts only when the global flag is enabled and sender isn’t whitelisted.
contracts/test/arbitration/staking-neo.ts (5)
131-142: Whitelist disabled path test reads wellCorrectly asserts dispute creation succeeds when the global flag is off.
145-175: Whitelist enabled paths cover both non-whitelisted and whitelisted casesGood split verifying revert and success with expected args.
178-191: Staking allowed when juror NFT is unsetMatches contract change; assertions look correct.
195-208: Staking blocked without NFT when juror NFT is setAccurately checks for
NotEligibleForStaking.
210-223: Staking allowed with NFT when juror NFT is setHappy-path coverage is good.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
contracts/src/arbitration/KlerosCore.sol (2)
731-795: Consider nonReentrant on execute()Execute performs external calls (ERC20 transfers, SafeSend, SortitionModule). You mitigate reentrancy by updating
round.repartitionsbefore interactions, which is good; adding a reentrancy guard would further harden this hot path.If acceptable, import and apply a lightweight nonReentrant modifier (or custom mutex) to
execute()and the two internal helpers.
1288-1298: Typo in error name
InvalidDisputKitParentlikely meantInvalidDisputeKitParent. If keeping the old selector is important for upgrade safety, leave as-is; otherwise, rename for clarity.contracts/test/foundry/KlerosCore_Drawing.t.sol (1)
29-29: Optionally assert all three Draw events for stronger guarantees.
Currently only voteID=0 is asserted. Consider asserting 1 and 2 as well.vm.expectEmit(true, true, true, true); emit KlerosCore.Draw(staker1, disputeID, roundID, 0); // VoteID = 0 + vm.expectEmit(true, true, true, true); + emit KlerosCore.Draw(staker1, disputeID, roundID, 1); + vm.expectEmit(true, true, true, true); + emit KlerosCore.Draw(staker1, disputeID, roundID, 2);contracts/test/foundry/KlerosCore_Disputes.t.sol (1)
5-9: Unify IArbitrableV2 import sourcePrefer importing both IArbitratorV2 and IArbitrableV2 from KlerosCore.sol for consistency and to avoid type duplication across modules.
-import {IArbitrableV2} from "../../src/arbitration/arbitrables/ArbitrableExample.sol"; +import {IArbitratorV2, IArbitrableV2} from "../../src/arbitration/KlerosCore.sol";contracts/test/foundry/KlerosCore_Governance.t.sol (1)
5-7: Imports migrated to KlerosCore namespace correctlyAlso, please add coverage for the new arbitrable whitelist toggle (arbitrableWhitelistEnabled/changeArbitrableWhitelistEnabled): owner/guardian perms, event emission (if any), state effect on dispute creation.
I can draft a focused test that toggles the flag on/off and asserts createDispute gating across ETH and token paths.
📜 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 (14)
contracts/src/arbitration/KlerosCore.sol(4 hunks)contracts/src/arbitration/KlerosCoreBase.sol(0 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(0 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(4 hunks)contracts/src/proxy/KlerosProxies.sol(0 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(11 hunks)contracts/test/foundry/KlerosCore_Disputes.t.sol(5 hunks)contracts/test/foundry/KlerosCore_Drawing.t.sol(4 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(11 hunks)contracts/test/foundry/KlerosCore_Governance.t.sol(21 hunks)contracts/test/foundry/KlerosCore_Initialization.t.sol(4 hunks)contracts/test/foundry/KlerosCore_Staking.t.sol(6 hunks)contracts/test/foundry/KlerosCore_TestBase.sol(2 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(9 hunks)
💤 Files with no reviewable changes (3)
- contracts/src/proxy/KlerosProxies.sol
- contracts/src/arbitration/KlerosCoreBase.sol
- contracts/src/arbitration/KlerosCoreNeo.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/test/foundry/KlerosCore_Governance.t.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/test/foundry/KlerosCore_Governance.t.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 (74)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
275-277: Enum migration looks correctPeriod checks now reference KlerosCore.Period and align with Core changes.
Also applies to: 316-318, 521-521
contracts/test/foundry/KlerosCore_TestBase.sol (1)
154-156: Init signature update acknowledgedPassing
wNativeandIERC721(address(0))matches the new Core initializer.contracts/test/foundry/KlerosCore_Drawing.t.sol (4)
5-5: Namespace migration to KlerosCore looks correct.
64-64: Round type/namespace update is consistent.
109-109: Draw event assertions updated to KlerosCore namespace — good.Also applies to: 111-111, 113-113
118-118: Round typing migration here also looks good.contracts/test/foundry/KlerosCore_Initialization.t.sol (4)
5-5: Importing KlerosCore and IERC721 from the new surface is correct.
141-141: Event DisputeKitCreated assertion updated to KlerosCore — OK.
146-156: CourtCreated event assertion migration — OK.
158-158: DisputeKitEnabled event assertion migration — OK.contracts/test/foundry/KlerosCore_Staking.t.sol (2)
5-5: Import switch to KlerosCore is correct.
17-17: All referenced custom errors are defined in KlerosCore.sol and match the selectors in tests.contracts/test/foundry/KlerosCore_Disputes.t.sol (5)
41-47: Selector migration to KlerosCore. looks correct*
65-76: Updated disputes tuple typing with KlerosCore.Period is correct
78-89: getRoundInfo return binding to KlerosCore.Round is correct
119-135: Token-path revert expectations align with new errors
140-144: Round assertions updated for token-fee flow look goodcontracts/test/foundry/KlerosCore_Execution.t.sol (11)
5-9: Imports aligned with KlerosCore public surface
56-66: Pause/period guards use new selectors correctly
119-122: Round.repartitions/pnkPenalties checks after partial execute are sound
126-136: Reward-iteration expectations and emits look consistent
203-211: LeftoverRewardSent emission target and values look correct
474-478: Round aggregates after withdrawLeftoverPNK: OK
483-486: SortitionModuleOnly guard covered appropriately
540-543: Fee-token shift events cover penalty and reward once each
545-547: sumFeeRewardPaid assertion for fee-token path is correct
586-600: No-coherence fee-token leftovers path validated
101-111: No action required: TokenAndETHShift parameters in tests match event signature
The event in KlerosCore.sol is declared as
(address indexed _account, uint256 indexed _disputeID, uint256 indexed _roundID, uint256 _degreeOfCoherency, int256 _pnkAmount, int256 _feeAmount, IERC20 _feeToken), which aligns exactly with the test’s emits.contracts/test/foundry/KlerosCore_Governance.t.sol (20)
16-29: Guardian/Owner pause guard behavior: OK
31-47: Unpause requires paused; event emission verified
49-64: executeOwnerProposal negative/positive paths covered well
66-73: OwnerOnly guard for changeOwner enforced
75-82: OwnerOnly guard for changeGuardian enforced
84-92: OwnerOnly guard for changePinakion enforced
95-101: OwnerOnly guard for changeJurorProsecutionModule enforced
104-111: OwnerOnly guard for changeSortitionModule enforced
113-124: DisputeKitCreated event and indexing validated
127-172: Court creation: ownership + minStake/DK validations correct
173-219: Parent/NULL/out-of-bounds DK and forking parent checks are solid
226-239: MustSupportDisputeKitClassic requirement enforced
240-257: CourtCreated/DisputeKitEnabled events asserted with full payloads
269-281: Post-create assertions (children, sortition properties) look good
302-357: changeCourtParameters: OwnerOnly + inter-court minStake invariant validatedMatches prior learning: child minStake >= parent minStake.
369-401: enableDisputeKits: guards and CannotDisableClassicDK covered
405-418: Accepted fee token toggling wired to arbitrator events
421-437: Currency rate update event and storage verified
440-471: extraData decoding fallback and bounds behavior validated
5-7: Repo-wide migration completeness checks
- Ensure no direct
import {KlerosCoreBase}or references toKlerosCoreBaseremain in any.solfiles (matches in JSON, docs or generated artifacts can be ignored).- Fix the import of
IArbitratorV2/IArbitrableV2incontracts/test/foundry/KlerosCore_Governance.t.solto pull fromsrc/arbitration/interfaces/IArbitratorV2.solinstead ofKlerosCore.sol.- Add tests covering the whitelist toggles (
arbitrableWhitelistEnabledandchangeArbitrableWhitelistEnabled) inKlerosCore_Governance.t.sol.contracts/test/foundry/KlerosCore_Appeals.t.sol (13)
5-5: KlerosCore import applied; namespace migration consistent
45-47: DisputeNotAppealable selector path updated correctly
49-53: AppealPossible/NewPeriod events under KlerosCore are correct
54-61: disputes tuple now uses KlerosCore.Period; assertions OK
62-69: AppealPeriodNotPassed guard maintained
178-184: AppealDecision and evidence-period reset after full funding: OK
197-200: Post-appeal period verification aligns with new API
201-208: Round parameters in appealed round validated
266-269: Round DK ID check uses KlerosCore.Round; good
289-298: CourtJump/DisputeKitJump/AppealDecision/NewPeriod events wired correctly
325-326: Draw event namespace migration validated
427-436: Alternate DK jump path to non-classic validated with events
463-464: Draw in jumped-to DK asserted correctlycontracts/test/foundry/KlerosCore_Voting.t.sol (13)
5-5: KlerosCore import migration in voting tests looks good
47-49: EvidenceNotPassedAndNotAppeal selector path updated
51-54: NewPeriod(commit) emission under KlerosCore verified
55-59: disputes tuple now uses KlerosCore.Period; commit period asserted
152-154: CommitPeriodNotPassed guard updated
156-159: Transition to vote period uses new event
181-183: DisputeStillDrawing guard migrated
186-194: Vote period event and disputes tuple assertions updated
253-255: VotePeriodNotPassed guard migrated
289-296: AppealPossible + NewPeriod(appeal) emissions asserted
383-385: Quick pass to appeal period relies on committed votes; event OK
410-413: DisputeKitEnabled event asserted for new DK
437-438: Round.disputeKitID check for new DK is correct
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
contracts/test/foundry/KlerosCore_TestBase.sol (1)
6-7: Use canonical interface imports; avoid relying on re-exports.Import IERC721 and IArbitratorV2 directly from their defining interfaces; import SortitionModule from the production contract, not the mock file.
-import {KlerosCoreMock, KlerosCore, IERC721} from "../../src/test/KlerosCoreMock.sol"; -import {IArbitratorV2} from "../../src/arbitration/KlerosCore.sol"; +import {KlerosCoreMock, KlerosCore} from "../../src/test/KlerosCoreMock.sol"; +import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; +import {IArbitratorV2} from "../../src/arbitration/interfaces/IArbitratorV2.sol"; @@ -import {SortitionModuleMock, SortitionModule} from "../../src/test/SortitionModuleMock.sol"; +import {SortitionModuleMock} from "../../src/test/SortitionModuleMock.sol"; +import {SortitionModule} from "../../src/arbitration/SortitionModule.sol";Also applies to: 12-12
contracts/test/foundry/KlerosCore_Execution.t.sol (1)
8-8: Import interfaces from their canonical locations.Import IArbitratorV2/IArbitrableV2 from interfaces (or ArbitrableExample for IArbitrableV2), not via KlerosCore.
-import {IArbitratorV2, IArbitrableV2} from "../../src/arbitration/KlerosCore.sol"; +import {IArbitratorV2} from "../../src/arbitration/interfaces/IArbitratorV2.sol"; +import {IArbitrableV2} from "../../src/arbitration/interfaces/IArbitrableV2.sol";
🧹 Nitpick comments (6)
contracts/test/foundry/KlerosCore_TestBase.sol (1)
135-137: Prefer uint256 constants for uint256 params.Avoid signed→unsigned implicit conversions in initialize args.
- rng, - type(int256).max, - type(int256).max + rng, + type(uint256).max, + type(uint256).maxcontracts/src/arbitration/SortitionModule.sol (5)
64-96: Emit governance change events.On-chain config changes need auditability.
Add events:
event StakeSet(address indexed _address, uint256 _courtID, uint256 _amount, uint256 _amountAllCourts); ... event LeftoverPNKWithdrawn(address indexed _account, uint256 _amount); + +// Governance/config events +event OwnerChanged(address indexed previousOwner, address indexed newOwner); +event MinStakingTimeChanged(uint256 previous, uint256 current); +event MaxDrawingTimeChanged(uint256 previous, uint256 current); +event RNGChanged(address indexed previous, address indexed current); +event MaxStakePerJurorChanged(uint256 previous, uint256 current); +event MaxTotalStakedChanged(uint256 previous, uint256 current);Then emit in the setters (see next comment).
158-191: Add emits and validate inputs in governance setters.Track changes and reject zero RNG/owner.
function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + require(_owner != address(0), "owner=0"); + emit OwnerChanged(owner, _owner); + owner = _owner; } ... function changeMinStakingTime(uint256 _minStakingTime) external onlyByOwner { - minStakingTime = _minStakingTime; + emit MinStakingTimeChanged(minStakingTime, _minStakingTime); + minStakingTime = _minStakingTime; } ... function changeMaxDrawingTime(uint256 _maxDrawingTime) external onlyByOwner { - maxDrawingTime = _maxDrawingTime; + emit MaxDrawingTimeChanged(maxDrawingTime, _maxDrawingTime); + maxDrawingTime = _maxDrawingTime; } ... function changeRandomNumberGenerator(IRNG _rng) external onlyByOwner { - rng = _rng; + require(address(_rng) != address(0), "rng=0"); + emit RNGChanged(address(rng), address(_rng)); + rng = _rng; if (phase == Phase.generating) { rng.requestRandomness(); } } ... function changeMaxStakePerJuror(uint256 _maxStakePerJuror) external onlyByOwner { - maxStakePerJuror = _maxStakePerJuror; + emit MaxStakePerJurorChanged(maxStakePerJuror, _maxStakePerJuror); + maxStakePerJuror = _maxStakePerJuror; } ... function changeMaxTotalStaked(uint256 _maxTotalStaked) external onlyByOwner { - maxTotalStaked = _maxTotalStaked; + emit MaxTotalStakedChanged(maxTotalStaked, _maxTotalStaked); + maxTotalStaked = _maxTotalStaked; }
227-245: Consider emitting progress for delayed stake execution.Operational visibility for keepers.
function executeDelayedStakes(uint256 _iterations) external { if (phase != Phase.staking) revert NotStakingPhase(); if (delayedStakeWriteIndex < delayedStakeReadIndex) revert NoDelayedStakeToExecute(); @@ for (uint256 i = delayedStakeReadIndex; i < newDelayedStakeReadIndex; i++) { DelayedStake storage delayedStake = delayedStakes[i]; core.setStakeBySortitionModule(delayedStake.account, delayedStake.courtID, delayedStake.stake); delete delayedStakes[i]; } delayedStakeReadIndex = newDelayedStakeReadIndex; + // Optional: emit processed count for observability. + // emit DelayedStakesExecuted(msg.sender, actualIterations); }(Add event if you adopt this.)
73-80: Unify event types: use uint96 for _courtID and index it.Aligns event ABI with storage/use and improves filtering.
-event StakeSet(address indexed _address, uint256 _courtID, uint256 _amount, uint256 _amountAllCourts); +event StakeSet(address indexed _address, uint96 indexed _courtID, uint256 _amount, uint256 _amountAllCourts);(If external consumers depend on uint256, skip this.)
598-607: Validate K from extraData.Protect against invalid K (e.g., 0 or 1) producing degenerate trees.
function _extraDataToTreeK(bytes memory _extraData) internal pure returns (uint256 K) { if (_extraData.length >= 32) { assembly { K := mload(add(_extraData, 0x20)) } } else { K = DEFAULT_K; } + if (K < 2) { + K = DEFAULT_K; + } }
📜 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 (9)
contracts/src/arbitration/SortitionModule.sol(3 hunks)contracts/src/arbitration/SortitionModuleBase.sol(0 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(0 hunks)contracts/test/foundry/KlerosCore_Drawing.t.sol(4 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(14 hunks)contracts/test/foundry/KlerosCore_Initialization.t.sol(5 hunks)contracts/test/foundry/KlerosCore_RNG.t.sol(2 hunks)contracts/test/foundry/KlerosCore_Staking.t.sol(12 hunks)contracts/test/foundry/KlerosCore_TestBase.sol(3 hunks)
💤 Files with no reviewable changes (2)
- contracts/src/arbitration/SortitionModuleBase.sol
- contracts/src/arbitration/SortitionModuleNeo.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/test/foundry/KlerosCore_Initialization.t.sol
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-07T11:39:10.927Z
Learnt from: jaybuidl PR: kleros/kleros-v2#1778 File: contracts/src/rng/ChainlinkRNG.sol:135-153 Timestamp: 2024-12-07T11:39:10.927Z Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context. Applied to files:
contracts/test/foundry/KlerosCore_RNG.t.sol
📚 Learning: 2024-12-07T11:41:15.835Z
Learnt from: jaybuidl PR: kleros/kleros-v2#1778 File: contracts/src/rng/RandomizerRNG.sol:21-21 Timestamp: 2024-12-07T11:41:15.835Z Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context. Applied to files:
contracts/test/foundry/KlerosCore_RNG.t.sol
📚 Learning: 2025-09-03T19:34:58.018Z
Learnt from: jaybuidl PR: kleros/kleros-v2#2107 File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092 Timestamp: 2025-09-03T19:34:58.018Z Learning: KlerosCoreUniversity and SortitionModuleUniversity do not have phases, unlike KlerosCoreBase and SortitionModuleBase. Therefore, validateStake in the University contracts will never return StakingResult.Delayed, only Successful or other failure states. Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.solcontracts/test/foundry/KlerosCore_Drawing.t.solcontracts/test/foundry/KlerosCore_Staking.t.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: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (21)
contracts/test/foundry/KlerosCore_RNG.t.sol (2)
5-5: LGTM: import migrated to consolidated SortitionModule.
37-37: LGTM: selector updated to SortitionModule.RandomNumberNotReady.contracts/test/foundry/KlerosCore_TestBase.sol (1)
156-158: LGTM: initialize signature extended with wNative and juror NFT (IERC721).contracts/test/foundry/KlerosCore_Drawing.t.sol (3)
5-6: LGTM: imports updated to KlerosCore and SortitionModule.
27-30: LGTM: event sources migrated (StakeLocked, Draw) to new modules.
64-64: LGTM: Round struct path updated to KlerosCore.Round.Also applies to: 118-118
contracts/test/foundry/KlerosCore_Execution.t.sol (2)
5-6: LGTM: imports realigned to consolidated KlerosCore/SortitionModule.
56-56: LGTM: selector and event migrations to KlerosCore/SortitionModule.Revert selectors (e.g., NotExecutionPeriod, WhenNotPausedOnly, SortitionModuleOnly) and events (TokenAndETHShift, LeftoverRewardSent, NewPeriod, StakeLocked/Set) are correctly pointed to the new modules/types.
Also applies to: 64-64, 98-111, 119-136, 204-205, 463-469, 483-489, 632-637, 648-649
contracts/test/foundry/KlerosCore_Staking.t.sol (9)
5-6: LGTM: imports updated to KlerosCore and SortitionModule.
17-39: LGTM: staking-related KlerosCore error selectors updated.
42-44: LGTM: SortitionModule event names (StakeSet/StakeDelayed) updated in expectations.Also applies to: 68-70, 191-193, 237-239, 323-325, 332-334, 339-341
61-64: LGTM: StakingTransferFailed path validated.
114-117: LGTM: UnstakingTransferFailed path validated.
168-171: LGTM: StakingInTooManyCourts updated to KlerosCore selector.
304-306: LGTM: NoDelayedStakeToExecute / NotStakingPhase selectors migrated.Also applies to: 317-319
387-390: LGTM: executeDelayedStakes emits updated StakeSet events as expected.
419-422: LGTM: access control revert uses KlerosCore.SortitionModuleOnly.contracts/src/arbitration/SortitionModule.sol (4)
16-19: Upgradeable wiring and modularization look solid.Initializable + UUPSProxiable composition and interface conformance read well.
585-592: Semantics check: isJurorStaked vs leftover PNK.isJurorStaked returns true when juror has leftover PNK (courtIDs.length==0, locked==0) because stakedPnk > 0. Confirm this is intended; otherwise consider checking stakeOf in General court or courtIDs.length > 0.
246-253: Double-check hooks balance to avoid underflow.postDrawHook unconditionally decrements disputesWithoutJurors. Ensure KC never calls postDrawHook more times than createDisputeHook per round/dispute.
Would you like a script to scan KC for the hook callsites and validate counts?
508-516: Nice: leftover PNK withdrawal is pull-based and reentrancy-safe.Effects-before-interactions and transfer via Core look correct.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
contracts/test/foundry/KlerosCore_Initialization.t.sol (1)
149-167: Event expectations look correct; add a small toggle test for whitelist.Nice coverage for DK/court-related events. Consider also asserting the whitelist toggle behavior to lock in the new owner API.
Add after initialization (or as a separate test):
newCore.initialize( /* ... */ IERC721(address(0)) ); + assertFalse(newCore.arbitrableWhitelistEnabled()); + vm.prank(newOwner); + newCore.changeArbitrableWhitelistEnabled(true); + assertTrue(newCore.arbitrableWhitelistEnabled());
🧹 Nitpick comments (1)
contracts/test/foundry/KlerosCore_TestBase.sol (1)
129-137: New SM initializer args look right; prefer named locals over magic max values.The signature and argument order align with the extended initializer. Consider naming the two
type(uint256).maxargs for readability (e.g.,maxStakePerJuror/maxTotalStaked).Example:
+ uint256 maxStakePerJuror = type(uint256).max; + uint256 maxTotalStaked = type(uint256).max; bytes memory initDataSm = abi.encodeWithSignature( "initialize(address,address,uint256,uint256,address,uint256,uint256)", owner, address(proxyCore), minStakingTime, maxDrawingTime, - rng, - type(uint256).max, - type(uint256).max + rng, + maxStakePerJuror, + maxTotalStaked );
📜 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 (2)
contracts/test/foundry/KlerosCore_Initialization.t.sol(7 hunks)contracts/test/foundry/KlerosCore_TestBase.sol(3 hunks)
⏰ 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 (4)
contracts/test/foundry/KlerosCore_TestBase.sol (1)
156-158: LGTM: core initializer extended params wired correctly.
wNativeandjurorNft(set to zero) are passed in the correct positions.contracts/test/foundry/KlerosCore_Initialization.t.sol (3)
48-50: Good: asserting new core fields.Asserts for
wNative,jurorNft, andarbitrableWhitelistEnabledverify the new initializer plumbing.
77-80: Good: asserting new SM caps and total staked.Confirms the two new initializer params and starting state.
132-140: LGTM: updated SM initializer usage.Signature and arg order match the contract change.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
contracts/src/arbitration/KlerosCore.sol (3)
549-561: Guard ERC20 path against unset rates before calling arbitrationCostCalling arbitrationCost(_extraData, _feeToken) can divide by zero if rateInEth is 0. Add an explicit check to fail fast with a clear error.
function createDispute( uint256 _numberOfChoices, bytes calldata _extraData, IERC20 _feeToken, uint256 _feeAmount ) external override returns (uint256 disputeID) { if (!currencyRates[_feeToken].feePaymentAccepted) revert TokenNotAccepted(); + if (currencyRates[_feeToken].rateInEth == 0) revert TokenRateNotSet(); if (_feeAmount < arbitrationCost(_extraData, _feeToken)) revert ArbitrationFeesNotEnough(); if (!_feeToken.safeTransferFrom(msg.sender, address(this), _feeAmount)) revert TransferFailed(); return _createDispute(_numberOfChoices, _extraData, _feeToken, _feeAmount); }And declare:
error StakingZeroWhenNoStake(); +error TokenRateNotSet();
1109-1111: convertEthToTokenAmount: handle ETH sentinel and division-by-zeroPassing the ETH sentinel or a token with unset rate will revert due to division-by-zero. Handle the sentinel and add an explicit guard.
-function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { - return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / currencyRates[_toToken].rateInEth; -} +function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { + if (_toToken == NATIVE_CURRENCY) return _amountInEth; + CurrencyRate memory rate = currencyRates[_toToken]; + if (rate.rateInEth == 0) revert TokenRateNotSet(); + return (_amountInEth * 10 ** rate.rateDecimals) / rate.rateInEth; +}
496-507: Whitelist updates: add input validation and emit eventsZero-address arbitrable should be rejected and whitelist state changes should emit events for auditability.
- function changeArbitrableWhitelist(address _arbitrable, bool _allowed) external onlyByOwner { - arbitrableWhitelist[_arbitrable] = _allowed; - } + function changeArbitrableWhitelist(address _arbitrable, bool _allowed) external onlyByOwner { + if (_arbitrable == address(0)) revert InvalidArbitrable(); + arbitrableWhitelist[_arbitrable] = _allowed; + emit ArbitrableWhitelistChanged(_arbitrable, _allowed); + } /// @dev Enables or disables the arbitrable whitelist. function changeArbitrableWhitelistEnabled(bool _enabled) external onlyByOwner { - arbitrableWhitelistEnabled = _enabled; + arbitrableWhitelistEnabled = _enabled; + emit ArbitrableWhitelistEnabledChanged(_enabled); }Also add to Events/Errors:
event Unpaused(); +event ArbitrableWhitelistChanged(address indexed _arbitrable, bool _allowed); +event ArbitrableWhitelistEnabledChanged(bool _enabled); +error InvalidArbitrable();
🧹 Nitpick comments (11)
contracts/src/arbitration/KlerosCore.sol (4)
472-489: Enforce invariants between acceptance and ratesAccepting a fee token with an unset rate (or zero decimals) enables silent misconfiguration. Require a non-zero rate when enabling, and optionally clear acceptance on rate set to zero.
function changeAcceptedFeeTokens(IERC20 _feeToken, bool _accepted) external onlyByOwner { + if (_accepted && currencyRates[_feeToken].rateInEth == 0) revert TokenRateNotSet(); currencyRates[_feeToken].feePaymentAccepted = _accepted; emit AcceptedFeeToken(_feeToken, _accepted); } function changeCurrencyRates(IERC20 _feeToken, uint64 _rateInEth, uint8 _rateDecimals) external onlyByOwner { currencyRates[_feeToken].rateInEth = _rateInEth; currencyRates[_feeToken].rateDecimals = _rateDecimals; emit NewCurrencyRate(_feeToken, _rateInEth, _rateDecimals); + if (_rateInEth == 0) { + currencyRates[_feeToken].feePaymentAccepted = false; + emit AcceptedFeeToken(_feeToken, false); + } }
314-343: Emit governance change events (owner/guardian/pnk/prosecution/sortition/jurorNft)These state changes currently have no events, making off-chain indexing harder. Emitting events is low-cost and improves observability.
Example:
+event OwnerChanged(address indexed previousOwner, address indexed newOwner); +event GuardianChanged(address indexed previousGuardian, address indexed newGuardian); +event PinakionChanged(IERC20 indexed previous, IERC20 indexed current); +event JurorProsecutionModuleChanged(address indexed previous, address indexed current); +event SortitionModuleChanged(address indexed previous, address indexed current); +event JurorNftChanged(IERC721 indexed previous, IERC721 indexed current); function changeOwner(address payable _owner) external onlyByOwner { - owner = _owner; + address prev = owner; + owner = _owner; + emit OwnerChanged(prev, _owner); } @@ function changeJurorNft(IERC721 _jurorNft) external onlyByOwner { - jurorNft = _jurorNft; + IERC721 prev = jurorNft; + jurorNft = _jurorNft; + emit JurorNftChanged(prev, _jurorNft); }Also applies to: 490-494
1264-1288: Avoid out-of-bounds mload for disputeKitIDThe assembly branch triggers when _extraData.length >= 64 but then reads at +0x60 (96). Tighten the length check to 96.
- if (_extraData.length >= 64) { + if (_extraData.length >= 96) { assembly { courtID := mload(add(_extraData, 0x20)) minJurors := mload(add(_extraData, 0x40)) disputeKitID := mload(add(_extraData, 0x60)) }Optionally keep the >=64 branch to read just courtID/minJurors, and default disputeKitID.
1294-1333: Typo in error name: InvalidDisputKitParentMinor: rename to InvalidDisputeKitParent for consistency, or remove if unused.
-error InvalidDisputKitParent(); +error InvalidDisputeKitParent();Note: rename changes the ABI; do it before external integrations rely on it.
contracts/test/arbitration/staking-neo.ts (2)
131-175: Whitelist tests: add positive/negative ERC20-fee casesConsider adding cases for ERC20 fee payments:
- When token is accepted but rate is unset → expect revert TokenRateNotSet.
- When token is accepted and rate set → dispute created and event emitted.
I can draft the additional tests targeting createDispute(token) once TokenRateNotSet is implemented.
440-519: Delayed stakes coverage is solid; consider covering reentrancy-sensitive execute pathGiven execute distributes rewards and performs external transfers, add a regression test using a mock ERC20 with a reentrant transfer hook to ensure nonReentrant (once added) blocks reentry.
Happy to supply a minimal malicious ERC20 mock and test.
contracts/deploy/00-home-chain-arbitration.ts (3)
44-49: Precomputing KlerosCore address via nonce is brittleAny extra tx before deployment breaks the prediction and the SortitionModule’s initializer arg. Prefer deploying KlerosCore first (or reading the actual proxy address) and then initializing SortitionModule with the concrete value.
If you prefer to keep prediction, add an assertion after deploy to verify sortitionModule.core() matches the actual KlerosCore proxy address.
56-64: Max stake limits: use explicit large values over MaxUint256MaxUint256 may cause overflows in downstream arithmetic or UI issues. Consider a realistic cap (e.g., type(uint128).max) or a business-defined upper bound.
106-114: Set coherent currency rates for all accepted tokens before enabling acceptance in prodThe helper calls set both acceptance and rates—ensure this pattern mirrors mainnet scripts. Also add a negative test path where rate=0 keeps acceptance=false after your core-side invariant change.
contracts/deploy/00-home-chain-arbitration-mainnet.ts (2)
38-43: Brittle address prediction for KlerosCoreSame nonce-based prediction risk as the dev script. Recommend deploying KlerosCore first or validating the predicted address post-deploy.
120-124: Enable whitelist: emit events once addedOnce ArbitrableWhitelistEnabledChanged and ArbitrableWhitelistChanged are added, this script will produce helpful logs and on-chain traces.
📜 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 (5)
contracts/deploy/00-home-chain-arbitration-mainnet.ts(7 hunks)contracts/deploy/00-home-chain-arbitration.ts(2 hunks)contracts/src/arbitration/KlerosCore.sol(4 hunks)contracts/src/arbitration/SortitionModule.sol(3 hunks)contracts/test/arbitration/staking-neo.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/arbitration/SortitionModule.sol
🧰 Additional context used
🧬 Code graph analysis (3)
contracts/deploy/00-home-chain-arbitration.ts (2)
contracts/deployments/devnet.viem.ts (1)
klerosCoreAddress(12320-12322)contracts/deployments/testnet.viem.ts (1)
klerosCoreAddress(10190-10192)
contracts/deploy/00-home-chain-arbitration-mainnet.ts (3)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deployments/utils.ts (1)
deployments(3-16)contracts/deploy/utils/index.ts (2)
isDevnet(30-30)PNK(40-40)
contracts/test/arbitration/staking-neo.ts (2)
contracts/deployments/utils.ts (1)
deployments(3-16)contracts/deploy/utils/index.ts (2)
PNK(40-40)ETH(41-41)
⏰ 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). (13)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: hardhat-tests
🔇 Additional comments (3)
contracts/test/arbitration/staking-neo.ts (2)
55-65: Fixture tag alignmentUsing deployments.fixture(["ArbitrationMainnet"]) assumes the tag matches the mainnet deployment script. Confirm tags across all deploy scripts remain consistent after the rename.
I can add a quick repo check script if helpful.
178-224: NFT gating paths look goodThe scenarios cover both unset-NFT and set-NFT with/without holder. Assertions on StakeSet and totalStaked match expectations.
contracts/deploy/00-home-chain-arbitration-mainnet.ts (1)
68-86: Initializer args ordering changes look correctJuror NFT appended after weth and consistent with core initializer. Good.
jaybuidl force-pushed the feat/migration-no-more-neo branch from e367565 to 923bd7b Compare There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/deploy/change-sortition-module-rng.ts (1)
18-22: Await transaction receipts to avoid early exit/racesBoth writes fire without confirmations. Await receipts so the task doesn’t terminate before state is finalized.
- console.log(`chainlinkRng.changeConsumer(${sortitionModule.target})`); - await chainlinkRng.changeConsumer(sortitionModule.target); + console.log(`chainlinkRng.changeConsumer(${sortitionModule.target})`); + const tx1 = await chainlinkRng.changeConsumer(sortitionModule.target); + await tx1.wait(); - console.log(`sortitionModule.changeRandomNumberGenerator(${chainlinkRng.target})`); - await sortitionModule.changeRandomNumberGenerator(chainlinkRng.target); + console.log(`sortitionModule.changeRandomNumberGenerator(${chainlinkRng.target})`); + const tx2 = await sortitionModule.changeRandomNumberGenerator(chainlinkRng.target); + await tx2.wait();contracts/deploy/utils/klerosCoreHelper.ts (1)
17-21: BigInt comparison bug causes unnecessary writespnkRate.rateDecimals is a bigint; comparing to a non-coerced BigNumberish makes the condition always true when a number/string is passed.
- if (pnkRate.rateInEth !== toBigInt(rateInEth) || pnkRate.rateDecimals !== rateDecimals) { - console.log(`core.changeCurrencyRates(${erc20}, ${rateInEth}, ${rateDecimals})`); - const tx = await core.changeCurrencyRates(erc20, rateInEth, rateDecimals); + const rateDecimalsBI = toBigInt(rateDecimals); + if (pnkRate.rateInEth !== toBigInt(rateInEth) || pnkRate.rateDecimals !== rateDecimalsBI) { + console.log(`core.changeCurrencyRates(${erc20}, ${rateInEth}, ${rateDecimalsBI})`); + const tx = await core.changeCurrencyRates(erc20, rateInEth, rateDecimalsBI); await tx.wait(); }
🧹 Nitpick comments (7)
contracts/deploy/change-sortition-module-rng.ts (1)
15-17: Make the task idempotent (read-before-write guards)Skip writes if already configured (e.g., compare current RNG on SortitionModule and current consumer on ChainlinkRNG before sending txs). This cuts gas/noise on reruns.
Also applies to: 18-22
contracts/deploy/upgrade-all.ts (2)
54-59: Add confirmations to stabilize post-upgrade verificationWaiting a few blocks reduces Etherscan race conditions and flaky CI.
- await deployUpgradable(deployments, contractName, { + await deployUpgradable(deployments, contractName, { newImplementation: contractName, initializer, from: deployer, args, // Warning: do not reinitialize existing state variables, only the new ones + waitConfirmations: 3, });
70-79: Reinitializer reruns can revertCalling reinitialize on every run can fail once the version has been consumed. Consider guarding (version check/no-op) or making initializer optional per contract when rerunning.
contracts/scripts/storage-layout.ts (1)
7-9: Handle missing build info defensivelyHardhat usually compiles first, but an explicit guard prevents cryptic undefined errors.
- const buildInfo = await hre.artifacts.getBuildInfo(`src/arbitration/KlerosCore.sol:KlerosCore`); - console.log(buildInfo.output.contracts["src/arbitration/KlerosCore.sol"]["KlerosCore"].storageLayout); + const buildInfo = await hre.artifacts.getBuildInfo(`src/arbitration/KlerosCore.sol:KlerosCore`); + if (!buildInfo) throw new Error("Build info not found for KlerosCore"); + console.log(buildInfo.output.contracts["src/arbitration/KlerosCore.sol"]["KlerosCore"].storageLayout);contracts/deploy/00-home-chain-arbitration-mainnet.ts (3)
38-43: Drop nonce-based precompute; set SortitionModule.core post-deploy for robustness.Nonce prediction is brittle (any extra tx breaks the offset). Initialize SortitionModule with ZeroAddress (or existing) and call changeCore after KlerosCore is up.
Apply:
- let klerosCoreAddress = await deployments.getOrNull("KlerosCore").then((deployment) => deployment?.address); - if (!klerosCoreAddress) { - const nonce = await ethers.provider.getTransactionCount(deployer); - klerosCoreAddress = getContractAddress(deployer, nonce + 3); // deployed on the 4th tx (nonce+3): SortitionModule Impl tx, SortitionModule Proxy tx, KlerosCore Impl tx, KlerosCore Proxy tx - console.log("calculated future KlerosCore address for nonce %d: %s", nonce + 3, klerosCoreAddress); - } + const existingCoreDeployment = await deployments.getOrNull("KlerosCore"); + let klerosCoreAddress = existingCoreDeployment?.address ?? ZeroAddress;const sortitionModule = await deployUpgradable(deployments, "SortitionModule", { from: deployer, args: [ deployer, - klerosCoreAddress, + klerosCoreAddress, // ZeroAddress if not yet deployed; set below via changeCore minStakingTime, maxFreezingTime, rngWithFallback.target, maxStakePerJuror, maxTotalStaked, ], log: true, }); // nonce (implementation), nonce+1 (proxy)Follow-up addition (outside the ranges above, right after KlerosCore deployment):
const sortition = await hre.ethers.getContract("SortitionModule"); const currentSortitionCore = await sortition.core(); if (ethers.getAddress(currentSortitionCore) !== ethers.getAddress(klerosCore.address)) { console.log(`sortitionModule.changeCore(${klerosCore.address})`); await sortition.changeCore(klerosCore.address); }Also applies to: 50-62
88-93: Normalize address comparisons to avoid checksum-case mismatches.- if (currentCore !== klerosCore.address) { + if (ethers.getAddress(currentCore) !== ethers.getAddress(klerosCore.address)) { console.log(`disputeKit.changeCore(${klerosCore.address})`); await disputeKitContract.changeCore(klerosCore.address); }- if (rngConsumer !== sortitionModule.address) { + if (ethers.getAddress(rngConsumer) !== ethers.getAddress(sortitionModule.address)) { console.log(`rngWithFallback.changeConsumer(${sortitionModule.address})`); await rngWithFallback.changeConsumer(sortitionModule.address); }Also applies to: 96-101
115-119: Mainnet default: enabling arbitrable whitelist — confirm product intent.Turning whitelist on and whitelisting DisputeResolver by default is a governance/product switch; verify this is intended for mainnet rollout (vs. testnets or gated via env).
If needed, gate with network/env:
if (!devnet && process.env.ENABLE_ARBITRABLE_WHITELIST === "true") { await core.changeArbitrableWhitelistEnabled(true); await core.changeArbitrableWhitelist(resolver.address, true); }Also applies to: 120-123
📜 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/deploy/00-home-chain-arbitration-mainnet.ts(7 hunks)contracts/deploy/00-home-chain-arbitration.ts(3 hunks)contracts/deploy/change-sortition-module-rng.ts(2 hunks)contracts/deploy/upgrade-all.ts(2 hunks)contracts/deploy/utils/klerosCoreHelper.ts(1 hunks)contracts/package.json(1 hunks)contracts/scripts/generateDeploymentsMarkdown.sh(2 hunks)contracts/scripts/storage-layout.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/scripts/generateDeploymentsMarkdown.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/deploy/00-home-chain-arbitration.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-07T11:39:10.927Z
Learnt from: jaybuidl PR: kleros/kleros-v2#1778 File: contracts/src/rng/ChainlinkRNG.sol:135-153 Timestamp: 2024-12-07T11:39:10.927Z Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context. Applied to files:
contracts/deploy/change-sortition-module-rng.ts
🧬 Code graph analysis (2)
contracts/deploy/upgrade-all.ts (1)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
contracts/deploy/00-home-chain-arbitration-mainnet.ts (3)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deployments/utils.ts (1)
deployments(3-16)contracts/deploy/utils/index.ts (2)
isDevnet(30-30)PNK(40-40)
⏰ 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). (6)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (6)
contracts/deploy/upgrade-all.ts (1)
39-41: Confirm compare-storage target matches the implementation artifact nameIf contractName here is a proxy alias rather than the implementation FQN/name, storage checks may be misleading. Please verify your mapping returns the implementation name.
contracts/deploy/00-home-chain-arbitration-mainnet.ts (5)
9-9: Type imports align with non‑Neo migration — LGTM.
32-36: DisputeKitClassic deploy pattern (ZeroAddress core, then changeCore) — LGTM.
68-85: Deployment args match initializer signature; no action required. Theargsarray aligns exactly withinitialize(address, address, IERC20, address, IDisputeKit, bool, uint256[4], uint256[4], bytes, ISortitionModule, address, IERC721)in KlerosCore.sol.
158-162: Confirm and use the correct RNGWithFallback deploy tag in dependencies
I couldn’t find any script assigning.tags = ["RNGWithFallback"]; locate the deploy script that registers the fallback consumer, note its exact tag, and then update:-deployArbitration.dependencies = ["ChainlinkRNG"]; +deployArbitration.dependencies = ["<CorrectFallbackTag>", "ChainlinkRNG"];
102-107: changeCurrencyRate parameters align with core ABI
The helper’s signature ischangeCurrencyRate(core, erc20: string, accepted: boolean, rateInEth: BigNumberish, rateDecimals: number)and it calls
core.changeCurrencyRates(erc20, rateInEth, rateDecimals)which matches the contract’s
function changeCurrencyRates(IERC20 _feeToken, uint64 _rateInEth, uint8 _rateDecimals)– no reordering needed. (git.instadapp.io)
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 (3)
contracts/scripts/keeperBot.ts (1)
279-286: Static-call to BlockHashRNG.receiveRandomness can revert (state-changing function)
receiveRandomnesscommonly writes to storage;staticCall()would then revert, breaking readiness checks. Wrap in try/catch and fall back to a pure/view readiness probe (e.g.,lastRequestId+randomNumbers). This keeps behavior consistent with Chainlink/Randomizer branches and avoids false negatives.- } else if (currentRng === blockHashRNG?.target && blockHashRNG !== null) { - const n = await blockHashRNG.receiveRandomness.staticCall(); + } else if (currentRng === blockHashRNG?.target && blockHashRNG !== null) { + let n: bigint; + try { + n = await blockHashRNG.receiveRandomness.staticCall(); + } catch { + // Fallback to a view-based readiness check to avoid STATICCALL reverts + const requestID = await blockHashRNG.lastRequestId(); + n = await blockHashRNG.randomNumbers(requestID); + } if (Number(n) === 0) { logger.info("BlockHashRNG is NOT ready yet"); return false; } else { logger.info(`BlockHashRNG is ready: ${n.toString()}`); return true; }contracts/scripts/changeOwner.ts (1)
13-15: Refuse the zero address explicitly.Prevent accidental ownership burns.
- if (!isAddress(newOwner)) { + if (!isAddress(newOwner)) { throw new Error("Invalid owner address provided"); } + if (newOwner.toLowerCase() === "0x0000000000000000000000000000000000000000") { + throw new Error("Refusing to set owner to the zero address"); + }contracts/scripts/populateCourts.ts (1)
107-113: Bug: post-increment returns the unshifted value and mutates input.
court.id++andcourt.parent++return the original values (and mutatecourt). You want shifted values without side effects.- const parametersV1ToV2 = (court: Court): Court => ({ - ...court, - id: court.id++, - parent: court.parent++, - }); + const parametersV1ToV2 = (court: Court): Court => ({ + ...court, + id: court.id + 1, + parent: court.parent + 1, + });
🧹 Nitpick comments (10)
contracts/scripts/keeperBot.ts (1)
51-52: Guard against missing SortitionModule at runtimeCasting hides the possibility of an undefined/null
sortition. Add a runtime assert to fail fast if not deployed.- return { ...contracts, sortition: contracts.sortition as SortitionModule }; + if (!contracts.sortition) { + throw new Error("SortitionModule not deployed"); + } + return { ...contracts, sortition: contracts.sortition as SortitionModule };contracts/scripts/utils/contracts.ts (4)
57-57: Tighten the error text.Remove the extra comma for polish.
- if (!(coreType in coreSpecificNames)) throw new Error("Invalid core type, must be one of BASE, or UNIVERSITY"); + if (!(coreType in coreSpecificNames)) throw new Error("Invalid core type, must be one of BASE or UNIVERSITY");
79-117: Avoid repeated lookups of contract names.Cache
getContractNames(coreType)once; it reduces repetition and the risk of name drift.export const getContracts = async (hre: HardhatRuntimeEnvironment, coreType: Core) => { const { ethers } = hre; - let core: KlerosCore | KlerosCoreUniversity; - let sortition: SortitionModule | SortitionModuleUniversity; + const names = getContractNames(coreType); + let core: KlerosCore | KlerosCoreUniversity; + let sortition: SortitionModule | SortitionModuleUniversity; switch (coreType) { case Cores.BASE: - core = await ethers.getContract<KlerosCore>(getContractNames(coreType).core); - sortition = await ethers.getContract<SortitionModule>(getContractNames(coreType).sortition); + core = await ethers.getContract<KlerosCore>(names.core); + sortition = await ethers.getContract<SortitionModule>(names.sortition); break; case Cores.UNIVERSITY: - core = await ethers.getContract<KlerosCoreUniversity>(getContractNames(coreType).core); - sortition = await ethers.getContract<SortitionModuleUniversity>(getContractNames(coreType).sortition); + core = await ethers.getContract<KlerosCoreUniversity>(names.core); + sortition = await ethers.getContract<SortitionModuleUniversity>(names.sortition); break; default: throw new Error("Invalid core type, must be one of BASE, or UNIVERSITY"); } - const disputeKitClassic = await ethers.getContract<DisputeKitClassic>(getContractNames(coreType).disputeKitClassic); - const disputeKitShutter = await ethers.getContractOrNull<DisputeKitShutter>( - getContractNames(coreType).disputeKitShutter - ); - const disputeKitGated = await ethers.getContractOrNull<DisputeKitGated>(getContractNames(coreType).disputeKitGated); - const disputeKitGatedShutter = await ethers.getContractOrNull<DisputeKitGatedShutter>( - getContractNames(coreType).disputeKitGatedShutter - ); - const disputeResolver = await ethers.getContract<DisputeResolver>(getContractNames(coreType).disputeResolver); - const disputeTemplateRegistry = await ethers.getContract<DisputeTemplateRegistry>( - getContractNames(coreType).disputeTemplateRegistry - ); - const evidence = await ethers.getContract<EvidenceModule>(getContractNames(coreType).evidence); - const policyRegistry = await ethers.getContract<PolicyRegistry>(getContractNames(coreType).policyRegistry); - const batcher = await ethers.getContract<TransactionBatcher>(getContractNames(coreType).batcher); - const chainlinkRng = await ethers.getContractOrNull<ChainlinkRNG>(getContractNames(coreType).chainlinkRng); - const randomizerRng = await ethers.getContractOrNull<RandomizerRNG>(getContractNames(coreType).randomizerRng); - const blockHashRNG = await ethers.getContractOrNull<BlockHashRNG>(getContractNames(coreType).blockHashRNG); - const pnk = await ethers.getContract<PNK>(getContractNames(coreType).pnk); - const snapshotProxy = await ethers.getContractOrNull<KlerosCoreSnapshotProxy>( - getContractNames(coreType).snapshotProxy - ); + const disputeKitClassic = await ethers.getContract<DisputeKitClassic>(names.disputeKitClassic); + const disputeKitShutter = await ethers.getContractOrNull<DisputeKitShutter>(names.disputeKitShutter); + const disputeKitGated = await ethers.getContractOrNull<DisputeKitGated>(names.disputeKitGated); + const disputeKitGatedShutter = await ethers.getContractOrNull<DisputeKitGatedShutter>(names.disputeKitGatedShutter); + const disputeResolver = await ethers.getContract<DisputeResolver>(names.disputeResolver); + const disputeTemplateRegistry = await ethers.getContract<DisputeTemplateRegistry>(names.disputeTemplateRegistry); + const evidence = await ethers.getContract<EvidenceModule>(names.evidence); + const policyRegistry = await ethers.getContract<PolicyRegistry>(names.policyRegistry); + const batcher = await ethers.getContract<TransactionBatcher>(names.batcher); + const chainlinkRng = await ethers.getContractOrNull<ChainlinkRNG>(names.chainlinkRng); + const randomizerRng = await ethers.getContractOrNull<RandomizerRNG>(names.randomizerRng); + const blockHashRNG = await ethers.getContractOrNull<BlockHashRNG>(names.blockHashRNG); + const pnk = await ethers.getContract<PNK>(names.pnk); + const snapshotProxy = await ethers.getContractOrNull<KlerosCoreSnapshotProxy>(names.snapshotProxy);
144-149: Broaden network aliases and improve the error.Support common aliases like hardhat/localhost and print the unknown network in the error.
-export const getContractsFromNetwork = async (hre: HardhatRuntimeEnvironment) => { +export const getContractsFromNetwork = async (hre: HardhatRuntimeEnvironment) => { const { network } = hre; - if (["arbitrumSepoliaDevnet", "arbitrumSepolia", "arbitrum"].includes(network.name)) { + const baseNetworks = new Set(["arbitrumSepoliaDevnet", "arbitrumSepolia", "arbitrum", "arbitrumOne", "hardhat", "localhost"]); + if (baseNetworks.has(network.name)) { return getContracts(hre, Cores.BASE); } else { - throw new Error("Invalid network"); + throw new Error(`Invalid or unsupported network: ${network.name}`); } }
158-163: Mirror the alias handling for names.- if (["arbitrumSepoliaDevnet", "arbitrumSepolia", "arbitrum"].includes(network.name)) { + const baseNetworks = new Set(["arbitrumSepoliaDevnet", "arbitrumSepolia", "arbitrum", "arbitrumOne", "hardhat", "localhost"]); + if (baseNetworks.has(network.name)) { return getContractNames(Cores.BASE); } else { - throw new Error("Invalid network"); + throw new Error(`Invalid or unsupported network: ${network.name}`); }contracts/scripts/changeOwner.ts (2)
65-75: Skip absent optional contracts instead of failing noisy.Guard
KlerosCoreSnapshotProxylike other optional contracts.- await updateOwner("KlerosCoreSnapshotProxy", snapshotProxy); + if (snapshotProxy) await updateOwner("KlerosCoreSnapshotProxy", snapshotProxy);
48-63: Surface missing method errors clearly.If a contract lacks
changeOwner, fail fast with a clearer message to reduce confusion across heterogeneous Ownable patterns.- try { - const tx = await contractInstance.changeOwner(newOwner); + try { + if (!contractInstance?.changeOwner) { + throw new Error("Contract does not implement changeOwner(address)"); + } + const tx = await contractInstance.changeOwner(newOwner); await tx.wait();contracts/scripts/populatePolicyRegistry.ts (1)
56-57: ValidatetaskArgs.fromearly with a helpful hint.Tiny improvement: echo the received value to speed up CLI troubleshooting.
- console.error("Invalid source, must be one of v1_mainnet, v1_gnosis, v2_devnet, v2_testnet, v2_mainnet"); + console.error(`Invalid source "${taskArgs.from}", must be one of v1_mainnet, v1_gnosis, v2_devnet, v2_testnet, v2_mainnet`);contracts/scripts/populateCourts.ts (2)
242-268: Use configured dispute kits when present.If
supportedDisputeKitsexists in the JSON, pass it through; default to classic otherwise.- [DISPUTE_KIT_CLASSIC] + court.supportedDisputeKits?.length ? court.supportedDisputeKits : [DISPUTE_KIT_CLASSIC]
255-266: Avoid magic number; name Sortition K.Make the hard-coded
5explicit.- ethers.toBeHex(5), // Not accessible on-chain, but has always been set to the same value so far. + ethers.toBeHex(5), // SORTITION_KOr define a
const SORTITION_K = 5;near the other constants and use it here.
📜 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 (6)
contracts/scripts/changeOwner.ts(2 hunks)contracts/scripts/keeperBot.ts(2 hunks)contracts/scripts/keeperBotShutter.ts(2 hunks)contracts/scripts/populateCourts.ts(7 hunks)contracts/scripts/populatePolicyRegistry.ts(4 hunks)contracts/scripts/utils/contracts.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-07T11:39:10.927Z
Learnt from: jaybuidl PR: kleros/kleros-v2#1778 File: contracts/src/rng/ChainlinkRNG.sol:135-153 Timestamp: 2024-12-07T11:39:10.927Z Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context. Applied to files:
contracts/scripts/keeperBot.ts
📚 Learning: 2024-11-19T17:18:39.007Z
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-5 Timestamp: 2024-11-19T17:18:39.007Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews. Applied to files:
contracts/scripts/populateCourts.ts
📚 Learning: 2024-11-19T16:09:41.467Z
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-17 Timestamp: 2024-11-19T16:09:41.467Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently. Applied to files:
contracts/scripts/populateCourts.ts
🧬 Code graph analysis (1)
contracts/scripts/utils/contracts.ts (3)
contracts/deployments/index.ts (2)
getContracts(18-18)getContracts(19-19)contracts/deployments/contractsEthers.ts (1)
getContracts(172-278)contracts/deployments/contractsViem.ts (1)
getContracts(234-342)
⏰ 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). (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (6)
contracts/scripts/keeperBotShutter.ts (2)
3-3: LGTM: imports trimmed to non‑Neo typesRemoving Neo imports aligns with the consolidation effort. No issues.
315-316: LGTM: simplified getContracts passthroughReturning the raw
getContractsForCoreTyperesult is fine here and keeps this script decoupled from Sortition typing.contracts/scripts/populatePolicyRegistry.ts (2)
22-23: Enum rename looks consistent.The swap to
V2_MAINNETand the new JSON import align with the repo-wide Neo removal.
91-93: LGTM on mainnet source switch.Switching to
policies.v2.mainnet.jsonmatches the new enum and CLI help text.contracts/scripts/populateCourts.ts (2)
56-56: CLI help text: good.
coreTypenow only lists base/university; consistent with script utilities.
136-138: Mainnet config swap is correct.Using
courts.v2.mainnet.jsonaligns with the PR objective and prior configs. Note: per our earlier learning, a self-parent for the General Court (id 1) is intentional—no action needed.
jaybuidl force-pushed the feat/migration-no-more-neo branch from a61e521 to daa8bf9 Compare 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 (8)
contracts/scripts/generateDeploymentArtifact.sh (3)
11-12: Validate the address format to prevent query-parameter injection.Unvalidated
$addresscan inject&...into the query and corrupt the request. Validate EVM address shape.Apply:
network=$1 address=$2 + +# Basic EVM address validation +if [[ ! "$address" =~ ^0x[0-9a-fA-F]{40}$ ]]; then + echo "error: invalid address: $address" + exit 1 +fi
51-55: URL-encode address before building the query.Even with validation, URI-encoding is safer and future-proof.
Apply:
-query="$url/api?module=contract&action=getabi&address=$address" +# URI-encode address to be safe in querystring +encoded_address=$(jq -rn --arg s "$address" '$s|@uri') +query="$url/api?module=contract&action=getabi&address=$encoded_address" if [[ -n $apiKey ]] then query="$query&apikey=$apiKey" fi
57-64: Harden network/JSON error handling and parse ABI correctly.
- Add curl timeouts/retries and fail on HTTP errors.
- Use
fromjsonto parse Etherscan-style stringified ABI.- Fall back to
[]only on verified failure; otherwise exit on transport errors.Apply:
-result=$(curl -s "$query") -if [[ $(echo "$result" | jq -r .status) == 0 ]] -then - echo "error: contract not verified or does not exist" - abi="[]" -else - abi=$(echo "$result" | jq -r .result) -fi +if ! result=$(curl -sSsfL --connect-timeout 10 --max-time 20 --retry 2 --retry-all-errors "$query"); then + echo "error: failed to fetch ABI from explorer" + exit 1 +fi +# If explorer says not verified or returns invalid JSON, emit empty ABI. +if ! abi=$(jq -re 'select(.status=="1") | .result | fromjson' <<<"$result" 2>/dev/null); then + echo "warning: contract not verified or ABI unavailable; emitting empty ABI" + abi='[]' +fi @@ jq \ --arg address "$address" \ --argjson abi "$abi" \ '{ "address": $address, "abi": $abi }' <<< '{}'Also applies to: 66-69
contracts/test/integration/getContractsEthers.test.ts (1)
100-117: Replace leftover "Neo" contract names in mainnet mappings/testsmainnetContractMapping in tests still uses Neo‑suffixed artifact names — update to the unified contract names to avoid drift with deployments/address files.
-const mainnetContractMapping: ContractMapping = { - klerosCore: { name: "KlerosCoreNeo" }, - sortition: { name: "SortitionModuleNeo" }, - disputeKitClassic: { name: "DisputeKitClassicNeo" }, - disputeKitShutter: { name: "DisputeKitShutterNeo" }, - disputeKitGated: { name: "DisputeKitGatedNeo" }, - disputeKitGatedShutter: { name: "DisputeKitGatedShutterNeo" }, - disputeResolver: { name: "DisputeResolverNeo" }, +const mainnetContractMapping: ContractMapping = { + klerosCore: { name: "KlerosCore" }, + sortition: { name: "SortitionModule" }, + disputeKitClassic: { name: "DisputeKitClassic" }, + disputeKitShutter: { name: "DisputeKitShutter" }, + disputeKitGated: { name: "DisputeKitGated" }, + disputeKitGatedShutter: { name: "DisputeKitGatedShutter" }, + disputeResolver: { name: "DisputeResolver" },Locations to fix:
- contracts/test/integration/getContractsEthers.test.ts (lines ~100–117) — apply the diff above.
- contracts/test/integration/getContractsViem.test.ts (lines ~79–87) — apply the same replacements.
Also note: repository deployment artifacts and README still contain many Neo‑named entries (contracts/README.md, contracts/deployments/arbitrum*, contracts/deployments/arbitrum.ts). Decide whether to (A) keep deployment keys as-is and map to them, or (B) rename deployment keys/artifacts to the unified names — verify deployments/address consumers before changing artifacts.
contracts/test/integration/getContractsViem.test.ts (1)
80-97: Replace Neo suffix in mainnetContractMapping in tests
- In contracts/test/integration/getContractsViem.test.ts (lines 81–87) and contracts/test/integration/getContractsEthers.test.ts (lines 101–107), remove the “Neo” suffix from each name in mainnetContractMapping:
• KlerosCoreNeo → KlerosCore
• SortitionModuleNeo → SortitionModule
• DisputeKitClassicNeo → DisputeKitClassic
• DisputeKitShutterNeo → DisputeKitShutter
• DisputeKitGatedNeo → DisputeKitGated
• DisputeKitGatedShutterNeo → DisputeKitGatedShutter
• DisputeResolverNeo → DisputeResolvercontracts/scripts/utils/contracts.ts (3)
66-70: Standardize naming: use blockHashRng (camel ‘Rng’) for consistency across repo.Other modules export
blockHashRng; this file usesblockHashRNG(all-caps). This divergence is a footgun for consumers.- blockHashRNG: "BlockHashRNG", + blockHashRng: "BlockHashRNG",
110-114: Follow-through on blockHashRng rename in locals.Align local var and lookup to
blockHashRng.- const blockHashRNG = await ethers.getContractOrNull<BlockHashRNG>(getContractNames(coreType).blockHashRNG); + const blockHashRng = await ethers.getContractOrNull<BlockHashRNG>(getContractNames(coreType).blockHashRng);
128-133: Return shape consistency: expose blockHashRng, not blockHashRNG.Keep the public API consistent with deployments/* helpers.
- blockHashRNG, + blockHashRng,
🧹 Nitpick comments (11)
contracts/scripts/generateDeploymentArtifact.sh (3)
5-9: Enforce exactly two args (not “at least two”).Extra args are silently ignored; fail fast for misuse.
Apply:
-if [[ $# -lt 2 ]] +if [[ $# -ne 2 ]] then echo "usage: $(basename $0) network address" exit 1 fi
46-49: Improve UX: print supported networks on error.Helps callers discover valid options.
Apply:
*) echo "error: unknown network $network" + echo "supported: gnosischain chiado arbitrum arbitrumSepolia mainnet sepolia" exit 1 esac
1-4: Enable strict bash mode for safer scripting.Catches unset vars and early failures.
Apply:
#!/usr/bin/env bash +set -Eeuo pipefail +IFS=$'\n\t' SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"contracts/test/integration/getContractsEthers.test.ts (1)
204-220: Handle undefined optionals in deployed-address checksOptional contracts can be undefined (not only null). Current check may throw when contract is undefined.
- const contract = contracts[key as keyof typeof contracts]; - if (contract === null) { + const contract = contracts[key as keyof typeof contracts]; + if (contract == null) { if (!optional) { throw new Error(`Required contract ${name} is null`); } continue; }contracts/scripts/utils/contracts.ts (4)
57-58: Tighten error copy and remove extraneous comma.Message reads oddly; suggest “Invalid core type, must be one of BASE or UNIVERSITY”.
- if (!(coreType in coreSpecificNames)) throw new Error("Invalid core type, must be one of BASE, or UNIVERSITY"); + if (!(coreType in coreSpecificNames)) throw new Error("Invalid core type, must be one of BASE or UNIVERSITY");
144-149: Improve diagnostics: include network name in error.Eases debugging when running under unexpected networks.
- } else { - throw new Error("Invalid network"); - } + } else { + throw new Error(`Invalid network: ${network.name}`); + }
158-164: Mirror the improved error message here as well.- } else { - throw new Error("Invalid network"); - } + } else { + throw new Error(`Invalid network: ${network.name}`); + }
142-149: Optional: consider supporting ‘hardhat’/‘localhost’ for BASE inference.Common DX improvement; map them to BASE or gate behind an env flag.
contracts/deployments/utils.ts (1)
25-29: More informative error from getAddress.List available chainIds to speed up misconfig debugging.
export function getAddress(config: ContractConfig, chainId: number): `0x${string}` { const address = config.address[chainId]; - if (!address) throw new Error(`No address found for chainId ${chainId}`); + if (!address) { + const available = Object.keys(config.address).join(", "); + throw new Error(`No address found for chainId ${chainId}. Available: [${available}]`); + } return address; }contracts/deployments/contractsEthers.ts (2)
168-174: Optional: dedupe config-to-contract wiring with viem version.Both files implement near-identical mapping; consider centralizing to reduce drift.
41-57: Remove "Neo" from deployment export names (optional refactor).Viem and Ethers deployment modules re-export symbols suffixed with "Neo" — e.g. klerosCoreNeoConfig, sortitionModuleNeoConfig, disputeKitClassicNeoConfig — defined in contracts/deployments/mainnet.viem.ts and re‑exported in contracts/deployments/contractsEthers.ts and contracts/deployments/contractsViem.ts. Rename the exports and re-exports (klerosCoreNeoConfig → klerosCoreConfig, sortitionModuleNeoConfig → sortitionModuleConfig, etc.) to keep terminology consistent with the de-neo-ification effort.
📜 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 (13)
contracts/deployments/contractsEthers.ts(2 hunks)contracts/deployments/contractsViem.ts(1 hunks)contracts/deployments/utils.ts(1 hunks)contracts/scripts/changeOwner.ts(2 hunks)contracts/scripts/generateDeploymentArtifact.sh(1 hunks)contracts/scripts/generateDeploymentsMarkdown.sh(3 hunks)contracts/scripts/keeperBot.ts(2 hunks)contracts/scripts/keeperBotShutter.ts(2 hunks)contracts/scripts/populateCourts.ts(7 hunks)contracts/scripts/populatePolicyRegistry.ts(4 hunks)contracts/scripts/utils/contracts.ts(5 hunks)contracts/test/integration/getContractsEthers.test.ts(3 hunks)contracts/test/integration/getContractsViem.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- contracts/scripts/keeperBot.ts
- contracts/scripts/populatePolicyRegistry.ts
- contracts/scripts/generateDeploymentsMarkdown.sh
- contracts/scripts/populateCourts.ts
- contracts/scripts/changeOwner.ts
- contracts/scripts/keeperBotShutter.ts
🧰 Additional context used
🧬 Code graph analysis (4)
contracts/test/integration/getContractsViem.test.ts (3)
contracts/deployments/contractsEthers.ts (1)
getContracts(168-274)contracts/deployments/contractsViem.ts (1)
getContracts(234-342)contracts/deployments/index.ts (2)
getContracts(18-18)getContracts(19-19)
contracts/test/integration/getContractsEthers.test.ts (2)
contracts/deployments/contractsEthers.ts (1)
getContracts(168-274)contracts/deployments/contractsViem.ts (1)
getContracts(234-342)
contracts/scripts/utils/contracts.ts (3)
contracts/deployments/contractsEthers.ts (1)
getContracts(168-274)contracts/deployments/contractsViem.ts (1)
getContracts(234-342)contracts/deployments/index.ts (2)
getContracts(18-18)getContracts(19-19)
contracts/deployments/contractsEthers.ts (1)
contracts/deployments/utils.ts (1)
getAddress(25-29)
⏰ 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). (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (8)
contracts/scripts/generateDeploymentArtifact.sh (1)
5-9: Good fix: use numeric arg comparison (-lt).Prevents subtle misbehavior for 10+ args; invocation contract unchanged.
contracts/test/integration/getContractsEthers.test.ts (1)
292-294: LGTM: mainnet test now targets unified factories and mappingUsing KlerosCore__factory/SortitionModule__factory and the mainnet deployment is correct post-consolidation.
Also applies to: 300-301, 311-311
contracts/test/integration/getContractsViem.test.ts (1)
243-247: LGTM: mainnet path now uses "mainnet" deploymentThe test correctly points to the unified mainnet deployment.
contracts/deployments/contractsViem.ts (2)
206-228: Rename to mainnet looks correct and aligns with deployments/utils.ts.Switch label and configs selection are coherent; randomizer RNG only on mainnet is preserved.
42-58: Do not rename only mainnet.viem exports — 'Neo' identifiers are used repo‑wide; perform a project‑wide audit/rename.rg shows many "Neo" occurrences (subgraph/core-neo/, web/src/hooks/useGenesisBlock.ts, contracts/test/integration/, and numerous deployment artifacts under contracts/deployments/arbitrum/*.json and .ts). Changing the aliases in contracts/deployments/contractsViem.ts (lines 42–58) alone will break imports/fixtures/subgraphs/tests.
Actionable options:
- Keep current aliasing, or
- Do a project‑wide rename: update mainnet.viem exports AND all references (tests, web, subgraph templates, deployment JSON/TS). I can prepare a follow‑up PR to do this and update references.
Quick audit command: rg -nP '\bNeo\b' -g '!/node_modules/'
contracts/deployments/utils.ts (1)
13-16: LGTM: deployments key renamed to mainnet.Type narrowing via DeploymentName updates accordingly; consistent with callers.
contracts/deployments/contractsEthers.ts (2)
170-172: Type unions updated correctly.Removal of Neo variants reflected in return types; no API break internally.
246-249: Mainnet branch now connects to unified contracts — good.Correct factories and address configs are selected.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/test/integration/getContractsEthers.test.ts (2)
306-307: Use “exist” to catch both null and undefined for required RNGs.Stronger assertion for mainnet presence.
- expect(contracts.chainlinkRng).to.not.be.null; - expect(contracts.randomizerRng).to.not.be.null; + expect(contracts.chainlinkRng).to.exist; + expect(contracts.randomizerRng).to.exist;
100-117: Use a loose null check for both null and undefined
Switch the condition fromif (contract === null)toif (contract == null)and update the thrown message to"Required contract ${name} is null or undefined".
📜 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 (4)
contracts/deployments/contractsEthers.ts(3 hunks)contracts/deployments/contractsViem.ts(2 hunks)contracts/test/integration/getContractsEthers.test.ts(3 hunks)contracts/test/integration/getContractsViem.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/test/integration/getContractsViem.test.ts
- contracts/deployments/contractsEthers.ts
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/test/integration/getContractsEthers.test.ts (2)
contracts/deployments/contractsEthers.ts (1)
getContracts(168-274)contracts/deployments/contractsViem.ts (1)
getContracts(234-342)
⏰ 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). (15)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: SonarCloud
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
contracts/test/integration/getContractsEthers.test.ts (1)
300-301: Factory switch to non-Neo contracts is correct.Good replacement of Neo factories with
KlerosCore__factoryandSortitionModule__factoryfor mainnet.contracts/deployments/contractsViem.ts (2)
42-48: Mainnet exports and imports verified
All renamed exports exist in mainnet.viem.ts and nomainnetNeoorNeoConfigreferences remain.
206-227: Remove legacymainnetNeoconcern—no occurrences found
All code and tests now use"mainnet", andDeploymentNamederives from the updateddeploymentskeys. No migration shim required.
PR-Codex overview
This PR primarily focuses on refactoring the Kleros smart contracts and their associated deployment scripts by removing references to the
Neoversions, consolidating configurations for themainnet, and updating various scripts and tests accordingly.Detailed summary
KlerosCoreNeo,SortitionModuleNeo, and related contracts.KlerosCoreandSortitionModule.mainnetreferences.Neoversions.Summary by CodeRabbit
New Features
Chores
Tests