Release #2076
Release #2076
Conversation
…use of @wagmi/cli
Feat: Dispute Kits helper in contracts package
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Enabled Hardhat viaIR compilation with solc v0.8.30
bumped @kleros/vea-contracts to v0.7.0
chore: viaIR compilation enabled for Foundry with explicit solc v0.8.30
… deprecated for later removal
feat: court llms.txt
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a Viem helper and script to enumerate dispute kits; converts RNG to a zero-arg IRNG with RNGWithFallback and timestamped BlockhashRNG; replaces string requires with typed errors; introduces ONE_BASIS_POINT and split coherence APIs; updates tooling, deployments, many contracts, tests, and frontend gated-token validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as getDisputeKits.ts
participant Client as Viem PublicClient
participant Helper as disputeKitsViem.getDisputeKits
participant Chain as KlerosCore (on-chain)
Note over Script,Client: init RPC client from ARBITRUM_SEPOLIA_RPC
Script->>Helper: getDisputeKits(client, "devnet")
Helper->>Chain: fetch DisputeKitCreated logs (filter)
Chain-->>Helper: logs
Helper->>Helper: decode logs → id→address map
Helper->>Helper: correlate addresses with local artifacts
Helper-->>Script: return DisputeKitByIds (address, contract?, isGated, isShutter)
sequenceDiagram
participant Sort as SortitionModule (consumer)
participant RF as RNGWithFallback
participant RNGP as Primary RNG (IRNG)
participant BH as BlockhashRNG (fallback)
Note over Sort,RF: SortitionModule requests randomness
Sort->>RF: requestRandomness()
RF->>RNGP: requestRandomness()
Note over RNGP: primary RNG fulfills asynchronously or later
Sort->>RF: receiveRandomness()
alt primary ready (non-zero)
RF->>RNGP: receiveRandomness()
RNGP-->>RF: random
RF-->>Sort: return primary random
else timed out or primary returned 0
RF->>BH: derive fallback from recent blockhash
RF-->>Sort: return fallback random (emit RNGFallback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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-university failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-neo-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
contracts/deploy/change-arbitrable-dispute-template.ts (1)
36-38: Fix:instanceof EventLogwill throw/fail under ethers v6
EventLogis a type (not a runtime class/constructor) in ethers v6, soevent instanceof EventLogwill either always be false or throw “Right-hand side of 'instanceof' is not callable”. Use a structural check (e.g.,'args' in event) or parse logs via the contract interface.Apply this minimal fix:
- tx?.logs?.forEach((event) => { - if (event instanceof EventLog) console.log("event: %O", event.args); - }); + tx?.logs?.forEach((event) => { + if (typeof event === "object" && event !== null && "args" in event) { + console.log("event: %O", (event as any).args); + } else { + console.log("log: %O", event); + } + });contracts/test/arbitration/staking.ts (3)
77-77: Use BigInt literals in assertions to match ethers v6 return typesSeveral assertions compare BigInt-returning values to number literals (e.g., 0, 2). With ethers v6, Solidity uints decode to bigint, and chai deep equality will fail across types. Replace number literals with BigInt (0n, 2n, etc.).
Representative fixes below (apply similarly to other occurrences in this file):
- expect(await sortition.getJurorBalance(deployer, 2)).to.be.deep.equal([PNK(4000), 0, PNK(2000), 2]); + expect(await sortition.getJurorBalance(deployer, 2)).to.be.deep.equal([PNK(4000), 0n, PNK(2000), 2n]);- expect(await sortition.getJurorBalance(deployer, 2)).to.be.deep.equal([ + expect(await sortition.getJurorBalance(deployer, 2)).to.be.deep.equal([ PNK(5000), PNK(300), // we're the only juror so we are drawn 3 times PNK(3000), - 2, + 2n, ]); // stake unchanged, delayed- expect(await sortition.getJurorBalance(deployer, 2)).to.be.deep.equal([PNK(4000), 0, PNK(2000), 2]); + expect(await sortition.getJurorBalance(deployer, 2)).to.be.deep.equal([PNK(4000), 0n, PNK(2000), 2n]);- expect(await sortition.getJurorBalance(deployer, 2)).to.be.deep.equal([ + expect(await sortition.getJurorBalance(deployer, 2)).to.be.deep.equal([ PNK(3000), PNK(300), // we're the only juror so we are drawn 3 times PNK(1000), - 2, + 2n, ]); // stake unchanged, delayed- expect(await sortition.getJurorBalance(deployer, 2)).to.be.deep.equal([ + expect(await sortition.getJurorBalance(deployer, 2)).to.be.deep.equal([ PNK(4000), PNK(300), // we're the only juror so we are drawn 3 times PNK(2000), - 2, + 2n, ]); // stake unchanged, delayedAnd similarly for the other repeated getJurorBalance deep.equal checks in this file.
Also applies to: 89-89, 115-120, 142-142, 178-183, 265-270, 291-293, 326-327, 353-358
123-124: Normalize delayedStakes assertions to BigIntLikewise, delayedStakes returns uints (BigInt). Replace 0 with 0n for the numeric fields:
- expect(await sortition.delayedStakes(1)).to.be.deep.equal([ethers.ZeroAddress, 0, 0, false]); // the 1st delayed stake got deleted + expect(await sortition.delayedStakes(1)).to.be.deep.equal([ethers.ZeroAddress, 0n, 0n, false]); // the 1st delayed stake got deletedApply the same change to the other delayedStakes[...] assertions in this file.
Also applies to: 186-188, 273-275, 361-363
386-386: BigInt expected for getJurorCourtIDsConsistently use BigInt literals for court IDs to match ethers v6 decoding:
- expect(await sortition.getJurorCourtIDs(deployer)).to.be.deep.equal([1, 2]); + expect(await sortition.getJurorCourtIDs(deployer)).to.be.deep.equal([1n, 2n]);contracts/scripts/disputeRelayerBot.ts (2)
107-109: Bug: missing await on tx.wait() leads to broken logging and race on mining
tx = tx.wait();assigns a Promise totx. The next line attempts to readtransactionHashfrom a Promise, which will be undefined and can crash or log incorrectly. Also, you likely want to ensure the tx is mined before proceeding/logging.Apply this fix:
- tx = tx.wait(); - logger.info(`relayCreateDispute txId: ${tx.transactionHash}`); + const receipt = await tx.wait(); + logger.info(`relayCreateDispute txId: ${receipt.transactionHash}`);
71-78: Guard against missing DisputeRequest logs and fix unconditional warning
- If no DisputeRequest logs are present,
disputeRequests[0]is undefined and subsequent property access will throw.- The warning "More than 1 DisputeRequest event..." is logged unconditionally; it should be conditional and the message can be clearer.
Apply:
- const disputeRequests: DisputeRequestEventObject[] = fullTxReceipt.logs + const disputeRequests: DisputeRequestEventObject[] = fullTxReceipt.logs .filter((log) => log.topics[0] === arbitrableInterface.getEventTopic("DisputeRequest")) .map((log) => arbitrableInterface.parseLog(log).args as unknown as DisputeRequestEventObject); - logger.warn(`More than 1 DisputeRequest event: not supported yet, skipping the others events.`); + if (disputeRequests.length === 0) { + logger.error("No DisputeRequest events found in transaction; skipping."); + return; + } + if (disputeRequests.length > 1) { + logger.warn("More than one DisputeRequest event found in tx; not supported yet — skipping others."); + } const disputeRequest = disputeRequests[0]; logger.info(`tx events DisputeRequest: ${JSON.stringify(disputeRequest)}`);contracts/test/arbitration/dispute-kit-gated.ts (1)
24-25: Name collision: local PNK() helper shadows imported PNK type (compile error)You import PNK from typechain-types (used as the contract type), then declare a const PNK helper. This redeclares the identifier and will fail to compile.
Rename the helper (or import the type via
import type { PNK as PNKToken }and update its usage). Minimal change below renames the helper and its only usage.Apply:
- const PNK = (amount: BigNumberish) => toBigInt(amount) * 10n ** 18n; + const pnkUnits = (amount: BigNumberish) => toBigInt(amount) * 10n ** 18n; @@ - const minStake = PNK(200); + const minStake = pnkUnits(200);Alternative (optional): make the import type-only to avoid value-namespace collisions.
-import { - PNK, +import type { + PNK, KlerosCore, SortitionModule, IncrementalNG, DisputeKitGated, TestERC20, TestERC721, TestERC1155, } from "../../typechain-types";Also applies to: 30-31, 40-41
contracts/deploy/00-home-chain-arbitration-neo.ts (1)
47-58: Ensure SortitionModuleNeo.initialize args include the RNG lookahead
Theinitializefunction in contracts/src/arbitration/SortitionModuleNeo.sol still expects eight parameters—specifically a_rngLookaheadvalue between_rngand_maxStakePerJuror. The deploy script is currently passing:
- deployer (governor)
- klerosCoreAddress (core)
- minStakingTime
- maxDrawingTime
- rngWithFallback.target
- maxStakePerJuror
- maxTotalStaked
…which omits the
rngLookaheadargument and shifts all subsequent parameters out of place.Locations to update:
- contracts/deploy/00-home-chain-arbitration-neo.ts (the
argsarray forSortitionModuleNeo)Suggested diff:
const sortitionModule = await deployUpgradable(deployments, "SortitionModuleNeo", { from: deployer, args: [ deployer, klerosCoreAddress, minStakingTime, maxFreezingTime, rngWithFallback.target, - maxStakePerJuror, - maxTotalStaked, + rngLookahead, // ← insert this + maxStakePerJuror, + maxTotalStaked, ], });
♻️ Duplicate comments (2)
contracts/CHANGELOG.md (2)
21-21: Fix constant name (typo) and phrasingThe constant is ONE_BASIS_POINT (singular). “Consolidate … with” is imprecise; “Replace” is clearer.
Apply:
- - Consolidate the constant `ALPHA_DIVISOR` with `ONE_BASIS_POINTS` ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) + - Replace `ALPHA_DIVISOR` with `ONE_BASIS_POINT` ([#2090](https://github.com/kleros/kleros-v2/issues/2090))
142-143: Remove unpublished 0.13.0 release link to avoid 404The 0.13.0 link 404s until the tag is published. Hide it until release is live.
Apply:
-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%[email protected] +<!-- [0.13.0]: (add after publishing) -->
🧹 Nitpick comments (48)
contracts/deploy/change-arbitrable-dispute-template.ts (4)
4-4: Remove or makeEventLoga type-only importAfter replacing
instanceof, the runtime import is unnecessary and may cause bundlers to look for a non-existent value export. Remove it (or convert toimport typeif you still annotate types elsewhere).-import { EventLog } from "ethers";
35-36: Nit: renametxtoreceiptand avoid “double-await” confusionThe variable holds a receipt, not a transaction; renaming improves readability.
- let tx = await (await arbitrable.changeDisputeTemplate(template, "disputeTemplateMapping: TODO")).wait(); - tx?.logs?.forEach((event) => { + const receipt = await (await arbitrable.changeDisputeTemplate(template, "disputeTemplateMapping: TODO")).wait(); + receipt?.logs?.forEach((event) => {
35-35: Resolve placeholder mapping before merging
"disputeTemplateMapping: TODO"looks like a placeholder that will be persisted on-chain. Consider sourcing this from config/env or deriving it deterministically (e.g., a versioned identifier or keccak256 of the template).Would you like me to wire this to an env var (with a clear error if missing) or derive it from the template content and network?
41-43: Optional: Declare dependencies/id to make the deploy script idempotent in hardhat-deployTo ensure execution order and avoid accidental re-runs of this side-effectful script, consider adding dependencies and an id.
Example:
deployResolver.tags = ["ArbitrableDisputeTemplate"]; +deployResolver.dependencies = ["ArbitrableExample"]; +deployResolver.id = "change-arbitrable-dispute-template@v1";This helps hardhat-deploy track and order this script relative to the deployment of
ArbitrableExample.contracts/deploy/00-home-chain-resolver.ts (2)
7-9: Remove unused deployments.deploy destructuringdeploy is no longer used after switching to getContractOrDeploy; this may trip noUnusedLocals and fail type-checking.
Apply:
- const { deploy } = deployments;
18-22: Optional: log the resolved address for traceabilityCapturing and logging the DisputeResolver instance helps when auditing CI logs and manual runs.
Apply:
- await getContractOrDeploy(hre, "DisputeResolver", { + const disputeResolver = await getContractOrDeploy(hre, "DisputeResolver", { from: deployer, args: [klerosCore.address, disputeTemplateRegistry.address], log: true, }); + console.log("DisputeResolver at %s", disputeResolver.address);contracts/test/arbitration/staking.ts (3)
2-2: Prefer type-only imports for TypeChain contract types to avoid runtime importsImport the TypeChain types as type-only to prevent unintended runtime imports and avoid confusion with the local PNK constant in value space.
-import { PNK, KlerosCore, SortitionModule, ChainlinkRNG, ChainlinkVRFCoordinatorV2Mock } from "../../typechain-types"; +import type { PNK, KlerosCore, SortitionModule, ChainlinkRNG, ChainlinkVRFCoordinatorV2Mock } from "../../typechain-types";Optional: consider renaming the local PNK value helper (Line 10) to something like toPNK to reduce mental overhead when reading types vs. values.
63-63: Avoid hardcoding VRF requestId = 1; derive it from the emitted eventHardcoding 1 for fulfillRandomWords can become brittle if deployment order or previous requests change. Prefer extracting the requestId from the RNG request event emitted during passPhase.
Example approach:
- Capture the tx receipt from sortition.passPhase() (or wherever the RNG request is made).
- Parse logs for the RNG contract’s “request created” event and read its requestId.
- Pass that requestId to fulfillRandomWords.
This reduces flakiness and future-proofs the test if request sequencing changes.
Also applies to: 396-396
129-130: Fix misleading comments to match assertionsTwo comments contradict the surrounding test expectations.
- expect(await pnk.balanceOf(deployer)).to.be.equal(balanceBefore - PNK(1000)); // No PNK transfer + expect(await pnk.balanceOf(deployer)).to.be.equal(balanceBefore - PNK(1000)); // PNK transferred after delayed stake execution- expect(await pnk.balanceOf(deployer)).to.be.equal(balanceBefore + PNK(1000)); // No PNK transfer yet + expect(await pnk.balanceOf(deployer)).to.be.equal(balanceBefore + PNK(1000)); // PNK withdrawn after delayed stake executionAlso applies to: 191-193
contracts/scripts/disputeRelayerBot.ts (4)
49-54: Heartbeat is fire-and-forget; consider awaiting and handling errorsNot awaiting means network errors are swallowed. Optional, but better observability if you log status or errors.
if (HEARTBEAT_URL) { logger.debug("Sending heartbeat"); - fetch(HEARTBEAT_URL); + try { + const res = await fetch(HEARTBEAT_URL, { method: "GET" }); + logger.debug(`Heartbeat status: ${res.status}`); + } catch (err) { + logger.warn(`Heartbeat failed: ${(err as Error).message}`); + } } else { logger.debug("Heartbeat not set up, skipping"); }
59-110: Wrap event handler body in try/catch to avoid unhandled rejectionsThe callback is async and performs multiple awaits. Any thrown error becomes an unhandled rejection on the event loop. Wrap the body to log context and bail cleanly.
foreignGateway.on( "CrossChainDisputeOutgoing", async (foreignBlockHash, foreignArbitrable, foreignDisputeID, choices, extraData, txReceipt) => { - logger.info( - `CrossChainDisputeOutgoing: ${foreignBlockHash} ${foreignArbitrable} ${foreignDisputeID} ${choices} ${extraData}` - ); - logger.debug(`tx receipt: ${JSON.stringify(txReceipt)}`); + try { + logger.info( + `CrossChainDisputeOutgoing: ${foreignBlockHash} ${foreignArbitrable} ${foreignDisputeID} ${choices} ${extraData}` + ); + logger.debug(`tx receipt: ${JSON.stringify(txReceipt)}`); - // txReceipt is missing the full logs for this tx so we need to request it here - const fullTxReceipt = await foreignChainProvider.getTransactionReceipt(txReceipt.transactionHash); + // txReceipt is missing the full logs for this tx so we need to request it here + const fullTxReceipt = await foreignChainProvider.getTransactionReceipt(txReceipt.transactionHash); - // Retrieve the DisputeRequest event + // Retrieve the DisputeRequest event const disputeRequests: DisputeRequestEventObject[] = fullTxReceipt.logs .filter((log) => log.topics[0] === arbitrableInterface.getEventTopic("DisputeRequest")) .map((log) => arbitrableInterface.parseLog(log).args as unknown as DisputeRequestEventObject); - logger.warn(`More than 1 DisputeRequest event: not supported yet, skipping the others events.`); + if (disputeRequests.length === 0) { + logger.error("No DisputeRequest events found in transaction; skipping."); + return; + } + if (disputeRequests.length > 1) { + logger.warn("More than one DisputeRequest event found in tx; not supported yet — skipping others."); + } - const disputeRequest = disputeRequests[0]; - logger.info(`tx events DisputeRequest: ${JSON.stringify(disputeRequest)}`); + const disputeRequest = disputeRequests[0]; + logger.info(`tx events DisputeRequest: ${JSON.stringify(disputeRequest)}`); - const relayCreateDisputeParams = { + const relayCreateDisputeParams = { foreignBlockHash, foreignChainID, foreignArbitrable, foreignDisputeID, externalDisputeID: disputeRequest._externalDisputeID, templateId: disputeRequest._templateId, templateUri: disputeRequest._templateUri, choices, extraData, - }; - logger.info(`Relaying dispute to home chain... ${JSON.stringify(relayCreateDisputeParams)}`); + }; + logger.info(`Relaying dispute to home chain... ${JSON.stringify(relayCreateDisputeParams)}`); - let tx; - if (feeToken === undefined) { + let tx; + if (feeToken === undefined) { // Paying in native Arbitrum ETH const cost = (await core.functions["arbitrationCost(bytes)"](extraData)).cost; tx = await homeGateway.functions[ "relayCreateDispute((bytes32,uint256,address,uint256,uint256,uint256,string,uint256,bytes))" ](relayCreateDisputeParams, { value: cost }); - } else { + } else { // Paying in ERC20 const cost = (await core.functions["arbitrationCost(bytes,address)"](extraData, feeToken.address)).cost; await (await feeToken.approve(homeGateway.address, cost)).wait(); tx = await homeGateway.functions[ "relayCreateDispute((bytes32,uint256,address,uint256,uint256,uint256,string,uint256,bytes),uint256)" ](relayCreateDisputeParams, cost); - } - tx = tx.wait(); - logger.info(`relayCreateDispute txId: ${tx.transactionHash}`); + } + const receipt = await tx.wait(); + logger.info(`relayCreateDispute txId: ${receipt.transactionHash}`); + } catch (err) { + logger.error({ err }, "Failed to relay dispute"); + } } );
56-61: Deduplicate processing to prevent double-relay on restartsYou already note callbacks can run more than once. Consider deduping per unique key (e.g.,
${foreignBlockHash}-${foreignDisputeID}) in-memory and/or via a lightweight store to avoid double spending fees if the home chain call doesn’t revert.
- Minimal: in-memory Set for current process lifetime.
- Robust: persistent KV (Redis/S3) keyed by tx hash or event log index.
I can provide a small utility for this if helpful.
112-114: Type the delay helperMinor typing improvement for clarity and IntelliSense.
- const delay = (ms) => new Promise((x) => setTimeout(x, ms)); + const delay = (ms: number) => new Promise<void>((resolve) => setTimeout(resolve, ms));contracts/test/integration/index.ts (1)
206-208: Make helper non-async (nit)logJurorBalance is declared async but contains no awaits. Removing async avoids returning a Promise unnecessarily.
Apply:
-const logJurorBalance = async (result: { totalStaked: bigint; totalLocked: bigint }) => { +const logJurorBalance = (result: { totalStaked: bigint; totalLocked: bigint }) => { console.log("staked=%s, locked=%s", ethers.formatUnits(result.totalStaked), ethers.formatUnits(result.totalLocked)); };contracts/test/proxy/index.ts (1)
7-8: Use type-only imports for TypeChain types to avoid runtime module resolution and reduce overheadThese symbols are only used as generic type arguments; importing them as types ensures TS erases the import at runtime and avoids potential module resolution issues with deep, file-qualified TypeChain paths.
Apply this diff:
-import { UpgradedByRewrite as UpgradedByRewriteV1 } from "../../typechain-types/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol"; +import type { UpgradedByRewrite as UpgradedByRewriteV1 } from "../../typechain-types/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol"; -import { UpgradedByRewrite as UpgradedByRewriteV2 } from "../../typechain-types/src/proxy/mock/by-rewrite/UpgradedByRewriteV2.sol"; +import type { UpgradedByRewrite as UpgradedByRewriteV2 } from "../../typechain-types/src/proxy/mock/by-rewrite/UpgradedByRewriteV2.sol";Additionally, consider converting the root TypeChain imports on Line 6 to type-only as well (outside the selected range):
import type { UpgradedByInheritanceV1, UpgradedByInheritanceV2 } from "../../typechain-types";contracts/src/test/RNGMock.sol (2)
2-2: Unify pragma to repo-wide constraint for consistency.Other new files in this PR use >=0.8.0 <0.9.0. Consider aligning to avoid drift and ease compiler config.
Apply this diff:
-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;
16-18: Match interface mutability for clarity.IRNG.receiveRandomness is intentionally non-view (implementations may emit). While overriding with view compiles, matching the interface signature avoids confusion.
Use:
- function receiveRandomness() external view override returns (uint256) { + function receiveRandomness() external override returns (uint256) { return randomNumber; }contracts/src/rng/RNGWithFallback.sol (3)
2-2: Consider aligning pragma with repo-wide range.Most files in this PR use >=0.8.0 <0.9.0. Aligning avoids mixed version constraints.
-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;
73-79: Optional guardrails on governance setters.Changing critical roles/timeouts to zero can brick usage or make fallback immediate. Add zero-address/zero-timeout checks and emit events for governor/consumer changes for traceability.
Example:
- function changeGovernor(address _newGovernor) external onlyByGovernor { - governor = _newGovernor; - } + event GovernorChanged(address indexed previousGovernor, address indexed newGovernor); + function changeGovernor(address _newGovernor) external onlyByGovernor { + require(_newGovernor != address(0), "Governor=0"); + emit GovernorChanged(governor, _newGovernor); + governor = _newGovernor; + } - function changeConsumer(address _consumer) external onlyByGovernor { - consumer = _consumer; - } + event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer); + function changeConsumer(address _consumer) external onlyByGovernor { + require(_consumer != address(0), "Consumer=0"); + emit ConsumerChanged(consumer, _consumer); + consumer = _consumer; + } - function changeFallbackTimeout(uint256 _fallbackTimeoutSeconds) external onlyByGovernor { + function changeFallbackTimeout(uint256 _fallbackTimeoutSeconds) external onlyByGovernor { + require(_fallbackTimeoutSeconds > 0, "Timeout=0"); fallbackTimeoutSeconds = _fallbackTimeoutSeconds; emit FallbackTimeoutChanged(_fallbackTimeoutSeconds); }
93-101: Edge-case: consider checking a request exists before falling back.If receiveRandomness is called without a prior request, this would still fall back after timeout=0. Not critical given SortitionModule flow, but adding
requestTimestamp != 0makes the intent explicit.- if (randomNumber == 0 && block.timestamp > requestTimestamp + fallbackTimeoutSeconds) { + if (randomNumber == 0 && requestTimestamp != 0 && block.timestamp > requestTimestamp + fallbackTimeoutSeconds) { randomNumber = uint256(blockhash(block.number - 1)); emit RNGFallback(); }contracts/deploy/00-home-chain-arbitration-university.ts (1)
74-82: Typed core instance and guarded currency-rate updates — LGTM.The try/catch is pragmatic for non-critical deploy steps. Consider logging token addresses alongside errors for easier troubleshooting.
- } catch (e) { - console.error("Failed to change currency rates for token, with error:", e); + } catch (e) { + console.error( + "Failed to change currency rates", + { pnk: await pnk.getAddress(), dai: await dai.getAddress(), weth: await weth.getAddress() }, + "error:", + e + ); }contracts/deploy/change-sortition-module-rng.ts (2)
26-27: Make changeConsumer idempotent and wait for confirmationsGuard the call to avoid an unnecessary transaction if the consumer is already set, and explicitly wait for mining to avoid timing issues with subsequent ops.
Apply this diff:
- console.log(`chainlinkRng.changeConsumer(${sortitionModule.target})`); - await chainlinkRng.changeConsumer(sortitionModule.target); + const currentConsumer = await chainlinkRng.consumer(); + if (currentConsumer !== sortitionModule.target) { + console.log(`chainlinkRng.changeConsumer(${sortitionModule.target})`); + const tx1 = await chainlinkRng.changeConsumer(sortitionModule.target); + await tx1.wait(); + } else { + console.log("chainlinkRng.consumer already set; skipping"); + }
29-30: Wait for tx confirmation when switching RNG on the SortitionModuleExplicitly awaiting the receipt reduces flakiness in CI and when composing deployments.
- 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/src/arbitration/SortitionModuleNeo.sol (3)
77-102: Consider updating totalStaked only after validating outcomeYou update
totalStakedbefore delegating tosuper._validateStake. If the base returns a non-successStakingResult,totalStakedwould be mutated without the stake actually changing, causing drift.
- Move the
totalStakedincrement/decrement to after thesuper._validateStake(...)call.- Update it only if the returned
stakingResultindicates the change will be applied (e.g., Success/OK).Please confirm the exact success enum value in
StakingResultand whethersuper._validateStakecan return a non-successful result without reverting.
35-36: Clarify the docstring for maxStakePerJurorThe code checks
juror.stakedPnk + stakeChange, which enforces a per-juror total cap across courts. The doc says “in a court”, which is misleading.- /// @param _maxStakePerJuror The maximum amount of PNK a juror can stake in a court. - /// @param _maxTotalStaked The maximum amount of PNK that can be staked in all courts. + /// @param _maxStakePerJuror The maximum total amount of PNK a juror can stake across all courts. + /// @param _maxTotalStaked The maximum total amount of PNK that can be staked across all jurors and courts.
3-3: Pragma version is inconsistent with the PR-wide version updateThe PR states a pragma update (e.g., >=0.8.0 <0.9.0 or 0.8.30), but this file still uses
^0.8.24.If the project-wide target is a range, align it:
-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;Or pin to the agreed version if that’s the standard.
contracts/deploy/00-rng-randomizer.ts (4)
5-5: Import RandomizerRNG for proper typingYou use
rng.consumer()/rng.changeConsumer(...)later; a typed instance avoids TS “unknown property” issues and improves DX.-import { RNGWithFallback } from "../typechain-types"; +import { RNGWithFallback, RandomizerRNG } from "../typechain-types";
23-31: Return a typed contract instance for rng
getContractOrDeployreturns a genericContract; follow it by grabbing a typed instance for calls.- const rng = await getContractOrDeploy(hre, "RandomizerRNG", { + await getContractOrDeploy(hre, "RandomizerRNG", { from: deployer, args: [ deployer, deployer, // The consumer is configured as the RNGWithFallback later randomizerOracle.target, ], log: true, }); + // Use a typed instance for subsequent calls + const rng = await ethers.getContract<RandomizerRNG>("RandomizerRNG");
33-44: Parameterize fallback timeout via env var with a sane defaultThis makes the deployment configurable without code changes.
- const fallbackTimeoutSeconds = 30 * 60; // 30 minutes + const fallbackTimeoutSeconds = + process.env.RNG_FALLBACK_TIMEOUT_SECONDS + ? Number(process.env.RNG_FALLBACK_TIMEOUT_SECONDS) + : 30 * 60; // Default: 30 minutes
45-51: Wait for tx confirmation when switching the consumerYou already guard the call; add
wait()to ensure it’s mined before the script exits.const rngWithFallback = await ethers.getContract<RNGWithFallback>("RNGWithFallback"); const rngConsumer = await rng.consumer(); if (rngConsumer !== rngWithFallback.target) { console.log(`rng.changeConsumer(${rngWithFallback.target})`); - await rng.changeConsumer(rngWithFallback.target); + const tx = await rng.changeConsumer(rngWithFallback.target); + await tx.wait(); }contracts/src/rng/IncrementalNG.sol (2)
21-27: Docstring and behavior mismatch; also minor styleComment says “always the same” but the function increments. Update the doc for clarity.
- /// @dev Get the "random number" (which is always the same). + /// @dev Get the current pseudo-random number and increment it deterministically.
3-3: Pragma version is inconsistent with the repo-wide targetThis file still uses
^0.8.24. Align to the PR’s chosen pragma.-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;contracts/src/arbitration/SortitionModule.sol (1)
3-3: Pragma version alignmentKeep the pragma consistent with the PR-wide version update.
-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;contracts/deploy/00-rng-chainlink.ts (2)
60-64: Use HTTPS and include actionable context in the VRF registration hintSmall nit: the VRF site uses HTTPS. Also, including chainId and subId in the log saves time when registering.
Apply:
- if (!oldRng) { - console.log("Register this Chainlink consumer here: http://vrf.chain.link/"); - } + if (!oldRng) { + console.log( + `Reminder: register the subscription/consumer at https://vrf.chain.link/ (chainId=${chainId}, subId=${subscriptionId})` + ); + }
79-89: Make fallback timeout configurable via env for different networks30 min may be conservative or aggressive depending on network conditions. Suggest supporting an env override with a sanity check.
Apply:
- const fallbackTimeoutSeconds = 30 * 60; // 30 minutes + const fallbackTimeoutSecondsRaw = process.env.FALLBACK_TIMEOUT_SECONDS; + const fallbackTimeoutSeconds = + fallbackTimeoutSecondsRaw !== undefined ? Number(fallbackTimeoutSecondsRaw) : 30 * 60; // default: 30 minutes + if (!Number.isFinite(fallbackTimeoutSeconds) || fallbackTimeoutSeconds <= 0) { + throw new Error(`Invalid FALLBACK_TIMEOUT_SECONDS=${fallbackTimeoutSecondsRaw}`); + }contracts/CHANGELOG.md (1)
19-19: Clarify pragma widening scope and exact range“for the interfaces only” is misleading given the broader changes described in the PR summary. Also, stating the exact range improves traceability.
Apply:
- - Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) + - Widen the allowed pragma to `>=0.8.0 <0.9.0` across interfaces (and other updated modules) ([#2083](https://github.com/kleros/kleros-v2/issues/2083))contracts/src/rng/ChainlinkRNG.sol (1)
94-98: Guard against setting consumer to the zero address and emit an eventSetting consumer to address(0) would brick requestRandomness(). Add a zero-address check and emit an event for observability.
Apply:
- function changeConsumer(address _consumer) external onlyByGovernor { - consumer = _consumer; - } + function changeConsumer(address _consumer) external onlyByGovernor { + if (_consumer == address(0)) revert InvalidConsumer(); + address previous = consumer; + consumer = _consumer; + emit ConsumerChanged(previous, _consumer); + }And add these declarations (outside the selected range):
// Place near other errors or at the top of the contract error InvalidConsumer(); // Place near other events event ConsumerChanged(address indexed _previousConsumer, address indexed _newConsumer);contracts/src/rng/RandomizerRNG.sol (4)
3-3: Align pragma with project-wide compiler constraintPR summary states moving to >=0.8.0 <0.9.0. This file still uses ^0.8.24. Align to the project-wide constraint to avoid compiler skew across packages.
-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;
53-62: Constructor: validate inputs (non-zero addresses) to prevent bricking the RNGGuard against accidentally deploying with zero addresses for governor/consumer/randomizer. Cheap now, expensive later.
constructor(address _governor, address _consumer, IRandomizer _randomizer) { - governor = _governor; - consumer = _consumer; - randomizer = _randomizer; + if (_governor == address(0) || _consumer == address(0) || address(_randomizer) == address(0)) { + revert InvalidAddress(); + } + governor = _governor; + consumer = _consumer; + randomizer = _randomizer; callbackGasLimit = 50000; }Add this error to the bottom error section:
error InvalidAddress();
74-78: Emit event and validate new consumer on changeConsumer()
- Emit an event so off-chain infra and dashboards can track consumer rotations.
- Guard against setting zero consumer.
- function changeConsumer(address _consumer) external onlyByGovernor { - consumer = _consumer; - } + function changeConsumer(address _consumer) external onlyByGovernor { + if (_consumer == address(0)) revert InvalidAddress(); + address old = consumer; + consumer = _consumer; + emit ConsumerChanged(old, _consumer); + }Add the event (near the other events):
/// @param randomWords The random values answering the request. event RequestFulfilled(uint256 indexed requestId, uint256 randomWords); +/// Emitted when the consumer is changed by the governor. +event ConsumerChanged(address indexed oldConsumer, address indexed newConsumer);
26-34: Nit: event docs are VRF-flavored; adjust for Randomizer.ai and single wordThe comments mention “VRF Coordinator” and “random values” but this contract uses Randomizer.ai and a single 256-bit value. Consider tightening the docstrings for accuracy. No code changes required if you prefer to leave as-is.
contracts/src/rng/BlockhashRNG.sol (2)
3-3: Align pragma with project-wide compiler constraintAlign to >=0.8.0 <0.9.0 to match the PR-wide pragma update.
-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;
46-50: Validate lookaheadTime to avoid degenerate configurationA zero lookaheadTime removes the intended “request then wait” property and can allow immediate harvesting of current/previous block hash. Add a guard.
constructor(address _governor, address _consumer, uint256 _lookaheadTime) { - governor = _governor; - consumer = _consumer; - lookaheadTime = _lookaheadTime; + if (_governor == address(0) || _consumer == address(0)) revert InvalidAddress(); + if (_lookaheadTime == 0) revert InvalidLookaheadTime(); + governor = _governor; + consumer = _consumer; + lookaheadTime = _lookaheadTime; }Add errors at the bottom:
error InvalidAddress(); error InvalidLookaheadTime();contracts/src/arbitration/SortitionModuleBase.sol (2)
483-487: Nit: prefer uint256 over uint for consistencyMinor style consistency. Not critical.
- uint treeIndex = tree.IDsToNodeIndexes[_ID]; + uint256 treeIndex = tree.IDsToNodeIndexes[_ID];
154-161: Action: document the RNG-consumer precondition or add a safe-guard in changeRandomNumberGeneratorShort summary — I verified the repo: deployment scripts and tests set the RNG consumer before swapping RNGs, but the SortitionModule still calls rng.requestRandomness(...) while in Phase.generating, so an operator mistake (omitting setting the RNG consumer) can revert and brick generating. Recommend documenting the operational requirement and (optionally) adding a sanity check / try-catch to surface a clearer error.
Evidence (selected locations):
- Deploy scripts that set consumer before swap:
- contracts/deploy/change-sortition-module-rng.ts — calls chainlinkRng.changeConsumer(sortitionModule.target) then sortitionModule.changeRandomNumberGenerator(...)
- contracts/deploy/00-home-chain-arbitration.ts — checks rngWithFallback.consumer() and calls rngWithFallback.changeConsumer(sortitionModule.address) if needed
- contracts/deploy/00-rng-randomizer.ts and contracts/deploy/00-rng-chainlink.ts — similar consumer checks/calls
- Tests that perform consumer setup before swapping or run as governor:
- contracts/test/arbitration/dispute-kit-gated.ts — calls sortitionModule.changeRandomNumberGenerator(rng.target)
- contracts/test/arbitration/draw.ts — same
- contracts/test/foundry/KlerosCore.t.sol — vm.prank(governor); sortitionModule.changeRandomNumberGenerator(...)
- RNG contracts expose changeConsumer / consumer:
- contracts/src/rng/ChainlinkRNG.sol, RandomizerRNG.sol, RNGWithFallback.sol, BlockhashRNG.sol (have changeConsumer / consumer functions)
Recommendation (concise)
- Minimum: document in the SortitionModule API / deploy README that "When changing RNG while phase == generating the governor must set the new RNG's consumer to the SortitionModule before invoking changeRandomNumberGenerator — otherwise rng.requestRandomness() may revert."
- Optional (recommended for safer UX): wrap the requestRandomness call in a try/catch and revert with a clear error (or emit an event) to avoid opaque reverts. Example minimal patch:
- if (phase == Phase.generating) { - rng.requestRandomness(block.number + rngLookahead); - randomNumberRequestBlock = block.number; - } + if (phase == Phase.generating) { + // Prefer a clear revert instead of an opaque external revert (e.g. ConsumerOnly) + try rng.requestRandomness(block.number + rngLookahead) { + randomNumberRequestBlock = block.number; + } catch { + revert RNGRequestFailed(); // add: error RNGRequestFailed(); + } + }Tag:
contracts/test/rng/index.ts (3)
39-46: Avoid variable shadowing for readabilityYou already have a top-level deployer variable (string) and re-declare a local
deployer. Rename the local to avoid confusion.- const [deployer] = await ethers.getSigners(); + const [blockhashDeployer] = await ethers.getSigners(); ... - from: deployer.address, - args: [deployer.address, deployer.address, 10], // governor, consumer, lookaheadTime (seconds) + from: blockhashDeployer.address, + args: [blockhashDeployer.address, blockhashDeployer.address, 10], // governor, consumer, lookaheadTime (seconds)
48-65: Add negative-path tests for BlockHashRNG access controlSince receiveRandomness() and requestRandomness() are ConsumerOnly, add tests that a non-consumer gets reverted. This guards regressions in modifiers and helps document the intended access model.
Example:
it("Should revert for non-consumer", async () => { const [, other] = await ethers.getSigners(); await expect(rng.connect(other).requestRandomness()).to.be.revertedWithCustomError(rng, "ConsumerOnly"); await expect(rng.connect(other).receiveRandomness.staticCall()).to.be.revertedWithCustomError(rng, "ConsumerOnly"); });Also applies to: 72-81
98-99: Add negative-path tests for changeConsumer governor gatingTests currently set consumer with the correct governor. Add a non-governor attempt to ensure GovernorOnly is enforced on both ChainlinkRNG and RandomizerRNG.
Example:
it("Should revert non-governor changeConsumer", async () => { const [, other] = await ethers.getSigners(); await expect(rng.connect(other).changeConsumer(other.address)).to.be.revertedWithCustomError(rng, "GovernorOnly"); });Also applies to: 161-162
| @@ -1,7 +1,7 @@ | |||
| import { HardhatRuntimeEnvironment } from "hardhat/types"; | |||
| import { DeployFunction } from "hardhat-deploy/types"; | |||
| import { HomeChains, isSkipped } from "./utils"; | |||
| import { deployUpgradable } from "./utils/deployUpgradable"; | |||
| import { getContractOrDeploy } from "./utils/getContractOrDeploy"; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
getContractOrDeploy can skip required redeploys; this risks stale deployments
The helper returns early if a contract with the same name exists, bypassing hardhat-deploy’s bytecode/args/libraries diffing logic. If the artifact or constructor args change, this will not redeploy and you’ll silently keep the old instance. The prior pattern (always calling deployments.deploy) is idempotent and safer.
Recommended fix: update the helper to always call deployments.deploy and rely on newlyDeployed to detect reuse.
Proposed helper update (contracts/deploy/utils/getContractOrDeploy.ts):
import type { HardhatRuntimeEnvironment } from "hardhat/types";
import type { DeployOptions } from "hardhat-deploy/types";
import type { Contract } from "ethers";
export const getContractOrDeploy = async (
hre: HardhatRuntimeEnvironment,
contractName: string,
options: DeployOptions
): Promise<Contract> => {
const result = await hre.deployments.deploy(contractName, options);
if (result.newlyDeployed) {
console.log(`contract ${contractName} deployed at ${result.address}`);
} else {
console.log(`contract ${contractName} already deployed at ${result.address}`);
}
return hre.ethers.getContract<Contract>(contractName);
};Run this quick scan to see all call sites using the helper (so we don’t miss any):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "getContractOrDeploy call sites:"
rg -nP --type=ts -C2 '\bgetContractOrDeploy\s*\(' contracts
echo
echo "Current helper implementation (first 120 lines):"
sed -n '1,120p' contracts/deploy/utils/getContractOrDeploy.tsLength of output: 7802
Enforce safe, idempotent deployments in getContractOrDeploy
The current helper in contracts/deploy/utils/getContractOrDeploy.ts only calls hre.deployments.deploy when no existing instance is found via getContractOrNull. This bypasses Hardhat-Deploy’s bytecode/args/libraries diff check, so changes to the artifact or constructor parameters won’t trigger a redeploy and you’ll continue using an outdated contract.
Required update:
- Always invoke hre.deployments.deploy(contractName, options) and rely on the returned newlyDeployed flag to detect reuse.
- Preserve logging of whether the contract was newly deployed or already in place.
- Ensure all existing call sites (e.g., in 00-home-chain-resolver.ts and the other scripts listed by the scan) continue to work without modification.
Example refactored helper (contracts/deploy/utils/getContractOrDeploy.ts):
import type { HardhatRuntimeEnvironment } from "hardhat/types";
import type { DeployOptions } from "hardhat-deploy/types";
import { Contract } from "ethers";
export const getContractOrDeploy = async (
hre: HardhatRuntimeEnvironment,
contractName: string,
options: DeployOptions
): Promise<Contract> => {
const result = await hre.deployments.deploy(contractName, options);
if (result.newlyDeployed) {
console.log(`contract ${contractName} deployed at ${result.address}`);
} else {
console.log(`contract ${contractName} already deployed at ${result.address}`);
}
return hre.ethers.getContract<Contract>(contractName);
};Please update the helper implementation accordingly to avoid stale deployments.
🤖 Prompt for AI Agents
In contracts/deploy/00-home-chain-resolver.ts around line 4, the project uses
getContractOrDeploy which currently skips calling hre.deployments.deploy when a
contract exists, bypassing Hardhat-Deploy’s diff checks; update
contracts/deploy/utils/getContractOrDeploy.ts so it always calls await
hre.deployments.deploy(contractName, options), read the returned newlyDeployed
flag to log whether it was newly deployed or reused, and then return the
contract via hre.ethers.getContract(contractName) so all existing call sites
(including 00-home-chain-resolver.ts) continue to work unchanged.
| args: [ | ||
| deployer, | ||
| deployer, // The consumer is configured as the SortitionModule later | ||
| fallbackTimeoutSeconds, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm downstream wiring sets RNGWithFallback’s consumer to the SortitionModule
Comment states this will be configured later. Please confirm the corresponding script/tag runs after this one to complete the wiring.
You can verify with:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP --type=ts -C2 '\bgetContract<SortitionModule(Neo)?>\('
rg -nP --type=ts -C2 '\bRNGWithFallback\b' | rg -nP -C2 'changeConsumer\s*\('Length of output: 5110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sortition module RNG change script for consumer wiring
sed -n '1,200p' contracts/deploy/change-sortition-module-rng.tsLength of output: 1769
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show 00-rng-randomizer.ts to verify consumer wiring
sed -n '1,200p' contracts/deploy/00-rng-randomizer.tsLength of output: 2201
Wire RNGWithFallback’s consumer to the SortitionModule
We didn’t find any calls to rngWithFallback.changeConsumer(...) in the deploy scripts, so the fallback RNG never hands off to the SortitionModule. You need to add a step—either in 00-rng-randomizer.ts (or in a new deploy script that runs immediately afterward)—to point the fallback at the sortition module:
• File: contracts/deploy/00-rng-randomizer.ts
After deploying RNGWithFallback, fetch the SortitionModule and invoke changeConsumer:
// …existing RNGWithFallback deployment…
const rngWithFallback = await ethers.getContract<RNGWithFallback>("RNGWithFallback");
// Wire fallback RNG into SortitionModule
const sortitionModule = await ethers.getContract<SortitionModule>("SortitionModule");
console.log(`rngWithFallback.changeConsumer(${sortitionModule.target})`);
await rngWithFallback.changeConsumer(sortitionModule.target);Ensure this runs after your other RNG scripts so that SortitionModule is configured to use the fallback.
🤖 Prompt for AI Agents
In contracts/deploy/00-rng-randomizer.ts around line 39, the RNGWithFallback
deployment is missing wiring to the SortitionModule so the fallback never hands
off; after deploying and obtaining the RNGWithFallback instance, fetch the
deployed SortitionModule contract and call RNGWithFallback.changeConsumer(...)
with the SortitionModule address (ensure this step runs after SortitionModule is
deployed and after other RNG deploy scripts so the fallback is configured to
point to the SortitionModule).
| function receiveRandomness() external override onlyByConsumer returns (uint256 randomNumber) { | ||
| if (requestTimestamp == 0) return 0; // No requests were made yet. | ||
|
|
||
| uint256 expectedTimestamp = requestTimestamp + lookaheadTime; | ||
|
|
||
| // Check if enough time has passed | ||
| if (block.timestamp < expectedTimestamp) { | ||
| return 0; // Not ready yet | ||
| } | ||
|
|
||
| // Check if we already have a saved random number for this timestamp window | ||
| randomNumber = randomNumbers[expectedTimestamp]; | ||
| if (randomNumber != 0) { | ||
| return randomNumber; | ||
| } | ||
|
|
||
| if (_block < block.number) { | ||
| // The random number is not already set and can be. | ||
| if (blockhash(_block) != 0x0) { | ||
| // Normal case. | ||
| randomNumber = uint256(blockhash(_block)); | ||
| } else { | ||
| // The contract was not called in time. Fallback to returning previous blockhash. | ||
| randomNumber = uint256(blockhash(block.number - 1)); | ||
| } | ||
| // Use last block hash for randomness | ||
| randomNumber = uint256(blockhash(block.number - 1)); | ||
| if (randomNumber != 0) { | ||
| randomNumbers[expectedTimestamp] = randomNumber; | ||
| } | ||
| randomNumbers[_block] = randomNumber; | ||
| return randomNumber; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Timing-manipulation vulnerability: randomness depends on the caller-chosen block after readiness
As written, once ready, receiveRandomness() derives the random number from blockhash(block.number - 1) at the time of the call and caches it by expectedTimestamp. Since receiveRandomness() is callable by the consumer (and, transitively, passPhase in SortitionModule can be triggered by anyone), the first caller after readiness effectively chooses the block used, enabling timing-based bias.
Mitigation: pre-commit the target block at request time and use that block’s hash after the time gate. One pragmatic approach:
- Save requestBlock at request time.
- After readiness, always derive from blockhash(requestBlock + 1) (or +k if you prefer).
- Cache both the number and the block used to make replays deterministic.
- Return 0 if the target block’s hash is no longer available (older than 256 blocks).
- uint256 public requestTimestamp; // Timestamp of the current request
- mapping(uint256 timestamp => uint256 number) public randomNumbers; // randomNumbers[timestamp] is the random number for this timestamp, 0 otherwise.
+ uint256 public requestTimestamp; // Timestamp of the current request
+ uint256 public requestBlock; // Block number at request time (anchor to remove caller timing bias).
+ mapping(uint256 timestamp => uint256 number) public randomNumbers; // randomNumbers[timestamp] is the random number for this timestamp window, 0 otherwise.
+ mapping(uint256 timestamp => uint256 blockNumber) public randomBlocks; // The block number used to derive that randomness.- function requestRandomness() external override onlyByConsumer {
- requestTimestamp = block.timestamp;
- }
+ function requestRandomness() external override onlyByConsumer {
+ requestTimestamp = block.timestamp;
+ requestBlock = block.number;
+ }- // Use last block hash for randomness
- randomNumber = uint256(blockhash(block.number - 1));
- if (randomNumber != 0) {
- randomNumbers[expectedTimestamp] = randomNumber;
- }
- return randomNumber;
+ // Derive from a pre-committed block to avoid timing selection attacks.
+ uint256 targetBlock = requestBlock + 1;
+ randomNumber = uint256(blockhash(targetBlock));
+ if (randomNumber == 0) {
+ // Target block is either not yet mined (should not happen after time gate)
+ // or its hash is no longer available (>256 blocks old). Consider returning 0 to let caller retry or re-request.
+ return 0;
+ }
+ randomNumbers[expectedTimestamp] = randomNumber;
+ randomBlocks[expectedTimestamp] = targetBlock;
+ return randomNumber;| modifier onlyByGovernor() { | ||
| if (governor != msg.sender) revert GovernorOnly(); | ||
| _; | ||
| } | ||
|
|
||
| modifier onlyByConsumer() { | ||
| if (consumer != msg.sender) revert ConsumerOnly(); | ||
| _; | ||
| } |
There was a problem hiding this comment.
Custom errors must be qualified with IRNG to compile.
GovernorOnly and ConsumerOnly are declared inside IRNG; unqualified references here will not resolve.
Apply:
modifier onlyByGovernor() {
- if (governor != msg.sender) revert GovernorOnly();
+ if (governor != msg.sender) revert IRNG.GovernorOnly();
_;
}
modifier onlyByConsumer() {
- if (consumer != msg.sender) revert ConsumerOnly();
+ if (consumer != msg.sender) revert IRNG.ConsumerOnly();
_;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| modifier onlyByGovernor() { | |
| if (governor != msg.sender) revert GovernorOnly(); | |
| _; | |
| } | |
| modifier onlyByConsumer() { | |
| if (consumer != msg.sender) revert ConsumerOnly(); | |
| _; | |
| } | |
| modifier onlyByGovernor() { | |
| if (governor != msg.sender) revert IRNG.GovernorOnly(); | |
| _; | |
| } | |
| modifier onlyByConsumer() { | |
| if (consumer != msg.sender) revert IRNG.ConsumerOnly(); | |
| _; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/rng/RNGWithFallback.sol around lines 47 to 55, the modifiers
revert using unqualified custom errors GovernorOnly and ConsumerOnly which are
declared in the IRNG interface; update the revert calls to use the
fully-qualified names (revert IRNG.GovernorOnly(); and revert
IRNG.ConsumerOnly();) so the compiler can resolve them, keeping the rest of the
modifier logic unchanged.
…ounts-to-profile feat: add available stake, effectivestake
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
web/src/components/NumberDisplay.tsx (1)
38-41: Guard against undefined units in tooltip and avoid stray spaces.If
unitis omitted, the tooltip currently renders "undefined" (e.g., "undefined 1,000"). Also, you may end up with leading/trailing spaces. Coalesce the unit and trim.Apply this diff:
- const tooltipValue = isCurrency ? `${unit} ${commify(value)}` : `${commify(value)} ${unit}`; + const safeUnit = unit ?? ""; + const tooltipValue = isCurrency + ? `${safeUnit} ${commify(value)}`.trim() + : `${commify(value)} ${safeUnit}`.trim();web/src/pages/Profile/Stakes/Header.tsx (2)
111-114: Avoid unnecessary.toString(); values are already strings in these guarded branches.
formatUnits(...)yieldsstring. Inside the!isUndefined(...)branches, the formatted values are guaranteed to be truthy strings. Dropping.toString()reduces noise and avoids accidental stringifying if future refactors change types.Apply this diff:
- <NumberDisplay value={formattedAvailableStake.toString()} unit="PNK" /> + <NumberDisplay value={formattedAvailableStake} unit="PNK" /> ... - <NumberDisplay value={formattedEffectiveStake.toString()} unit="PNK" /> + <NumberDisplay value={formattedEffectiveStake} unit="PNK" /> ... - <NumberDisplay value={formattedLockedStake.toString()} unit="PNK" /> + <NumberDisplay value={formattedLockedStake} unit="PNK" />Also applies to: 120-123, 130-133
109-113: Usespaninstead oflabelfor non-form text.
labelis intended to be associated with form controls viahtmlFor. Usingspanis semantically correct and avoids accessibility confusion.Apply this diff:
- <label> Available: </label> + <span> Available: </span> ... - <label> Staked: </label> + <span> Staked: </span> ... - <label> Locked: </label> + <span> Locked: </span>Also applies to: 118-122, 127-131
web/src/pages/Profile/Stakes/index.tsx (1)
49-51: Minor: streamline truthiness checks.
const isStaked = Boolean(stakedCourts?.length);is a bit cleaner and avoids double checks.Apply this diff:
- const stakedCourts = stakeData?.jurorTokensPerCourts?.filter(({ staked }) => staked > 0); - const isStaked = stakedCourts && stakedCourts.length > 0; + const stakedCourts = stakeData?.jurorTokensPerCourts?.filter(({ staked }) => staked > 0); + const isStaked = Boolean(stakedCourts?.length);
📜 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)
web/src/components/NumberDisplay.tsx(1 hunks)web/src/pages/Profile/Courts/Header.tsx(0 hunks)web/src/pages/Profile/Stakes/Header.tsx(1 hunks)web/src/pages/Profile/Stakes/index.tsx(3 hunks)web/src/pages/Profile/index.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- web/src/pages/Profile/Courts/Header.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-06-27T10:11:54.861Z
Learnt from: nikhilverma360
PR: kleros/kleros-v2#1632
File: web/src/components/DisputeView/DisputeInfo/DisputeInfoList.tsx:37-42
Timestamp: 2024-06-27T10:11:54.861Z
Learning: `useMemo` is used in `DisputeInfoList` to optimize the rendering of `FieldItems` based on changes in `fieldItems`, ensuring that the mapping and truncation operation are only performed when necessary.
Applied to files:
web/src/pages/Profile/index.tsx
⏰ 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: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (4)
web/src/pages/Profile/index.tsx (3)
10-12: LGTM: query imports.The new GraphQL hooks/imports look consistent with the rest of the file.
15-17: LGTM: style imports reorder.Moving the style imports below GraphQL imports is fine and consistent.
24-25: LGTM: Stakes replaces Courts.Importing
Stakeshere aligns with the new Profile page wiring.web/src/pages/Profile/Stakes/index.tsx (1)
59-60: LGTM: header wiring with available/locked/effective stake.Passing the three stake aggregates up to the header matches the new UI requirements.
| @@ -110,7 +113,7 @@ const Profile: React.FC = () => { | |||
| {isConnected || searchParamAddress ? ( | |||
| <> | |||
| <JurorInfo {...{ addressToQuery }} /> | |||
| <Courts {...{ addressToQuery }} /> | |||
| <Stakes {...{ addressToQuery }} /> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type mismatch: Stakes expects a 0x-prefixed address; addressToQuery is string.
IStakes.addressToQuery is typed as 0x${string} (see Stakes/index.tsx Line 38), but addressToQuery here is inferred as string via URL param or toLowerCase(). This will fail TS type-checking.
Two viable fixes (pick one):
- Option A (loosen prop type): Change
IStakes.addressToQuerytostring. See suggested change in Stakes/index.tsx (Lines 38-40). - Option B (validate and narrow type here): Ensure the string looks like an EVM address, then cast.
Apply this diff for Option B:
- <Stakes {...{ addressToQuery }} />
+ {addressToQuery?.startsWith("0x") && addressToQuery.length === 42 ? (
+ <Stakes addressToQuery={addressToQuery as `0x${string}`} />
+ ) : null}If you prefer Option A, apply the interface tweak in Stakes/index.tsx as proposed in that file’s comment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Stakes {...{ addressToQuery }} /> | |
| {addressToQuery?.startsWith("0x") && addressToQuery.length === 42 ? ( | |
| <Stakes addressToQuery={addressToQuery as `0x${string}`} /> | |
| ) : null} |
🤖 Prompt for AI Agents
In web/src/pages/Profile/index.tsx around line 116, addressToQuery is currently
a plain string but Stakes.prop expects a 0x-prefixed address; validate and
narrow the type here by checking the value matches an EVM address pattern (e.g.
/^0x[0-9a-fA-F]{40}$/) before passing it to <Stakes />: if the check passes cast
the value to the narrower type and pass it, otherwise handle the invalid case
(skip rendering Stakes or show an error/placeholder). Ensure the runtime guard
is tight so TypeScript accepts the cast where you pass the prop.
| interface IStakes { | ||
| addressToQuery: `0x${string}`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prop type too restrictive for caller; align with actual usage.
addressToQuery is typed as 0x${string}, but the caller (Profile/index.tsx) produces a generic string. Unless you validate upstream, this will cause a TS error. Either validate upstream (see Profile comment) or relax this prop type to string.
Apply this diff to relax the prop:
-interface IStakes {
- addressToQuery: `0x${string}`;
-}
+interface IStakes {
+ addressToQuery: string;
+}If you want to keep the stronger type, keep this interface as-is and adopt Option B in Profile (runtime validation + cast).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface IStakes { | |
| addressToQuery: `0x${string}`; | |
| } | |
| interface IStakes { | |
| addressToQuery: string; | |
| } |
🤖 Prompt for AI Agents
In web/src/pages/Profile/Stakes/index.tsx around lines 38-40, the IStakes
interface types addressToQuery as a template literal `0x${string}` which is too
restrictive for callers that pass a plain string; update the interface to use
addressToQuery: string so it accepts the upstream value (or alternatively leave
as-is only if you implement upstream runtime validation and cast there),
ensuring the component signature matches actual usage to avoid TS errors.
| const effectiveStake = stakeData?.jurorTokensPerCourts?.[0]?.effectiveStake | ||
| ? BigInt(stakeData.jurorTokensPerCourts[0].effectiveStake) | ||
| : undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Effective stake should aggregate across all staked courts (not just index 0).
Using only jurorTokensPerCourts[0] undercounts when the juror is staked in multiple courts. Sum all effectiveStake values instead.
Apply this diff:
- const effectiveStake = stakeData?.jurorTokensPerCourts?.[0]?.effectiveStake
- ? BigInt(stakeData.jurorTokensPerCourts[0].effectiveStake)
- : undefined;
+ const effectiveStake =
+ stakeData?.jurorTokensPerCourts && stakeData.jurorTokensPerCourts.length > 0
+ ? stakeData.jurorTokensPerCourts.reduce<bigint>(
+ (acc, { effectiveStake }) => acc + BigInt(effectiveStake ?? 0),
+ 0n
+ )
+ : undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const effectiveStake = stakeData?.jurorTokensPerCourts?.[0]?.effectiveStake | |
| ? BigInt(stakeData.jurorTokensPerCourts[0].effectiveStake) | |
| : undefined; | |
| const effectiveStake = | |
| stakeData?.jurorTokensPerCourts && stakeData.jurorTokensPerCourts.length > 0 | |
| ? stakeData.jurorTokensPerCourts.reduce<bigint>( | |
| (acc, { effectiveStake }) => acc + BigInt(effectiveStake ?? 0), | |
| 0n | |
| ) | |
| : undefined; |
🤖 Prompt for AI Agents
In web/src/pages/Profile/Stakes/index.tsx around lines 53 to 55, the code only
reads jurorTokensPerCourts[0].effectiveStake which undercounts when a juror is
staked in multiple courts; replace this with an aggregation over
stakeData.jurorTokensPerCourts: iterate or reduce the array, skip entries with
missing effectiveStake, convert each effectiveStake to BigInt and sum into a
single BigInt total, and set effectiveStake to undefined if the array is empty
or no valid stakes found.
… OZ reverting on it
feat(web): validation of the token address for ERC20/721/1155 types
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/utils/extradataToTokenInfo.ts (1)
24-29: Replace Buffer usage; it breaks in browsers and bundlers without Node polyfillsUsing Buffer in the web bundle can fail at runtime. viem provides bytesToHex for this purpose.
Apply:
-import { Address, hexToBytes } from "viem"; +import { Address, hexToBytes, bytesToHex } from "viem"; @@ - const packed = BigInt("0x" + Buffer.from(packedBytes).toString("hex")); + const packed = BigInt(bytesToHex(packedBytes)); @@ - const tokenId = BigInt("0x" + Buffer.from(tokenIdBytes).toString("hex")); + const tokenId = BigInt(bytesToHex(tokenIdBytes));
🧹 Nitpick comments (5)
web/src/hooks/useTokenAddressValidation.ts (1)
36-56: Optional: Use ERC-165 for ERC-721/1155 to reduce false positivesSome non-token contracts expose a balanceOf-like method. Consider ERC-165 feature detection for ERC-721 (0x80ac58cd) and ERC-1155 (0xd9b67a26) before or instead of balanceOf to avoid misclassification. Keep balanceOf for ERC-20.
Example approach (sketch):
// pre-check for ERC-721 / ERC-1155 publicClient.readContract({ address, abi: ERC165_ABI, functionName: 'supportsInterface', args: [is1155 ? '0xd9b67a26' : '0x80ac58cd'] })Also applies to: 74-87, 94-104
web/src/utils/extradataToTokenInfo.ts (1)
18-20: Be explicit about undefined return for readabilityReturning undefined implicitly is fine, but being explicit helps future readers and avoids lint noise.
- if (extraDataBytes.length < 160) { - return; - } + if (extraDataBytes.length < 160) { + return undefined; + }web/src/pages/Resolver/NavigationButtons/NextButton.tsx (1)
20-29: Gated token validity logic is correct; minor comment driftLogic correctly bypasses non-gated kits, requires a non-empty address, and enforces validation success. The inline comment on Line 27 mentions ERC-20 specifically; it now covers ERC-20/ERC-721 or ERC-1155 depending on type.
- // If token address is provided, it must be validated as valid ERC20 + // If token address is provided, it must be validated as a valid token (ERC-20/ERC-721 or ERC-1155)web/src/pages/Resolver/Parameters/Court.tsx (2)
160-168: Optional: show neutral border during validationWhile validating, consider neutral border (e.g., theme.stroke) to avoid flashing red/green before the result settles.
> input { - border-color: ${({ $isValid, theme }) => { - if ($isValid === true) return theme.success; - if ($isValid === false) return theme.error; - return "inherit"; - }}; + border-color: ${({ $isValid, theme }) => + $isValid === true ? theme.success : $isValid === false ? theme.error : theme.stroke}; }
224-235: EnhanceuseLocalStorageto support functional updatesThe setter returned by your current
useLocalStoragehook only accepts a new value of type T, so it will reject—rather than apply—a functional updater. To safely switch your effect inweb/src/pages/Resolver/Parameters/Court.tsxto a functional setter (and avoid stale‐closure races), update the hook first:• In
web/src/hooks/useLocalStorage.ts:- export function useLocalStorage<T>(keyName: string, defaultValue: T) { - const [storedValue, setStoredValue] = useState<T>(() => { /*…*/ }); - - const setValue = (newValue: T) => { - try { - window.localStorage.setItem(keyName, JSON.stringify(newValue)); - } finally { - setStoredValue(newValue); - } - }; - - return [storedValue, setValue]; - } + type SetLocalStorage<T> = (value: T | ((prev: T) => T)) => void; + + export function useLocalStorage<T>( + keyName: string, + defaultValue: T + ): [T, SetLocalStorage<T>] { + const [storedValue, setStoredValue] = useState<T>(() => { + try { + const item = window.localStorage.getItem(keyName); + return item ? JSON.parse(item) : defaultValue; + } catch { + return defaultValue; + } + }); + + const setValue: SetLocalStorage<T> = (valueOrFn) => + setStoredValue(prev => { + const newValue = + typeof valueOrFn === "function" + ? (valueOrFn as (prev: T) => T)(prev) + : valueOrFn; + try { + window.localStorage.setItem(keyName, JSON.stringify(newValue)); + } catch { /* ignore */ } + return newValue; + }); + + return [storedValue, setValue]; + }• Then in
web/src/pages/Resolver/Parameters/Court.tsx, dropdisputeData.disputeKitDatafrom the deps and refactor your effect to use the functional setter:- useEffect(() => { - if (isGatedDisputeKit && disputeData.disputeKitData) { - const current = disputeData.disputeKitData as IGatedDisputeData; - if (current.isTokenGateValid !== isValidToken) { - setDisputeData({ - ...disputeData, - disputeKitData: { ...current, isTokenGateValid: isValidToken }, - }); - } - } - }, [isValidToken, isGatedDisputeKit, disputeData.disputeKitData, setDisputeData]); + useEffect(() => { + if (!isGatedDisputeKit) return; + setDisputeData(prev => { + const current = prev.disputeKitData as IGatedDisputeData | undefined; + if (!current || current.isTokenGateValid === isValidToken) return prev; + return { + ...prev, + disputeKitData: { ...current, isTokenGateValid: isValidToken }, + }; + }); + }, [isValidToken, isGatedDisputeKit, setDisputeData]);This ensures you never read a stale
disputeDataobject in your effect and still persist to localStorage correctly.
📜 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)
web/src/context/NewDisputeContext.tsx(1 hunks)web/src/hooks/useTokenAddressValidation.ts(1 hunks)web/src/pages/Resolver/NavigationButtons/NextButton.tsx(3 hunks)web/src/pages/Resolver/Parameters/Court.tsx(6 hunks)web/src/utils/extradataToTokenInfo.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-10-14T13:58:25.708Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Applied to files:
web/src/context/NewDisputeContext.tsxweb/src/pages/Resolver/NavigationButtons/NextButton.tsxweb/src/pages/Resolver/Parameters/Court.tsx
📚 Learning: 2025-05-15T06:50:40.859Z
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
Applied to files:
web/src/pages/Resolver/NavigationButtons/NextButton.tsxweb/src/pages/Resolver/Parameters/Court.tsx
📚 Learning: 2024-10-29T10:13:04.524Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1729
File: web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx:69-69
Timestamp: 2024-10-29T10:13:04.524Z
Learning: In `web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx`, when the button is disabled, the associated logic won't be reached, and certain code paths may exist for TypeScript purposes.
Applied to files:
web/src/pages/Resolver/NavigationButtons/NextButton.tsx
🧬 Code graph analysis (2)
web/src/pages/Resolver/NavigationButtons/NextButton.tsx (1)
web/src/context/NewDisputeContext.tsx (1)
IGatedDisputeData(59-65)
web/src/pages/Resolver/Parameters/Court.tsx (4)
web-devtools/src/styles/responsiveSize.ts (1)
responsiveSize(9-12)web-devtools/src/styles/Theme.tsx (1)
theme(3-10)web/src/context/NewDisputeContext.tsx (1)
IGatedDisputeData(59-65)web/src/hooks/useTokenAddressValidation.ts (2)
useERC20ERC721Validation(75-87)useERC1155Validation(95-104)
⏰ 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). (5)
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (4)
web/src/hooks/useTokenAddressValidation.ts (1)
75-87: Hook composition and debounce approach look solidSeparation into ERC-20/ERC-721 vs. ERC-1155 via a generic validator is clean, with sensible retries and caching. The 500ms debounce is a good choice to limit RPC churn.
web/src/context/NewDisputeContext.tsx (1)
64-65: New tri-state isTokenGateValid field is appropriateThe nullable boolean communicates validation progress clearly and aligns with UI gating logic. No further changes needed here.
web/src/pages/Resolver/Parameters/Court.tsx (2)
200-217: Validation flow wiring looks goodConditional enabling, dual-hook strategy (ERC-20/721 vs ERC-1155), and unified status computation are clean and readable.
Also applies to: 218-222
305-306: Ensure DropdownSelect is used as a controlled componentSwitching from
defaultValuetovaluewill make the select reflect updates todisputeData.disputeKitIdafter initial render. Before applying this change, please verify:
- Confirm that
DropdownSelectsupports avalueprop (and that the existingcallback={handleDisputeKitChange}signature aligns with itsonChangeAPI).- Check other
DropdownSelectusages (e.g. in StatsAndFilters.tsx, Chart.tsx, ExtraStats.tsx, Filters.tsx) to see how they manage controlled vs. uncontrolled patterns.- If the component is indeed controlled, update the prop here and ensure no unintended UI regressions occur.
Suggested update:
- defaultValue={disputeData.disputeKitId} + value={disputeData.disputeKitId}
| queryFn: async () => { | ||
| if (!publicClient || !debouncedAddress) { | ||
| throw new Error("Missing required dependencies"); | ||
| } | ||
|
|
||
| try { | ||
| const contract = getContract({ | ||
| address: debouncedAddress as `0x${string}`, | ||
| abi, | ||
| client: publicClient, | ||
| }); | ||
|
|
||
| // Execute the contract call specific to the token type | ||
| await contractCall(contract); | ||
|
|
||
| return true; | ||
| } catch { | ||
| throw new Error(`Address does not implement ${tokenType} interface`); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Don't mask root causes; add bytecode check and preserve error classification
Catching all errors and rethrowing “does not implement …” makes downstream messages inaccurate and prevents distinguishing “not a contract” vs. transient network issues. Add a pre-check for bytecode and throw explicit sentinel errors so UI mapping works reliably.
Apply this diff to the queryFn to classify errors:
queryFn: async () => {
if (!publicClient || !debouncedAddress) {
throw new Error("Missing required dependencies");
}
- try {
- const contract = getContract({
- address: debouncedAddress as `0x${string}`,
- abi,
- client: publicClient,
- });
-
- // Execute the contract call specific to the token type
- await contractCall(contract);
-
- return true;
- } catch {
- throw new Error(`Address does not implement ${tokenType} interface`);
- }
+ // Ensure address has bytecode before attempting a contract call.
+ const bytecode = await publicClient.getBytecode({ address: debouncedAddress as `0x${string}` });
+ if (!bytecode || bytecode === "0x") {
+ throw new Error("NOT_A_CONTRACT");
+ }
+
+ try {
+ const contract = getContract({
+ address: debouncedAddress as `0x${string}`,
+ abi,
+ client: publicClient,
+ });
+ // Execute the contract call specific to the token type
+ await contractCall(contract);
+ return true;
+ } catch (_err) {
+ // The function exists only on valid token interfaces we expect.
+ // If the call fails, classify as interface mismatch rather than network error.
+ throw new Error("INVALID_INTERFACE");
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| queryFn: async () => { | |
| if (!publicClient || !debouncedAddress) { | |
| throw new Error("Missing required dependencies"); | |
| } | |
| try { | |
| const contract = getContract({ | |
| address: debouncedAddress as `0x${string}`, | |
| abi, | |
| client: publicClient, | |
| }); | |
| // Execute the contract call specific to the token type | |
| await contractCall(contract); | |
| return true; | |
| } catch { | |
| throw new Error(`Address does not implement ${tokenType} interface`); | |
| } | |
| }, | |
| }); | |
| queryFn: async () => { | |
| if (!publicClient || !debouncedAddress) { | |
| throw new Error("Missing required dependencies"); | |
| } | |
| // Ensure address has bytecode before attempting a contract call. | |
| const bytecode = await publicClient.getBytecode({ | |
| address: debouncedAddress as `0x${string}`, | |
| }); | |
| if (!bytecode || bytecode === "0x") { | |
| throw new Error("NOT_A_CONTRACT"); | |
| } | |
| try { | |
| const contract = getContract({ | |
| address: debouncedAddress as `0x${string}`, | |
| abi, | |
| client: publicClient, | |
| }); | |
| // Execute the contract call specific to the token type | |
| await contractCall(contract); | |
| return true; | |
| } catch (_err) { | |
| // The function exists only on valid token interfaces we expect. | |
| // If the call fails, classify as interface mismatch rather than network error. | |
| throw new Error("INVALID_INTERFACE"); | |
| } | |
| }, | |
| }); |
| const validationError = useMemo(() => { | ||
| if (!debouncedAddress || debouncedAddress.trim() === "") { | ||
| return null; | ||
| } | ||
|
|
||
| if (isValidFormat === false) { | ||
| return "Invalid Ethereum address format"; | ||
| } | ||
|
|
||
| if (error) { | ||
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | ||
| if (errorMessage.includes("not a contract")) { | ||
| return "Address is not a contract"; | ||
| } | ||
| if (errorMessage.includes(`does not implement ${tokenType}`)) { | ||
| return `Not a valid ${tokenType} token address`; | ||
| } | ||
| return "Network error - please try again"; | ||
| } | ||
|
|
||
| return null; | ||
| }, [debouncedAddress, isValidFormat, error, tokenType]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix unreachable error mappings; map to explicit sentinel errors
Because the catch above rewrites every error, “Address is not a contract” and “Network error” were unreachable. With sentinel errors, map them deterministically.
if (error) {
- const errorMessage = error instanceof Error ? error.message : "Unknown error";
- if (errorMessage.includes("not a contract")) {
- return "Address is not a contract";
- }
- if (errorMessage.includes(`does not implement ${tokenType}`)) {
- return `Not a valid ${tokenType} token address`;
- }
- return "Network error - please try again";
+ const errorMessage = error instanceof Error ? error.message : "Unknown error";
+ if (errorMessage === "NOT_A_CONTRACT") return "Address is not a contract";
+ if (errorMessage === "INVALID_INTERFACE") return `Not a valid ${tokenType} token address`;
+ return "Network error - please try again";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const validationError = useMemo(() => { | |
| if (!debouncedAddress || debouncedAddress.trim() === "") { | |
| return null; | |
| } | |
| if (isValidFormat === false) { | |
| return "Invalid Ethereum address format"; | |
| } | |
| if (error) { | |
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | |
| if (errorMessage.includes("not a contract")) { | |
| return "Address is not a contract"; | |
| } | |
| if (errorMessage.includes(`does not implement ${tokenType}`)) { | |
| return `Not a valid ${tokenType} token address`; | |
| } | |
| return "Network error - please try again"; | |
| } | |
| return null; | |
| }, [debouncedAddress, isValidFormat, error, tokenType]); | |
| const validationError = useMemo(() => { | |
| if (!debouncedAddress || debouncedAddress.trim() === "") { | |
| return null; | |
| } | |
| if (isValidFormat === false) { | |
| return "Invalid Ethereum address format"; | |
| } | |
| if (error) { | |
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | |
| if (errorMessage === "NOT_A_CONTRACT") return "Address is not a contract"; | |
| if (errorMessage === "INVALID_INTERFACE") return `Not a valid ${tokenType} token address`; | |
| return "Network error - please try again"; | |
| } | |
| return null; | |
| }, [debouncedAddress, isValidFormat, error, tokenType]); |
🤖 Prompt for AI Agents
In web/src/hooks/useTokenAddressValidation.ts around lines 189 to 210, the
current validation maps error cases by inspecting rewritten error.message which
makes the specific mappings unreachable; instead compare the error against
explicit sentinel values (or specific Error subclasses/named constants) emitted
by the catch logic. Update the logic to import/consume the sentinel error
identifiers (or check error.name or a well-known error.code) and map those to
"Address is not a contract" and `Not a valid ${tokenType} token address`,
falling back to "Network error - please try again" for anything else; ensure the
equality checks are deterministic (===) against the sentinel constants rather
than substring matching of error.message.
| const ValidationContainer = styled.div` | ||
| width: 84vw; | ||
| display: flex; | ||
| align-items: left; | ||
| gap: 8px; | ||
| margin-top: 8px; | ||
| ${landscapeStyle( | ||
| () => css` | ||
| width: ${responsiveSize(442, 700, 900)}; | ||
| ` | ||
| )} | ||
| `; |
There was a problem hiding this comment.
Invalid CSS: align-items: left; should be flex-start
align-items does not accept left; the rule is ignored. Use flex-start to align items to the start of the cross-axis.
const ValidationContainer = styled.div`
width: 84vw;
display: flex;
- align-items: left;
+ align-items: flex-start;
gap: 8px;
margin-top: 8px;
${landscapeStyle(
() => css`
width: ${responsiveSize(442, 700, 900)};
`
)}
`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ValidationContainer = styled.div` | |
| width: 84vw; | |
| display: flex; | |
| align-items: left; | |
| gap: 8px; | |
| margin-top: 8px; | |
| ${landscapeStyle( | |
| () => css` | |
| width: ${responsiveSize(442, 700, 900)}; | |
| ` | |
| )} | |
| `; | |
| const ValidationContainer = styled.div` | |
| width: 84vw; | |
| display: flex; | |
| align-items: flex-start; | |
| gap: 8px; | |
| margin-top: 8px; | |
| ${landscapeStyle( | |
| () => css` | |
| width: ${responsiveSize(442, 700, 900)}; | |
| ` | |
| )} | |
| `; |
🤖 Prompt for AI Agents
In web/src/pages/Resolver/Parameters/Court.tsx around lines 90 to 101, the
styled component uses an invalid CSS rule "align-items: left;" which is ignored;
change that to "align-items: flex-start;" (or another valid align-items value as
needed) so items align to the start of the cross-axis, leaving the rest of the
styles and responsive block unchanged.
Automatically stake PNK rewards instead of transferring them
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 (1)
contracts/src/arbitration/SortitionModuleBase.sol (1)
461-470: Ensure sortition trees are initialized before indexing nodes[0]We found a direct access to
tree.nodes[0]in contracts/src/arbitration/SortitionModuleBase.sol that will revert if the tree hasn’t yet been created (i.e.tree.nodes.length == 0). A simple length check avoids an out-of-bounds error.File: contracts/src/arbitration/SortitionModuleBase.sol
Lines: 461–470Apply:
if (phase != Phase.drawing) revert NotDrawingPhase(); SortitionSumTree storage tree = sortitionSumTrees[_key]; - if (tree.nodes[0] == 0) { + // No tree yet or no jurors staked. + if (tree.nodes.length == 0 || tree.nodes[0] == 0) { return address(0); } uint256 currentDrawnNumber = uint256( keccak256(abi.encodePacked(randomNumber, _coreDisputeID, _nonce)) ) % tree.nodes[0];Note: A repo-wide search also shows a
tree.nodes[0]access in the library’s draw function, but there the tree is always initialized viacreateTree, which pushes an initial zero, so no change is required in SortitionSumTreeFactory.sol.
♻️ Duplicate comments (3)
contracts/CHANGELOG.md (3)
20-20: Clarify pragma wording and scope (interfaces-only might be misleading)If the widening applies strictly to interfaces, keep it but specify the exact range. If other contracts (e.g., RNG) also adopted it, reflect that scope.
Apply one of the following (pick the accurate one):
- Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) + Widen interfaces' pragma to `>=0.8.0 <0.9.0` ([#2083](https://github.com/kleros/kleros-v2/issues/2083))or, if broader:
- Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) + Widen pragma to `>=0.8.0 <0.9.0` across interfaces (and RNG contracts) ([#2083](https://github.com/kleros/kleros-v2/issues/2083))Repo-wide verification:
#!/bin/bash echo "Files using widened pragma (>=0.8.0 <0.9.0):" rg -nP '^\s*pragma\s+solidity\s+>=0\.8\.0\s*<0\.9\.0\s*;' || true echo echo "Non-interface contracts with widened pragma (sanity check):" rg -nP '^\s*pragma\s+solidity\s+>=0\.8\.0\s*<0\.9\.0\s*;' -g '!**/interfaces/**' || true
7-8: Avoid linking to an unpublished 0.13.0 release tagThe section explicitly says “Not published yet,” but there’s a live link below. This will 404 until the tag is published. Comment it out (and re-enable post-release).
Apply:
-## [0.13.0] - 2025-08-07 (Not published yet) +## [0.13.0] - 2025-08-07 (Not published yet)-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%[email protected] +<!-- [0.13.0]: (add after publishing) -->To verify before merging:
#!/bin/bash # Exits non-200 if the tag isn't live yet set -e code=$(curl -s -o /dev/null -w "%{http_code}" \ "https://github.com/kleros/kleros-v2/releases/tag/@kleros%[email protected]") test "$code" = "200" && echo "Release link OK" || { echo "Release link not live ($code)"; exit 1; }Also applies to: 143-144
22-22: Fix constant name and phrasing for accuracyUse the correct constant name and clearer wording.
- Consolidate the constant `ALPHA_DIVISOR` with `ONE_BASIS_POINTS` ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) + Replace `ALPHA_DIVISOR` with `ONE_BASIS_POINT` ([#2090](https://github.com/kleros/kleros-v2/issues/2090))
🧹 Nitpick comments (10)
contracts/src/arbitration/university/SortitionModuleUniversity.sol (2)
196-219: setStakeReward semantics are sensible; add return docs and consider event coverageThe bool-return contract is intuitive (true on no-op or success, false when juror is no longer staked). Add a NatSpec
@returnnote for clarity. Event emission is already covered by_setStakeviaStakeSet.Proposed doc tweak:
- /// @param _reward The amount of PNK to be deposited as a reward. + /// @param _reward The amount of PNK to be deposited as a reward. + /// @return success True if applied or no-op (reward == 0); false if juror is no longer staked in the court.
220-262: Reduce double SSTORE when updating per-court stake aggregatesCurrent pattern performs two writes per parent level:
+= _newStake;then-= currentStake;. You can cut this to one SSTORE to save gas.Apply:
- juror.stakesByCourtID[currentCourtID] += _newStake; - juror.stakesByCourtID[currentCourtID] -= currentStake; + uint256 prev = juror.stakesByCourtID[currentCourtID]; + // Invariants ensure prev >= currentStake. + uint256 updated = prev + _newStake - currentStake; + juror.stakesByCourtID[currentCourtID] = updated;contracts/src/arbitration/SortitionModuleBase.sol (2)
53-58: Keeping deprecated storage preserves layoutLeaving
randomNumberRequestBlockandrngLookaheadin-place (marked DEPRECATED) is the right call for upgrade safety. Consider tagging with a custom NatSpec to help future maintainers.Add above each var:
- uint256 public randomNumberRequestBlock; // DEPRECATED: to be removed in the next redeploy + /// @custom:deprecated kept for storage layout compatibility; remove on next redeploy + uint256 public randomNumberRequestBlock; // DEPRECATED: to be removed in the next redeploy ... - uint256 public rngLookahead; // DEPRECATED: to be removed in the next redeploy + /// @custom:deprecated kept for storage layout compatibility; remove on next redeploy + uint256 public rngLookahead; // DEPRECATED: to be removed in the next redeploy
154-161: Emit an event when RNG is changed (optional)Consider an event to aid off-chain observability and incident response when RNG is rotated; also guard against zero address.
+ event RandomNumberGeneratorChanged(address indexed previous, address indexed current); @@ - function changeRandomNumberGenerator(IRNG _rng) external onlyByGovernor { - rng = _rng; + function changeRandomNumberGenerator(IRNG _rng) external onlyByGovernor { + require(address(_rng) != address(0), "RNG=0"); + IRNG prev = rng; + rng = _rng; + emit RandomNumberGeneratorChanged(address(prev), address(_rng)); if (phase == Phase.generating) { rng.requestRandomness(); } }contracts/src/arbitration/KlerosCoreBase.sol (6)
568-571: Commit period is now strictly time-gated; confirm DK/UX expectations and update tests accordinglyDropping the early-advance guard (areCommitsAllCast) means Commit -> Vote will never advance early, even if all commits are in. This aligns with the Shutter note, but changes liveness and UX: disputes can sit idle until the full commit timeout. Please:
- Verify downstream DK logic and front-end timers assume time-only progression for Period.commit.
- Add/adjust tests to ensure early commits do not move the period ahead of schedule.
If useful, I can draft a test matrix covering hiddenVotes on/off, early full-commit, and boundary timestamps.
791-799: Event now emits ‘coherence’; consider aligning the event parameter name for clarityTokenAndETHShift still declares parameter name _degreeOfCoherency, but you’re emitting coherence. Not functionally wrong, but it’s confusing for indexers and off-chain analytics.
Apply this cosmetic rename if ABI stability allows:
event TokenAndETHShift( address indexed _account, uint256 indexed _disputeID, uint256 indexed _roundID, - uint256 _degreeOfCoherency, + uint256 _coherence, int256 _pnkAmount, int256 _feeAmount, IERC20 _feeToken );
827-841: Split coherence (PNK vs Fee) with capping is fine; assert invariants at the DK levelUsing separate pnkCoherence and feeCoherence enables decoupled economics. However, execution relies on DK guarantees. At a minimum, please document and verify DK invariants:
- 0 <= pnkCoherence, feeCoherence <= ONE_BASIS_POINT (you clamp, good).
- The relationship between penalty-coherence and pnkCoherence (see next comment) must not allow unlocking more than was locked.
Add unit tests for edge cases: both coherences at 0, both at ONE_BASIS_POINT, and highly asymmetric pairs.
I can generate invariant tests against the Classic DK and any new DKs you introduced in this PR.
844-848: Risk of unlocking more than was locked if penalty- and reward-coherences diverge; ensure DK invariant or hard-cap unlocksYou unlock in penalties: unlockStake(account, penalty) where penalty = stake*(1 - coherence_penalty).
You unlock in rewards: unlockStake(account, pnkLocked) where pnkLocked = stake*pnkCoherence.Unless the DK guarantees coherence_penalty + pnkCoherence <= ONE_BASIS_POINT per juror, the sum of unlocks can exceed the initially locked stake, which would either revert in SortitionModule or leave accounting inconsistent. Conversely, if pnkCoherence + (1 - coherence_penalty) < ONE_BASIS_POINT, dust remains locked indefinitely.
Actions:
- Confirm/document the DK invariant coherence_penalty + pnkCoherence == ONE_BASIS_POINT (or <= with a well-defined dust handling).
- If not guaranteed, consider hard-capping the second unlock to the remaining locked amount for that juror/round.
If SortitionModule exposes “locked-for-round” balance, we can enforce a min() cap in rewards. Otherwise, we may need to record per-vote penalty in memory for use during reward unlocks.
858-862: New external call (setStakeReward) in the reward path increases reentrancy surface; add a nonReentrant guard on execute()Even though round.repartitions is set before external calls, execute() performs multiple external interactions (ERC20 transfers, ETH safeSend, DK and SortitionModule calls). A simple nonReentrant guard on execute() is a low-cost hardening.
Proposed minimal hardening (outside this hunk):
+import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; -abstract contract KlerosCoreBase is IArbitratorV2, Initializable, UUPSProxiable { +abstract contract KlerosCoreBase is IArbitratorV2, Initializable, UUPSProxiable, ReentrancyGuard { - function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external whenNotPaused { + function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external whenNotPaused nonReentrant {Alternatively, isolate all state writes before any external call and ensure no storage mutations remain after transfers.
863-871: Event emits pnkCoherence; rename param (nit) and ensure indexers updatedAs with penalties, consider renaming event parameter to _coherence for consistency. Not functionally required.
- emit TokenAndETHShift( + emit TokenAndETHShift( account, _params.disputeID, _params.round, - pnkCoherence, + pnkCoherence, int256(pnkReward), int256(feeReward), round.feeToken );
📜 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 (7)
contracts/CHANGELOG.md(2 hunks)contracts/src/arbitration/KlerosCoreBase.sol(5 hunks)contracts/src/arbitration/SortitionModuleBase.sol(12 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(2 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(6 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(4 hunks)contracts/test/foundry/KlerosCore.t.sol(53 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/src/arbitration/interfaces/ISortitionModule.sol
- contracts/src/arbitration/university/KlerosCoreUniversity.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`.
Applied to files:
contracts/src/arbitration/SortitionModuleBase.sol
⏰ 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). (7)
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (51)
contracts/CHANGELOG.md (1)
30-39: 0.12.0 notes read well; no action neededEntry is clear and references are specific. LGTM.
contracts/src/arbitration/university/SortitionModuleUniversity.sol (4)
69-76: Good move to typed errors for access controlSwitching to custom errors reduces bytecode size and improves consistency with the rest of the PR.
193-199: Wrapper to internal _setStake looks correctDelegating the write path to a single internal routine centralizes stake accounting and event emission.
317-321: Typed error on zero-withdrawal is consistentReverting with NotEligibleForWithdrawal() aligns with the base module and improves tooling for callers.
406-409: Error set is minimal and clearGovernorOnly, KlerosCoreOnly, and NotEligibleForWithdrawal are aligned with the base module. LGTM.
contracts/src/arbitration/SortitionModuleBase.sol (9)
10-12: IRNG import aligns with the interface migrationGood alignment with the RNG interface rename across the codebase.
102-116: Initializer signature updated to IRNG is correctInitializer now takes IRNG and sets delayedStakeReadIndex to 1. Matches the execution logic in executeDelayedStakes.
122-130: Typed errors for access controlGood consistency with the PR’s error approach; reduces bytecode and improves clarity.
167-186: Phase progression and RNG calls look correct
- Staking -> Generating: checks min time and open disputes; requests randomness.
- Generating -> Drawing: pulls randomness via IRNG; reverts if not ready.
- Drawing -> Staking: enforces maxDrawingTime gate.
lastPhaseChange updated once at the end—consistent across branches. LGTM.
191-199: Tree creation guards upgraded to typed errors
TreeAlreadyExistsandKMustBeGreaterThanOneare appropriate. LGTM.
202-218: Delayed stakes execution guards are appropriate
NotStakingPhaseandNoDelayedStakeToExecuteguard rails are clear. Index math looks correct for single remaining item edge cases.
333-376: Tree updates: parent traversal and stake-path usage are consistent
- Using stakePathID = account+leafCourt across all ancestor courts matches the expected data model.
- Renamed
currentCourtIDand parent traversal throughcore.courtsis clear.LGTM.
437-441: Consistent error for zero leftover withdrawalUsing NotEligibleForWithdrawal() harmonizes signaling across modules; state is set before external transfer—reentrancy-safe.
710-726: Error taxonomy is comprehensive and readableGood coverage of phases, RNG readiness, staking constraints, and governance guards. LGTM.
contracts/src/arbitration/KlerosCoreBase.sol (6)
768-779: Switch to penalty-coherence API and ONE_BASIS_POINT clamp looks correctUsing getDegreeOfCoherencePenalty(...) and clamping to ONE_BASIS_POINT is sound and defensive. Good change.
782-783: Penalty formula change to (1 - coherence) is semantically right; double-check rounding expectationsInteger division truncation will create small “dust.” That dust won’t be penalized here and won’t be unlocked until rewards. Ensure tests assert expected dust behavior per juror to avoid surprises.
849-854: Reward split by coherentCount and per-channel coherence looks goodDivision is safe because rewards execute only when coherentCount > 0. Residual rounding is correctly swept to governor later. LGTM.
1069-1074: applyCoherence() now scales by ONE_BASIS_POINT — LGTMClear semantics, consistent with the rest of the file and Constants. Good rename in the docstring.
1081-1082: _calculatePnkAtStake uses ONE_BASIS_POINT — confirm alpha remains in basis pointsThe math matches the new denominator. Please double-check that all alpha inputs across court creation and modification are still specified in basis points to avoid mis-staking.
827-834: Automate Invariant Verification for Dispute Kits and SortitionModuleTo guarantee that coherence rewards/penalties and juror stake accounting behave correctly across all Dispute Kits and SortitionModule implementations, add automated invariant checks and targeted unit tests:
• Dispute Kits
– Contracts:
•contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sollines 531–536 (getDegreeOfCoherenceReward) and 547–552 (getDegreeOfCoherencePenalty)
•contracts/src/arbitration/interfaces/IDisputeKit.sollines 72–77 and 87–92 (signatures & docs)
– Invariants: outputs must always be in [0, ONE_BASIS_POINT] (10 000 basis points). Confirm any clamping logic and add unit tests for edge vote IDs (0, max, mid), zero‐fee/PnK, and over-coherence scenarios.
– Constants reference:contracts/src/libraries/Constants.solline 24 (uint256 constant ONE_BASIS_POINT = 10000;)• SortitionModule
– Contracts:
•contracts/src/arbitration/SortitionModuleBase.sol
– unlockStake (lines 383–386)
– penalizeStake (394–397)
– setStakeReward (318–321)
•contracts/src/arbitration/university/SortitionModuleUniversity.sol
– unlockStake (269–272)
– penalizeStake (274–277)
– setStakeReward (205–208)
– Invariants:
•jurors[_account].lockedPnkmust never underflow or exceed original stake;
•penalizeStakereturns correct(pnkBalance, availablePenalty);
• events (StakeLocked) accurately reflect state changes.
– Add unit tests covering lock → unlock, lock → penalize, reward flows, and underflow protection.• Test Coverage
– Existing tests incontracts/test/foundry/KlerosCore.t.solcover Classic DK coherence rewards/penalties; extend or create new tests for:
• University DK (KlerosCoreUniversity.sol) coherence scenarios;
• All SortitionModule functions (execute,passPeriod,TokenAndETHShift, plus direct calls to lock/unlock/penalize/setStakeReward).By codifying these invariants into your test suite (e.g., property-based tests, fuzzing across vote IDs and stake amounts), you’ll catch regressions early and avoid manual assumptions about coherence and stake behavior.
contracts/test/foundry/KlerosCore.t.sol (31)
15-16: New RNG imports align with migration objectives.The imports for
RNGWithFallbackandRNGMockare correctly added to support the RNG migration described in the PR objectives. These interfaces will be used in the new test functions for RNG fallback scenarios.
58-58: RNG lookahead variable properly declared.The variable naming is clear and the comment indicates it's in seconds, which aligns with the BlockHashRNG constructor usage below.
99-101: RNG initialization updated correctly.The BlockHashRNG constructor now takes the governor, sortition module address, and lookahead time parameters. This aligns with the new RNG interface changes mentioned in the PR objectives.
115-121: SortitionModule initialization signature updated.The initialize call now uses a 5-parameter form including the RNG address, which matches the changes described in the AI summary for constructor/signature alignment.
125-126: RNG consumer change follows proper access control.The
changeConsumercall is correctly made by the governor, establishing the sortition module as the RNG consumer. This follows the expected governance pattern.
286-288: Consistent RNG setup in event test.The RNG initialization in the event test follows the same pattern as the main setup, ensuring consistency across test scenarios.
302-308: Event test maintains parameter consistency.The sortition module initialization in the event test uses the same 5-parameter signature, maintaining consistency with the main test setup.
312-313: RNG consumer setup duplicated correctly.The RNG consumer change is consistently applied in the event test scenario as well.
572-573: Error handling updated to use custom errors.The test correctly expects
NotCommitPeriod.selectorinstead of a string-based require message, aligning with the PR's migration from require() checks to custom errors.
589-590: Consistent custom error usage.The test properly expects
EmptyCommit.selector, continuing the pattern of using custom error selectors.
595-596: Access control error properly tested.The test correctly expects
JurorHasToOwnTheVote.selectorfor access control violations.
634-639: Hash verification errors properly handled.Both test cases for invalid hash verification use the appropriate custom error selector
HashDoesNotMatchHiddenVoteCommitment.selector.
706-707: Vote period error handling updated.The test correctly uses
NotVotePeriod.selectorfor period validation errors.
724-725: Empty vote IDs error properly tested.The test uses the appropriate custom error
EmptyVoteIDs.selector.
730-731: Choice bounds validation updated.The test correctly expects
ChoiceOutOfBounds.selectorfor invalid choice values.
734-735: Vote ownership validation consistent.The test properly uses
JurorHasToOwnTheVote.selectorfor access control.
743-744: Duplicate vote prevention properly tested.The test correctly expects
VoteAlreadyCast.selectorwhen attempting to vote twice.
978-979: Appeal choice validation updated.The test uses the appropriate custom error
ChoiceOutOfBounds.selectorfor invalid appeal choices.
1022-1069: Time progression properly updated.The migration from
vm.roll()tovm.warp()throughout the test functions is correct, as mentioned in the AI summary. This change reflects the shift from block-based to time-based progression, aligning with the RNG lookahead mechanism.
1144-1158: Delayed stake error handling updated.The tests correctly use custom error selectors
NoDelayedStakeToExecute.selectorandNotStakingPhase.selectorinstead of string-based require messages.
1277-1285: Snapshot proxy error handling consistent.The tests properly use
KlerosCoreSnapshotProxy.GovernorOnly.selectorfor access control validation in the snapshot proxy.
2003-2004: Appeal funding error handling updated.The test correctly expects
AppealFeeIsAlreadyPaid.selectorfor duplicate appeal funding attempts.
2032-2048: Appeal timing validation properly tested.All three appeal timing error cases use appropriate custom error selectors:
AppealPeriodIsOver.selectorandAppealPeriodIsOverForLoser.selector.
2218-2219: Dispute kit jump error properly handled.The test correctly expects
DisputeJumpedToParentDK.selectorwhen attempting operations on a jumped dispute kit.
2909-2917: Withdrawal error handling updated.Both withdrawal error cases use appropriate custom error selectors:
DisputeNotResolved.selectorandCoreIsPaused.selector.
3015-3016: Inactive dispute kit error properly tested.The test correctly expects
NotActiveForCoreDisputeID.selectorwhen attempting to use an inactive dispute kit.
2320-2345: New coherence reward/penalty functions tested.The new
getDegreeOfCoherenceRewardandgetDegreeOfCoherencePenaltyfunctions are properly tested, showing the coherence API split mentioned in the PR objectives. The test validates both PNK and fee coherence values correctly.
2400-2402: Token balance calculations verified.The token balance assertions correctly account for the reward distribution logic, ensuring the core contract and stakers have the expected PNK balances after execution.
2430-2447: No coherence scenario properly tested.The test correctly validates that when no jurors are coherent, both reward and penalty functions return zero values, ensuring proper edge case handling.
2640-2641: Withdrawal eligibility error updated.The test correctly expects
NotEligibleForWithdrawal.selectorfor withdrawal timing validation.
3037-3144: Comprehensive RNG fallback test suite.The three new RNG fallback test functions (
test_RNGFallback,test_RNGFallback_happyPath,test_RNGFallback_sanityChecks) provide thorough coverage of the new RNG fallback mechanism:
- Fallback scenario: Tests timeout-triggered fallback to blockhash
- Happy path: Tests normal RNG operation without fallback
- Sanity checks: Tests access control and parameter changes
The tests properly validate event emissions (
RNGFallbackandFallbackTimeoutChanged), access control with custom errors (ConsumerOnly.selector,GovernorOnly.selector), and deterministic RNG behavior as described in the PR objectives.
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 (1)
contracts/src/arbitration/KlerosCoreBase.sol (1)
747-750: Reentrancy window in execute(): update storage after external calls.round.pnkPenalties is written after loops while _executePenalties/_executeRewards perform external calls (sortitionModule, token transfers). Protect with a reentrancy guard or write critical storage before interactions.
Minimal hardening (custom guard; matches custom errors style):
abstract contract KlerosCoreBase is IArbitratorV2, Initializable, UUPSProxiable { + bool private _reentrancyLock; + error Reentrancy(); + modifier nonReentrant() { + if (_reentrancyLock) revert Reentrancy(); + _reentrancyLock = true; + _; + _reentrancyLock = false; + }Apply to execute():
- function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external whenNotPaused { + function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external whenNotPaused nonReentrant {Optionally also write pnkPenalties before any fee transfer to governor in the “no coherent jurors” branch (inside _executePenalties), but the guard is sufficient and simpler.
♻️ Duplicate comments (5)
contracts/hardhat.config.ts (1)
31-35: Parameterize optimizer runs to avoid Hardhat/Foundry divergence.Match Foundry by reading runs from an env var; keeps bytecode/gas comparable across toolchains.
viaIR: process.env.VIA_IR !== "false", // Defaults to true optimizer: { enabled: true, - runs: 2000, + runs: Number(process.env.SOLC_OPTIMIZER_RUNS ?? 2000), },contracts/CHANGELOG.md (3)
20-20: Clarify pragma widening scope and exact range.-- Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) +- Widen interfaces' pragma to `>=0.8.0 <0.9.0` (and RNG contracts where applicable) ([#2083](https://github.com/kleros/kleros-v2/issues/2083))
22-22: Fix constant name.Use ONE_BASIS_POINT (singular).
-- Consolidate the constant `ALPHA_DIVISOR` with `ONE_BASIS_POINTS` ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) +- Replace `ALPHA_DIVISOR` with `ONE_BASIS_POINT` ([#2090](https://github.com/kleros/kleros-v2/issues/2090))
147-148: Avoid broken release link for 0.13.0 until published.-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%[email protected] +<!-- [0.13.0]: (add after publishing) -->contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
137-140: Guard against inactive disputes in notJumped.Avoid reading uninitialized local ID when DK is inactive.
modifier notJumped(uint256 _coreDisputeID) { + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); _; }
🧹 Nitpick comments (6)
contracts/hardhat.config.ts (2)
31-31: Make VIA_IR parsing robust to common falsy values.Treat "0"/"no"/"false" as false; everything else true.
- viaIR: process.env.VIA_IR !== "false", // Defaults to true + viaIR: + process.env.VIA_IR === undefined + ? true + : !/^(false|0|no)$/i.test(process.env.VIA_IR),
296-299: Unify Etherscan API key env var.Top-level uses ETHERSCAN_API_KEY_FIX but networks use ETHERSCAN_API_KEY. Pick one to avoid surprises in CI.
verify: { etherscan: { - apiKey: process.env.ETHERSCAN_API_KEY_FIX, + apiKey: process.env.ETHERSCAN_API_KEY, }, },contracts/src/arbitration/interfaces/IDisputeKit.sol (2)
80-94: Penalty function doc: fix return description.It returns coherence used for the penalty, not a “reward.”
- /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. + /// @return pnkCoherence The degree of coherence (in basis points) used to compute the PNK penalty.
121-129: Consider marking _previousDisputeKit as deprecated or making it optional.There’s a TODO to remove it; add an inline “deprecated” note to guide integrators now.
- /// @param _previousDisputeKit The previous Dispute Kit. + /// @param _previousDisputeKit The previous Dispute Kit. Deprecated: only used during transition.contracts/src/arbitration/KlerosCoreBase.sol (1)
931-949: Appeal cost now uses DK-specific nbVotesAfterAppeal: verify downstream assumptions.If some DKs return odd/even sequences, ensure off-chain UIs and tests expect that count for fee displays.
Would you like a quick check script to diff nbVotesAfterAppeal across DKs?
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
542-556: Penalty coherence API: fix doc wording.Return describes reward; should say penalty coherence.
- /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. + /// @return pnkCoherence The degree of coherence (in basis points) used to compute the PNK penalty.
📜 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/CHANGELOG.md(2 hunks)contracts/hardhat.config.ts(1 hunks)contracts/src/arbitration/KlerosCoreBase.sol(9 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(11 hunks)contracts/src/arbitration/interfaces/IDisputeKit.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). (12)
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Analyze (javascript)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (9)
contracts/src/arbitration/KlerosCoreBase.sol (6)
568-571: Commit→Vote transition: LGTM, matches Shutter constraint.Time-gated only; avoids breaking auto-reveal when all commits are in.
645-653: Court-jump emission: LGTM.Centralized jump computation with explicit CourtJump event is clearer and reduces duplication.
760-776: Penalty math: LGTM.Capped coherence and (1 − coherence) scaling over ONE_BASIS_POINT are correct.
820-846: Rewards split: LGTM.Separate pnkCoherence/feeCoherence with capping and coherence-weighted splits is sound.
1106-1110: ONE_BASIS_POINT adoption: LGTM.Clearer than ALPHA_DIVISOR and consistent with interface.
Also applies to: 1117-1118
1225-1225: Do not renameInvalidDisputKitParent– it’s referenced externallyRenaming this error would break downstream tooling and on-chain metadata. References include:
subgraph/core-neo/abi-migrations/KlerosCoreNeo.json(ABI entry “InvalidDisputKitParent”)subgraph/core/abi-migrations/KlerosCore.json(ABI entry “InvalidDisputKitParent”)contracts/src/arbitration/university/KlerosCoreUniversity.sol(same error declaration)- Multiple deployment artifacts under
contracts/deployments/arbitrum/...and elsewhereUnless you’re prepared to update every ABI migration, deployment JSON, subgraph, and any client expecting the old name, it’s safest to leave the typo in place.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
272-275: Access and input guards: LGTM.Period checks, empty inputs, ownership, and bounds validations are crisp and use custom errors.
Also applies to: 313-316, 363-367, 373-378, 385-385
525-541: Reward coherence API: LGTM.Binary coherence mapped to both PNK and fee coherences; consistent with ONE_BASIS_POINT.
630-645: Appeal knobs: LGTM.No early court jump and 2n+1 vote growth are explicit and predictable for Classic.
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
contracts/src/token/Faucet.sol (1)
55-62: Apply Checks-Effects-Interactions to prevent reentrancy; use SafeERC20.Set the “withdrewAlready” flag before external transfer, then transfer safely.
function request() public { require( !withdrewAlready[msg.sender], "You have used this faucet already. If you need more tokens, please use another address." ); - token.transfer(msg.sender, amount); - withdrewAlready[msg.sender] = true; + withdrewAlready[msg.sender] = true; + token.safeTransfer(msg.sender, amount); }Optional: add ReentrancyGuard if other mutating logic is added later.
contracts/scripts/utils/execution.ts (1)
30-39: Unify request building and fix type mismatch with sendTransactionContractTransaction is not a TransactionRequest. Build a request object once and reuse it for both paths.
- if (isContract) { - // Don't execute, just log the tx. It must be submitted for execution separately. - const { to, value, data } = tx; - transactions.push(transaction({ to, value, data })); - console.log("tx = %O", tx); - } else { - // Execute the tx - const signer = (await ethers.getSigners())[0]; - await signer.sendTransaction(tx); - } + const { to, value, data } = tx as { to: string; value?: any; data: string }; + const request = { to, value, data }; + if (isContract) { + // Don't execute, just log the tx. It must be submitted for execution separately. + transactions.push(transaction(request)); + console.log("tx = %O", tx); + } else { + // Execute the tx + const signer = (await ethers.getSigners())[0]; + await signer.sendTransaction(request as any); + }Follow-up: consider changing the execute signature to accept PopulatedTransaction | TransactionRequest for stronger typing.
contracts/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol (1)
4-4: Pragma version pinning still present—please widen to ≥0.8.0 <0.9.0The PR description states that all Solidity files should use a widened pragma (
>=0.8.0 <0.9.0), but a search across the repo shows ~70 files (includingUpgradedByRewrite.sol) still pinned to^0.8.24. To align with the PR objective, update the pragma in these files or confirm if you intentionally want to pin them to 0.8.24.Key locations (non-exhaustive):
- contracts/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol (line 4)
- contracts/src/proxy/mock/by-rewrite/UpgradedByRewriteV2.sol (line 4)
- contracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol (line 4)
- contracts/src/token/PinakionV2.sol (line 3)
- contracts/src/gateway/HomeGateway.sol (line 3)
- contracts/src/arbitration/KlerosCore.sol (line 3)
- contracts/src/libraries/SafeERC20.sol (line 4)
- contracts/src/rng/ChainlinkRNG.sol (line 3)
- contracts/src/test/RNGMock.sol (line 2)
- contracts/test/foundry/KlerosCore.t.sol (line 2)
Please update each
pragma solidity ^0.8.24;to:- pragma solidity ^0.8.24; + pragma solidity >=0.8.0 <0.9.0;…or document why certain contracts must remain pinned to 0.8.24.
contracts/src/arbitration/evidence/EvidenceModule.sol (1)
3-3: Update Solidity Pragma to Match PR ObjectiveThe pragma in
EvidenceModule.solcurrently uses^0.8.24, but the PR specifies a range of>=0.8.0 <0.9.0. Please align this file with the intended solidity version constraint.• File:
contracts/src/arbitration/evidence/EvidenceModule.sol(line 3)
• Change pragma to the open-range format specified in the PRSuggested diff:
- pragma solidity ^0.8.24; + pragma solidity >=0.8.0 <0.9.0;contracts/src/arbitration/devtools/KlerosCoreRuler.sol (2)
258-266: Validate parent court index before use.Out-of-bounds on courts[_parent] will revert without a clear error.
- ) external onlyByOwner { + ) external onlyByOwner { + if (_parent >= courts.length) revert InvalidForkingCourtAsParent(); // or add a new InvalidParentCourt() if (_parent == FORKING_COURT) revert InvalidForkingCourtAsParent();
333-337: Guard currency rates against zero to avoid div-by-zero.convertEthToTokenAmount divides by rateInEth; ensure it’s non-zero.
- function changeCurrencyRates(IERC20 _feeToken, uint64 _rateInEth, uint8 _rateDecimals) external onlyByOwner { + error InvalidRate(); + function changeCurrencyRates(IERC20 _feeToken, uint64 _rateInEth, uint8 _rateDecimals) external onlyByOwner { + if (_rateInEth == 0) revert InvalidRate(); currencyRates[_feeToken].rateInEth = _rateInEth; currencyRates[_feeToken].rateDecimals = _rateDecimals; emit NewCurrencyRate(_feeToken, _rateInEth, _rateDecimals); }contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
69-75: Add safety checks around the PoH registry before using itThe
DisputeKitSybilResistantcontract currently assigns the PoH registry address without validation and callspoh.isRegistered(...)directly in a view method, which can lead to unexpected reverts if:
initialize(...)is called with the zero address (sopoh == address(0)), causing anypoh.isRegisteredcall to revert.- The Proof-of-Humanity contract is paused, upgraded, or otherwise malfunctions, also causing
isRegisteredto revert and break the drawing logic.To harden this:
• In initialize (around line 50), require a non-zero registry address:
--- a/contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol +++ b/contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol @@ function initialize( - poh = _poh; + require(address(_poh) != address(0), "PoH registry cannot be zero"); + poh = _poh;• In
_postDrawCheck(lines 69–75), wrap the external call in a try/catch to prevent bubbling reverts:--- a/contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol +++ b/contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol @@ function _postDrawCheck( - return super._postDrawCheck(_round, _coreDisputeID, _juror) - && poh.isRegistered(_juror); + bool registered; + try poh.isRegistered(_juror) returns (bool ok) { + registered = ok; + } catch { + // PoH check failed—treat as unregistered to avoid revert + registered = false; + } + return super._postDrawCheck(_round, _coreDisputeID, _juror) && registered;These changes ensure:
- The registry address is never left unset.
- Any failure in the external PoH call safely defaults to “not registered” rather than reverting the entire draw.
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
3-3: Update Solidity version constraint to align with PR objectives.The PR summary indicates that Solidity version constraints were updated from ^0.8.24 to >=0.8.0 <0.9.0, but this file still uses
^0.8.24.Apply this diff to update the version constraint:
-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;contracts/src/kleros-v1/interfaces/IKlerosLiquid.sol (1)
3-3: Align Solidity compiler pragma with PR objectiveThe PR aims to broaden supported compiler versions to
>=0.8.0 <0.9.0, but several files—includingIKlerosLiquid.sol—remain pinned to^0.8.24. This mismatch can cause compilation inconsistencies when consuming these contracts together.Files requiring pragma updates:
- contracts/src/kleros-v1/interfaces/IKlerosLiquid.sol
Proposed diff:
- pragma solidity ^0.8.24; + pragma solidity >=0.8.0 <0.9.0;Please apply the same change (or confirm any intentional exceptions) across the rest of the
contracts/src/kleros-v1/directory to ensure consistency.
♻️ Duplicate comments (14)
contracts/CHANGELOG.md (3)
148-149: Remove unpublished 0.13.0 release link (leave placeholder until tag is live).The link 0.13.0 currently points to an unpublished tag and will 404. Keep the note “Not published yet” but avoid a broken link.
Apply:
-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%[email protected] +<!-- [0.13.0]: (add after publishing) -->Optional script to verify before merging:
#!/bin/bash curl -s -o /dev/null -w "%{http_code}\n" \ "https://github.com/kleros/kleros-v2/releases/tag/@kleros%[email protected]"
21-21: Clarify pragma widening scope and exact range.The “interfaces only” wording is misleading given RNG contracts also widened to >=0.8.0 <0.9.0.
Apply:
-- Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) +- Widen the allowed pragma to `>=0.8.0 <0.9.0` for interfaces and RNG contracts ([#2083](https://github.com/kleros/kleros-v2/issues/2083))
23-23: Fix constant name: ONE_BASIS_POINT (singular).Apply:
-- Consolidate the constant `ALPHA_DIVISOR` with `ONE_BASIS_POINTS` ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) +- Replace `ALPHA_DIVISOR` with `ONE_BASIS_POINT` ([#2090](https://github.com/kleros/kleros-v2/issues/2090))contracts/src/rng/BlockhashRNG.sol (1)
72-75: Timing-manipulation vulnerability + anchor-block missing; caller can bias randomness by choosing the block after readinessAs implemented, once ready,
receiveRandomness()derives fromblockhash(block.number - 1). Since the consumer path can be triggered by anyone upstream, the first external caller effectively picks the block, enabling timing-based bias. We previously flagged this; still applies.Introduce a pre-committed anchor block at request time and always derive from that block (+1) after the time gate. Also persist the block used for auditing, and add minimal cleanup to avoid storage growth.
@@ - uint256 public requestTimestamp; // Timestamp of the current request - mapping(uint256 timestamp => uint256 number) public randomNumbers; // randomNumbers[timestamp] is the random number for this timestamp, 0 otherwise. + uint256 public requestTimestamp; // Timestamp of the current request. + uint256 public requestBlock; // Block number at request time (anchor to remove caller timing bias). + mapping(uint256 timestamp => uint256 number) public randomNumbers; // randomNumbers[timestamp] is the random number for this timestamp window, 0 otherwise. + mapping(uint256 timestamp => uint256 blockNumber) public randomBlocks; // Block used to derive that randomness. + uint256 private _lastExpectedTimestamp; // Track previous window for cleanup on new request. @@ - function requestRandomness() external override onlyByConsumer { - requestTimestamp = block.timestamp; - } + function requestRandomness() external override onlyByConsumer { + // Cleanup previous saved randomness to prevent unbounded storage growth. + if (_lastExpectedTimestamp != 0) { + delete randomNumbers[_lastExpectedTimestamp]; + delete randomBlocks[_lastExpectedTimestamp]; + _lastExpectedTimestamp = 0; + } + requestTimestamp = block.timestamp; + requestBlock = block.number; + _lastExpectedTimestamp = requestTimestamp + lookaheadTime; + } @@ - // Use last block hash for randomness - randomNumber = uint256(blockhash(block.number - 1)); - if (randomNumber != 0) { - randomNumbers[expectedTimestamp] = randomNumber; - } - return randomNumber; + // Derive from pre-committed block to prevent timing selection attacks. + uint256 targetBlock = requestBlock + 1; + randomNumber = uint256(blockhash(targetBlock)); + if (randomNumber == 0) { + // Target block not yet available or its hash expired (>256 blocks). Let caller retry or re-request. + return 0; + } + randomNumbers[expectedTimestamp] = randomNumber; + randomBlocks[expectedTimestamp] = targetBlock; + return randomNumber;Also applies to: 95-101, 18-23
contracts/src/arbitration/arbitrables/ArbitrableExample.sol (2)
143-147: Critical: Unknown arbitrator dispute IDs resolve to disputes[0]; add sentinel check.Absent a 1-based sentinel, an unknown _arbitratorDisputeID maps to 0 and targets disputes[0]. Also exploitable via reentrancy during createDispute before the mapping write.
Apply these diffs:
In rule():
- uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID]; - DisputeStruct storage dispute = disputes[localDisputeID]; + uint256 localDisputeIDPlusOne = externalIDtoLocalID[_arbitratorDisputeID]; + if (localDisputeIDPlusOne == 0) revert UnknownDisputeID(); + uint256 localDisputeID = localDisputeIDPlusOne - 1; + DisputeStruct storage dispute = disputes[localDisputeID];Where the mapping is written:
- externalIDtoLocalID[disputeID] = localDisputeID; + externalIDtoLocalID[disputeID] = localDisputeID + 1;(Apply the same change in both createDispute overloads.)
Add the error (see errors block below):
+ error UnknownDisputeID();
155-164: Extend errors with UnknownDisputeID used by the guard above.Pairs with the sentinel fix in rule().
error OwnerOnly(); error TransferFailed(); error AllowanceIncreaseFailed(); error ArbitratorOnly(); error RulingOutOfBounds(); error DisputeAlreadyRuled(); + error UnknownDisputeID();contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol (2)
204-207: Arithmetic switch from CappedMath is fine on ^0.8.24.Checked math in Solidity 0.8+ already reverts on overflow; no extra checks needed here.
245-251: Deposit calculation path OK; consider explicit cap logic note.Logic matches “double opposition or min-required”. No overflow risk on ^0.8; comment retained.
contracts/src/arbitration/KlerosGovernor.sol (2)
339-353: Reentrancy window: mark executed before external call.Set executed = true pre-call to avoid re-entrancy on target.
- if (!transaction.executed && transaction.value <= expendableFunds) { - (bool callResult, ) = transaction.target.call{value: transaction.value}(transaction.data); - // An extra check to prevent re-entrancy through target call. - if (callResult == true) { - if (transaction.executed) revert AlreadyExecuted(); - transaction.executed = true; - } - } + if (!transaction.executed && transaction.value <= expendableFunds) { + transaction.executed = true; // effects first + (bool callResult, ) = transaction.target.call{value: transaction.value}(transaction.data); + if (!callResult) { + transaction.executed = false; // preserve current non-reverting semantics on failure + } + }
211-213: Add calldata bounds check when slicing _data.Prevent panic on out-of-bounds while copying.
uint256 readingPosition; for (uint256 i = 0; i < _target.length; i++) { + if (readingPosition + _dataSize[i] > _data.length) revert WrongInputTargetAndDatasize(); bytes memory readData = new bytes(_dataSize[i]);contracts/hardhat.config.ts (1)
34-34: Parameterize optimizer runs via env to align toolchains.Hardcoding 2000 may diverge from Foundry. Use SOLC_OPTIMIZER_RUNS.
optimizer: { enabled: true, - runs: 2000, + runs: Number(process.env.SOLC_OPTIMIZER_RUNS ?? 2000), },contracts/src/rng/RNGWithFallback.sol (1)
47-55: Custom errors must be qualified with IRNG to compile.The custom errors
OwnerOnlyandConsumerOnlyare declared inside the IRNG interface, so unqualified references here will not compile. This needs to be fixed for the contract to compile successfully.Apply this fix:
modifier onlyByOwner() { - if (owner != msg.sender) revert OwnerOnly(); + if (owner != msg.sender) revert IRNG.OwnerOnly(); _; } modifier onlyByConsumer() { - if (consumer != msg.sender) revert ConsumerOnly(); + if (consumer != msg.sender) revert IRNG.ConsumerOnly(); _; }contracts/test/arbitration/staking-neo.ts (1)
263-266: Fix Chai assertion chain for custom error tests.The assertion uses
to.revertedWithCustomErrorinstead of the correctto.be.revertedWithCustomErrorchain that matches the Hardhat/Waffle plugin API used elsewhere in the file.Apply this fix:
-await expect(sortition.executeDelayedStakes(10)).to.revertedWithCustomError( +await expect(sortition.executeDelayedStakes(10)).to.be.revertedWithCustomError( sortition, "NoDelayedStakeToExecute" );Also applies to lines 330-333.
Also applies to: 330-333
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
137-140: Consider guarding against uninitialized disputes in notJumped modifierThe modifier accesses
disputes[coreDisputeIDToLocal[_coreDisputeID]]without first checking if the dispute is active. This could lead to reading an uninitialized dispute struct.modifier notJumped(uint256 _coreDisputeID) { + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); _; }
🧹 Nitpick comments (72)
contracts/scripts/viemTest.ts (3)
18-18: Simplify: avoid mixing await with then for logging.Use a single await for clarity and better stack traces.
- await disputeKit.read.owner().then(console.log); + console.log(await disputeKit.read.owner());
12-16: Guard against missing address for the selected chain.Fail fast if the mapping is undefined to avoid getContract with an invalid address.
- const disputeKit = getContract({ - address: disputeKitClassicConfig.address[arbitrumSepolia.id], + const address = disputeKitClassicConfig.address[arbitrumSepolia.id]; + if (!address) throw new Error(`No DisputeKitClassic address for chain ${arbitrumSepolia.id}`); + const disputeKit = getContract({ + address, abi: disputeKitClassicConfig.abi, client: client, });
35-38: Handle missing/overloaded ABI function safely.Using filter()[0] risks undefined and overload confusion. Prefer find() and validate.
- const getFunctionReturnParameterNames = (abi: AbiFunction[], name: string): string[] => { - const f = abi.filter((abi: AbiFunction) => abi.type === "function" && abi.name === name)[0]; // WARNING: overloaded functions confusion - return f.outputs.map((item) => item.name).filter(String) as string[]; - }; + const getFunctionReturnParameterNames = (abi: AbiFunction[], name: string): string[] => { + const f = abi.find((item: AbiFunction) => item.type === "function" && item.name === name); + if (!f) throw new Error(`Function ${name} not found in ABI`); + return (f.outputs ?? []).map((item) => item.name).filter(Boolean) as string[]; + };contracts/CHANGELOG.md (3)
15-15: Tighten phrasing and use the canonical EIP link.Apply:
-- **Breaking:** Rename `governor` to `owner` in order to comply with the lightweight ownership standard [ERC-5313](https://eipsinsight.com/ercs/erc-5313) ([#2112](https://github.com/kleros/kleros-v2/issues/2112)) +- **Breaking:** Rename `governor` to `owner` to comply with [ERC-5313](https://eips.ethereum.org/EIPS/eip-5313) ([#2112](https://github.com/kleros/kleros-v2/issues/2112))
17-17: Prefer “third parties” over “3rd parties.”Apply:
-- Authenticate the calls to the RNGs to prevent 3rd parties from depleting the Chainlink VRF subscription funds ([#2054](https://github.com/kleros/kleros-v2/issues/2054)) +- Authenticate RNG calls to prevent third parties from depleting the Chainlink VRF subscription funds ([#2054](https://github.com/kleros/kleros-v2/issues/2054))
33-33: Grammar: “advance to the Voting period.”Apply:
-- Do not pass to Voting period if all the commits are cast because it breaks the current Shutter auto-reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085)) +- Do not advance to the Voting period if all commits are cast, as it breaks the current Shutter auto‑reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085))contracts/src/token/Faucet.sol (4)
3-3: Align pragma with repo-wide version range.PR states contracts target >=0.8.0 <0.9.0; this file still pins ^0.8.24.
-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;
21-23: Use a custom error and drop redundant address() cast in modifier.Consistent with PR’s move to custom errors. Also removes an unnecessary cast.
modifier onlyByOwner() { - require(address(owner) == msg.sender, "Access not allowed: Owner only."); + if (msg.sender != owner) revert OwnerOnly(); _; }Add outside this range:
error OwnerOnly();Optional: rename modifier to onlyOwner to match OZ conventions.
43-45: Guard against disabling the faucet by setting amount to 0.function changeAmount(uint256 _amount) public onlyByOwner { - amount = _amount; + require(_amount > 0, "Amount must be > 0"); + amount = _amount; }
68-70: Prefer explicit uint256 over uint.- function balance() public view returns (uint) { + function balance() public view returns (uint256) { return token.balanceOf(address(this)); }contracts/src/rng/RandomizerRNG.sol (2)
53-62: Consider adding zero-address validation for critical parameters.While the constructor logic is correct, adding validation for
_owner,_consumer, and_randomizerwould prevent deployment with invalid addresses that could render the contract unusable.constructor(address _owner, address _consumer, IRandomizer _randomizer) { + if (_owner == address(0)) revert InvalidAddress(); + if (_consumer == address(0)) revert InvalidAddress(); + if (address(_randomizer) == address(0)) revert InvalidAddress(); owner = _owner; consumer = _consumer; randomizer = _randomizer; callbackGasLimit = 50000; }Add the error definition at line 132:
error RandomizerOnly(); +error InvalidAddress();
124-126: Consider adding validation for unfulfilled requests.The function could return a more predictable value or revert when no random number has been generated yet (when
randomNumbers[lastRequestId]is 0).function receiveRandomness() external view override returns (uint256 randomNumber) { randomNumber = randomNumbers[lastRequestId]; + if (randomNumber == 0) revert RandomnessNotAvailable(); }Add the error definition at line 132:
error RandomizerOnly(); +error RandomnessNotAvailable();Note: This suggestion assumes that 0 is not a valid random number in your use case. If 0 is valid, consider using a separate boolean flag to track fulfillment status.
contracts/src/rng/ChainlinkConsumerBaseV2Plus.sol (2)
38-43: Docs: constructor example is outdated (arg order and base-call).Update the example to reflect (owner, vrfCoordinator) order and no visibility on base call.
- * @dev contract VRFConsumerV2Plus is VRFConsumerBaseV2Plus { - * @dev constructor(<other arguments>, address _vrfCoordinator, address _subOwner) - * @dev VRFConsumerBaseV2Plus(_vrfCoordinator, _subOwner) public { + * @dev contract VRFConsumerV2Plus is VRFConsumerBaseV2Plus { + * @dev constructor(<other args>, address _owner, address _vrfCoordinator) + * @dev VRFConsumerBaseV2Plus(_owner, _vrfCoordinator) {
71-75: Docs: method name mismatch (Randomness vs RandomWords).Replace rawFulfillRandomness with rawFulfillRandomWords in the comments.
- * @dev (specifically, by the VRFConsumerBaseV2Plus.rawFulfillRandomness method). + * @dev (specifically, by the VRFConsumerBaseV2Plus.rawFulfillRandomWords method). @@ - * @dev associated with the randomness. (It is triggered via a call to - * @dev rawFulfillRandomness, below.) + * @dev associated with the randomness. (It is triggered via a call to + * @dev rawFulfillRandomWords, below.)Also applies to: 130-134
contracts/src/rng/ChainlinkRNG.sol (1)
86-90: Should owner renounce be allowed?If renouncing to address(0) is not intended, add a ZeroAddress check and (optionally) emit an OwnerChanged event for off-chain monitoring.
- function changeOwner(address _owner) external onlyByOwner { - owner = _owner; - } + function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; + // emit OwnerChanged(_owner); // optional + }contracts/src/rng/BlockhashRNG.sol (2)
18-20: Nit: comment is misleading“owner ... can withdraw funds” — this contract has no payable funding/withdrawal logic. Reword to “manages roles” or similar.
- address public owner; // The address that can withdraw funds. + address public owner; // The address that manages roles for this RNG.
107-112: Optional: expose the target block to help consumers detect expiryConsider a view that returns the pre-committed
requestBlockandrequestBlock + 1target. This lets consumers decide to re-request if >256 blocks passed.Example:
function getTargetBlock() external view returns (uint256 blockAtRequest, uint256 blockUsed) { if (requestTimestamp == 0) return (0, 0); return (requestBlock, requestBlock + 1); }contracts/src/arbitration/arbitrables/ArbitrableExample.sol (3)
26-26: Ownership is immutable; consider making it transferable (optional).Using immutable prevents any ownership handover if keys are rotated or the deployer is a factory. Consider a mutable owner + transferOwnership.
Apply within the selected lines:
- address public immutable owner; + address public owner;Additional code (outside the selected lines):
event OwnerTransferred(address indexed previousOwner, address indexed newOwner); error ZeroAddress(); function transferOwnership(address newOwner) external onlyByOwner { if (newOwner == address(0)) revert ZeroAddress(); emit OwnerTransferred(owner, newOwner); owner = newOwner; }Also applies to: 63-63
76-78: Harden changeArbitrator with input validation + event (optional).Avoid setting a zero arbitrator and emit an event for off-chain watchers.
function changeArbitrator(IArbitratorV2 _arbitrator) external onlyByOwner { - arbitrator = _arbitrator; + if (address(_arbitrator) == address(0)) revert ZeroAddress(); + IArbitratorV2 old = arbitrator; + arbitrator = _arbitrator; + emit ArbitratorChanged(old, _arbitrator); }Additional code (outside the selected lines):
event ArbitratorChanged(IArbitratorV2 indexed previousArbitrator, IArbitratorV2 indexed newArbitrator);
84-86: Add zero-address check and event for template registry (optional).Prevents accidental bricking and aids observability.
function changeTemplateRegistry(IDisputeTemplateRegistry _templateRegistry) external onlyByOwner { - templateRegistry = _templateRegistry; + if (address(_templateRegistry) == address(0)) revert ZeroAddress(); + IDisputeTemplateRegistry old = templateRegistry; + templateRegistry = _templateRegistry; + emit TemplateRegistryChanged(old, _templateRegistry); }Additional code (outside the selected lines):
event TemplateRegistryChanged(IDisputeTemplateRegistry indexed previousRegistry, IDisputeTemplateRegistry indexed newRegistry);contracts/scripts/utils/tx-builder.ts (3)
3-3: Rename to safeAddress and avoid hardcoding where possibleThis constant is used as a Safe address (see Lines 15 and 45). Rename for clarity and consider sourcing from env or a config per network to avoid accidental misuse across environments.
-const owner = "0x66e8DE9B42308c6Ca913D1EE041d6F6fD037A57e"; +// Safe address used to create and open the batch in the Safe app. +const safeAddress = process.env.SAFE_ADDRESS ?? "0x66e8DE9B42308c6Ca913D1EE041d6F6fD037A57e";
15-17: Confirm this is a Safe, not an EOAcreatedFromSafeAddress must be a Safe contract. If owner() returns an EOA for some targets, batches will be invalid in the Safe UI.
- createdFromSafeAddress: owner, + createdFromSafeAddress: safeAddress,
45-45: Batch URL assumes a single Safe; parameterize if multiple owners are possibleIf transactions come from contracts with different owner() addresses, this single URL may point to the wrong Safe. Either:
- ensure all targets share the same Safe; or
- accept a safeAddress parameter when writing the batch and build the URL dynamically.
-export const transactionBuilderUrl = `https://app.safe.global/apps/open?safe=arb1:${owner}&appUrl=https%3A%2F%2Fapps-portal.safe.global%2Ftx-builder`; +export const transactionBuilderUrl = `https://app.safe.global/apps/open?safe=arb1:${safeAddress}&appUrl=https%3A%2F%2Fapps-portal.safe.global%2Ftx-builder`;contracts/src/proxy/mock/by-rewrite/UpgradedByRewriteV2.sol (1)
31-33: Prefer custom errors over string reverts for gas and consistencyThe PR moves to custom errors; align _authorizeUpgrade with that.
- require(owner == msg.sender, "No privilege to upgrade"); + if (msg.sender != owner) revert OwnerOnly();Add once in this file (outside the contract or at top-level):
error OwnerOnly();contracts/scripts/utils/execution.ts (3)
27-30: Harden owner() discovery and contract-code checkHandle cases where the target doesn’t implement owner() or where the call fails. Also use a stricter code check.
- const contract = await ethers.getContractAt(governableAbi, tx.to); - const owner = await contract.owner(); - const isContract = (await ethers.provider.getCode(owner)).length > 2; + let isContract = false; + try { + const contract = await ethers.getContractAt(governableAbi, tx.to as string); + const owner = await contract.owner(); + const code = await ethers.provider.getCode(owner); + isContract = code && code !== "0x"; + } catch (err) { + console.warn(`owner() check failed for ${tx.to}:`, err); + }
35-39: EOA ownership flow may send from the wrong accountIf owner() is an EOA you don’t control, sending from the default signer will revert. Consider impersonating the owner (Hardhat) or requiring the active signer to equal owner before sending.
Example (Hardhat):
const ownerAddr = await contract.owner(); await hre.network.provider.request({ method: "hardhat_impersonateAccount", params: [ownerAddr] }); const signer = await ethers.getSigner(ownerAddr); await signer.sendTransaction(request);
55-55: Printed Safe URL may not match the actual batch ownertransactionBuilderUrl uses a static address from tx-builder.ts. If batches include txs for different owner Safes, the link will be wrong. Either group by owner or pass the Safe to writeTransactionBatch.
contracts/src/proxy/mock/UUPSUpgradeableMocks.sol (1)
37-39: Use custom error for consistency (optional)Aligns with the PR’s move away from string reverts.
- require(owner == msg.sender, "No privilege to upgrade"); + if (msg.sender != owner) revert OwnerOnly();Add once in this file:
error OwnerOnly();contracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol (1)
28-30: Optional: switch to custom errors for upgrade authFor gas and consistency with the PR.
- require(owner == msg.sender, "No privilege to upgrade"); + if (msg.sender != owner) revert OwnerOnly();Add once:
error OwnerOnly();contracts/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol (1)
31-33: Use typed errors for upgrade auth to align with repo and save gasReplace string revert with a custom error for consistency with OwnerOnly across the repo.
function _authorizeUpgrade(address) internal view override { - require(owner == msg.sender, "No privilege to upgrade"); + if (owner != msg.sender) revert OwnerOnly(); }Also declare:
error OwnerOnly();contracts/src/arbitration/evidence/EvidenceModule.sol (2)
71-76: Add ZeroAddress error to complement OwnerOnlySince multiple modules check zero-address, add an explicit typed error for consistency.
- error OwnerOnly(); + error OwnerOnly(); + error ZeroAddress();
18-19: Future-proofing: consider a storage gapUpgradeable contract without a __gap reduces flexibility for future vars. Consider reserving slots.
uint256[50] private __gap;contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (2)
49-53: Emit event and guard zero on owner changeEvents help off-chain infra; guard avoids bricking.
- function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + emit OwnerChanged(owner, _owner); + owner = _owner; }Add (outside this hunk):
event OwnerChanged(address indexed previousOwner, address indexed newOwner); error ZeroAddress();
57-59: Emit event and guard zero on core changeSame rationale as owner change.
- function changeCore(IKlerosCore _core) external onlyByOwner { - core = _core; + function changeCore(IKlerosCore _core) external onlyByOwner { + if (address(_core) == address(0)) revert ZeroAddress(); + emit CoreChanged(core, _core); + core = _core; }Add (outside this hunk):
event CoreChanged(IKlerosCore indexed previousCore, IKlerosCore indexed newCore);contracts/src/arbitration/DisputeTemplateRegistry.sol (2)
61-66: Emit event and guard zero on owner changeImproves observability and safety.
- function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + emit OwnerChanged(owner, _owner); + owner = _owner; }Add (outside this hunk):
event OwnerChanged(address indexed previousOwner, address indexed newOwner); error ZeroAddress();
17-19: Consider reserving storage gap for upgrade flexibilityAdd __gap to prevent layout conflicts in future versions.
uint256[50] private __gap;contracts/src/arbitration/PolicyRegistry.sol (2)
70-74: Emit event and guard zero on owner changeImproves safety and on-chain traceability.
- function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + emit OwnerChanged(owner, _owner); + owner = _owner; }Add (outside this hunk):
event OwnerChanged(address indexed previousOwner, address indexed newOwner); error ZeroAddress();
26-28: Consider reserving storage gap for upgradesAdd __gap to keep layout extensible.
uint256[50] private __gap;contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol (4)
54-66: Owner wiring looks correct; consider custom errors for consistency.You use string-based require in onlyOwner while most of the PR migrated to custom errors. Consider switching to a typed error (OwnerOnly) for gas and consistency.
- modifier onlyOwner() { - require(msg.sender == owner, "The caller must be the owner"); + error OwnerOnly(); + modifier onlyOwner() { + if (msg.sender != owner) revert OwnerOnly(); _; }
135-139: Emit event and validate input on owner change.Add zero-address check and an OwnerChanged event.
- function changeOwner(address _owner) external onlyOwner { - owner = _owner; - } + event OwnerChanged(address indexed previousOwner, address indexed newOwner); + function changeOwner(address _owner) external onlyOwner { + require(_owner != address(0), "Owner=0"); + emit OwnerChanged(owner, _owner); + owner = _owner; + }
325-325: Unreliable ETH refund with send; at least surface failures..send returns a bool and can silently fail. Emit an event on failure so users can reclaim via a pull pattern later.
- if (remainingETH != 0) _contributor.send(remainingETH); + if (remainingETH != 0) { + bool ok = _contributor.send(remainingETH); + if (!ok) emit RefundFailed(_contributor, remainingETH); + }Add at top:
+ event RefundFailed(address indexed to, uint256 amount);
379-380: Same .send caveat on rewards payout.Consider surfacing failures with an event or switching to a pull-withdraw model.
- _beneficiary.send(reward); // It is the user responsibility to accept ETH. + bool ok = _beneficiary.send(reward); // Still user's responsibility; we surface failures. + if (!ok) emit RefundFailed(_beneficiary, reward);contracts/test/evidence/index.ts (1)
163-168: Prefer precise assertions over magic numbers.Replace the 93n literal with a computed expected value to decouple the test from hardcoded fee math.
- expect(contributions[1]).to.equal(93n); + const expected = minRequiredDeposit - 0n; // compute from inputs/rounding if needed + expect(contributions[1]).to.equal(expected);contracts/src/arbitration/devtools/KlerosCoreRuler.sol (1)
239-241: Nit: changeOwner doesn’t need payable type.owner is address; drop payable to reduce confusion.
- function changeOwner(address payable _owner) external onlyByOwner { + function changeOwner(address _owner) external onlyByOwner { owner = _owner; }contracts/src/gateway/ForeignGateway.sol (2)
104-110: Use modifier and emit event for owner change.Unify with onlyByOwner style and add OwnerChanged event.
- function changeOwner(address _owner) external { - if (owner != msg.sender) revert OwnerOnly(); - owner = _owner; - } + event OwnerChanged(address indexed previousOwner, address indexed newOwner); + function changeOwner(address _owner) external onlyByOwner { + require(_owner != address(0), "Owner=0"); + emit OwnerChanged(owner, _owner); + owner = _owner; + }
176-183: Optional: replace “Not supported” string with typed error.For consistency with typed errors elsewhere.
- ) external pure override returns (uint256) { - revert("Not supported"); - } + ) external pure override returns (uint256) { + revert NotSupported(); + }Add at errors section:
+ error NotSupported();contracts/src/gateway/HomeGateway.sol (6)
32-32: Harden ownership changes: zero-address guard + event (consider 2-step).
- Prevent setting owner to address(0) and emit an OwnerChanged event for observability. Optionally adopt a two-step transfer to avoid lockouts.
Apply within this range:
- function changeOwner(address _owner) external onlyByOwner { - owner = _owner; - } + function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + emit OwnerChanged(owner, _owner); + owner = _owner; + }Add outside this range:
// Events event OwnerChanged(address indexed previousOwner, address indexed newOwner); // Errors (if not present already) error ZeroAddress();If desired, I can draft a minimal Ownable2Step-style flow. Do you want that?
Also applies to: 96-101
62-82: Validate initialize() inputs.Guard critical addresses during initialization to fail-fast.
function initialize( address _owner, IArbitratorV2 _arbitrator, IVeaInbox _veaInbox, uint256 _foreignChainID, address _foreignGateway, IERC20 _feeToken ) external reinitializer(1) { - owner = _owner; + if ( + _owner == address(0) || + address(_arbitrator) == address(0) || + address(_veaInbox) == address(0) || + _foreignGateway == address(0) + ) revert ZeroAddress(); + owner = _owner; arbitrator = _arbitrator; veaInbox = _veaInbox; foreignChainID = _foreignChainID; foreignGateway = _foreignGateway; feeToken = _feeToken; }
104-124: Emit governance change events and guard zero addresses.For parity with owner changes and better off-chain indexing, emit events and block zero addresses for mutated dependencies.
Suggested minimal diffs:
- function changeArbitrator(IArbitratorV2 _arbitrator) external onlyByOwner { - arbitrator = _arbitrator; - } + function changeArbitrator(IArbitratorV2 _arbitrator) external onlyByOwner { + if (address(_arbitrator) == address(0)) revert ZeroAddress(); + emit ArbitratorChanged(arbitrator, _arbitrator); + arbitrator = _arbitrator; + } - function changeVea(IVeaInbox _veaInbox) external onlyByOwner { - veaInbox = _veaInbox; - } + function changeVea(IVeaInbox _veaInbox) external onlyByOwner { + if (address(_veaInbox) == address(0)) revert ZeroAddress(); + emit VeaInboxChanged(veaInbox, _veaInbox); + veaInbox = _veaInbox; + } - function changeForeignGateway(address _foreignGateway) external onlyByOwner { - foreignGateway = _foreignGateway; - } + function changeForeignGateway(address _foreignGateway) external onlyByOwner { + if (_foreignGateway == address(0)) revert ZeroAddress(); + emit ForeignGatewayChanged(foreignGateway, _foreignGateway); + foreignGateway = _foreignGateway; + } - function changeFeeToken(IERC20 _feeToken) external onlyByOwner { - feeToken = _feeToken; - } + function changeFeeToken(IERC20 _feeToken) external onlyByOwner { + emit FeeTokenChanged(feeToken, _feeToken); + feeToken = _feeToken; // Assumes NATIVE_CURRENCY sentinel is valid. + }Add outside this range:
event ArbitratorChanged(IArbitratorV2 indexed previousArbitrator, IArbitratorV2 indexed newArbitrator); event VeaInboxChanged(IVeaInbox indexed previousVeaInbox, IVeaInbox indexed newVeaInbox); event ForeignGatewayChanged(address indexed previousForeignGateway, address indexed newForeignGateway); event FeeTokenChanged(IERC20 indexed previousFeeToken, IERC20 indexed newFeeToken);Note: If NATIVE_CURRENCY is IERC20(address(0)), keep changeFeeToken permissive; otherwise add a validation error.
132-134: Hashing: prefer abi.encode to avoid dynamic packing ambiguities.extraData is dynamic; abi.encodePacked can collide in corner cases. Switch to abi.encode for the hash preimage.
Outside this range, update construction:
bytes32 disputeHash = keccak256( abi.encode( "createDispute", _params.foreignBlockHash, _params.foreignChainID, _params.foreignArbitrable, _params.foreignDisputeID, _params.choices, _params.extraData ) );Also applies to: 147-147
170-189: ERC20 allowance handling: avoid allowance accumulation and clear after use.increaseAllowance accumulates approvals and can leave residual allowance to arbitrator. Prefer (force) approve exact amount, clear it after use; record fee amount for accounting.
Suggested pattern:
// Before external call: uint256 cur = feeToken.allowance(address(this), address(arbitrator)); if (cur != 0) feeToken.safeApprove(address(arbitrator), 0); // or forceApprove if available feeToken.safeApprove(address(arbitrator), _feeAmount); // External call uint256 disputeID = arbitrator.createDispute(_params.choices, _params.extraData, feeToken, _feeAmount); // After external call: feeToken.safeApprove(address(arbitrator), 0); relayedData.arbitrationCost = _feeAmount;If your SafeERC20 library supports forceApprove, use that instead of the 0→amount pattern. Also consider nonReentrant on both relayCreateDispute overloads for belt-and-suspenders safety. I can wire this up if you want.
238-250: Expand error set for input validation.Add a generic ZeroAddress error to support the proposed guards.
error OwnerOnly(); error ArbitratorOnly(); error FeesPaidInERC20Only(); error FeesPaidInNativeCurrencyOnly(); error ForeignChainIDNotSupported(); error DisputeAlreadyRelayed(); error TransferFailed(); error AllowanceIncreaseFailed(); + error ZeroAddress();contracts/test/proxy/index.ts (2)
82-88: Prefer custom-error assertions where available.If UUPS layer exposes a custom error for unauthorized upgrades, asserting it is less brittle than matching the revert string.
Example:
await expect( implementation.connect(deployer).upgradeToAndCall(newImplementation.target, "0x") ).to.be.revertedWithCustomError(implementation, "NoPrivilegeToUpgrade"); // If presentIf not exposed, ignore.
92-105: Auth test reads well; keep error-style consistent.Unauthorized path: assert custom error if available; Authorized path: event assertion is good.
contracts/src/arbitration/KlerosCoreNeo.sol (2)
90-91: Guard changeJurorNft and emit event.Avoid zero address; emit for off-chain consumers.
- function changeJurorNft(IERC721 _jurorNft) external onlyByOwner { - jurorNft = _jurorNft; - } + function changeJurorNft(IERC721 _jurorNft) external onlyByOwner { + if (address(_jurorNft) == address(0)) revert ZeroAddress(); + emit JurorNftChanged(jurorNft, _jurorNft); + jurorNft = _jurorNft; + }Add outside this range:
event JurorNftChanged(IERC721 indexed previousJurorNft, IERC721 indexed newJurorNft); error ZeroAddress();
96-98: Emit whitelist change events for traceability.Changing arbitrable permissions benefits from events.
event ArbitrableWhitelistChanged(address indexed arbitrable, bool allowed);Then:
- function changeArbitrableWhitelist(address _arbitrable, bool _allowed) external onlyByOwner { - arbitrableWhitelist[_arbitrable] = _allowed; - } + function changeArbitrableWhitelist(address _arbitrable, bool _allowed) external onlyByOwner { + arbitrableWhitelist[_arbitrable] = _allowed; + emit ArbitrableWhitelistChanged(_arbitrable, _allowed); + }contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
144-193: Optional: Enhance robustness of external token calls in_postDrawCheckThe current implementation delegates directly to
balanceOfon the providedtokenGatewithout any error handling or gas limits. While Solidity ^0.8.24 supportstry/catch, note that it still forwards all remaining gas to the callee, so it does not by itself prevent a malicious contract from consuming excessive gas. You may wish to adopt one of the following patterns:• Option A – Try/Catch Wrapper
// Check juror’s token balance if (isERC1155) { - return IBalanceHolderERC1155(tokenGate).balanceOf(_juror, tokenId) > 0; + try IBalanceHolderERC1155(tokenGate).balanceOf(_juror, tokenId) returns (uint256 balance) { + return balance > 0; + } catch { + // On revert or out-of-gas, treat as “no access” + return false; + } } else { - return IBalanceHolder(tokenGate).balanceOf(_juror) > 0; + try IBalanceHolder(tokenGate).balanceOf(_juror) returns (uint256 balance) { + return balance > 0; + } catch { + return false; + } }• Option B – Low-level
staticcallwith gas stipend// Check juror’s token balance bytes4 selector = isERC1155 ? IBalanceHolderERC1155(tokenGate).balanceOf.selector : IBalanceHolder(tokenGate).balanceOf.selector; bytes memory data = isERC1155 ? abi.encodeWithSelector(selector, _juror, tokenId) : abi.encodeWithSelector(selector, _juror); (bool success, bytes memory ret) = tokenGate.staticcall{gas: 100_000}(data); if (!success || ret.length < 32) return false; uint256 balance = abi.decode(ret, (uint256)); return balance > 0;• Interface Validation (optional)
To ensure thetokenGatecontract implements the expected method signatures, you could add an EIP-165 check for ERC-1155 (0xd9b67a26) whenisERC1155 == true. For ERC-20/ERC-721 style gating, no universal interface exists, so a custom registry or trust list may be required.No existing pattern for external-call error handling was found in other dispute-kits or modules, so please review and choose the approach that best fits your desired trade-offs between complexity, gas overhead, and DoS resistance.
contracts/src/arbitration/SortitionModuleNeo.sol (1)
65-71: Add events for configuration changesThe
changeMaxStakePerJurorandchangeMaxTotalStakedfunctions correctly use owner-based access control. However, these configuration changes should emit events for transparency and off-chain monitoring.+event MaxStakePerJurorChanged(uint256 newMaxStakePerJuror); +event MaxTotalStakedChanged(uint256 newMaxTotalStaked); function changeMaxStakePerJuror(uint256 _maxStakePerJuror) external onlyByOwner { maxStakePerJuror = _maxStakePerJuror; + emit MaxStakePerJurorChanged(_maxStakePerJuror); } function changeMaxTotalStaked(uint256 _maxTotalStaked) external onlyByOwner { maxTotalStaked = _maxTotalStaked; + emit MaxTotalStakedChanged(_maxTotalStaked); }contracts/src/arbitration/KlerosCore.sol (1)
23-23: Parameter rename from_governorto_owneris not fully documented.While the parameter is renamed in the function signature and documentation (Line 23), this represents a governance model change that should be clearly communicated. Consider adding a comment explaining the transition from governor-based to owner-based access control for maintainability.
Also applies to: 35-35, 48-48
contracts/src/rng/RNGWithFallback.sol (1)
97-100: Consider edge cases in the fallback mechanism.The fallback to
blockhash(block.number - 1)could potentially return 0 if the block is too old (>256 blocks). While unlikely in practice given the timeout mechanism, it's worth documenting this limitation or adding a check.Consider adding validation:
// If we didn't get a random number and the timeout is exceeded, try the fallback if (randomNumber == 0 && block.timestamp > requestTimestamp + fallbackTimeoutSeconds) { - randomNumber = uint256(blockhash(block.number - 1)); + bytes32 blockHash = blockhash(block.number - 1); + // blockhash returns 0 for blocks older than 256 blocks + if (blockHash != bytes32(0)) { + randomNumber = uint256(blockHash); + } emit RNGFallback(); }contracts/scripts/changeOwner.ts (1)
48-63: Consider adding batch error handling.The current implementation continues execution even if individual contract updates fail. While this provides resilience, consider collecting all errors and reporting them at the end to ensure visibility of any partial failures.
+const errors: Array<{ contract: string; error: string }> = []; const updateOwner = async (contractName: string, contractInstance: any) => { print.info(`Changing owner for ${contractName}`); const spinner = print.spin(`Executing transaction for ${contractName}...`); try { const tx = await contractInstance.changeOwner(newOwner); await tx.wait(); spinner.succeed(`Owner changed for ${contractName}, tx hash: ${tx.hash}`); } catch (error) { if (error instanceof Error) { spinner.fail(`Failed to change owner for ${contractName}: ${error.message}`); + errors.push({ contract: contractName, error: error.message }); } else { spinner.fail(`Failed to change owner for ${contractName}: ${String(error)}`); + errors.push({ contract: contractName, error: String(error) }); } } };Then after all updates:
-print.success("Owner changed successfully"); +if (errors.length > 0) { + print.error(`Owner change completed with ${errors.length} error(s):`); + errors.forEach(({ contract, error }) => { + print.error(` ${contract}: ${error}`); + }); +} else { + print.success("Owner changed successfully for all contracts"); +}contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
196-218: Consider using SafeMath or explicit checks for potential overflow in stake calculationsWhen adding the reward to the current stake at line 215, there's a potential for overflow if
_rewardis very large. While Solidity 0.8+ has built-in overflow protection, it's good practice to document this assumption or add explicit checks.function setStakeReward( address _account, uint96 _courtID, uint256 _reward ) external override onlyByCore returns (bool success) { if (_reward == 0) return true; // No reward to add. uint256 currentStake = _stakeOf(_account, _courtID); if (currentStake == 0) return false; // Juror has been unstaked, don't increase their stake. + // Note: Safe from overflow due to Solidity 0.8+ automatic checks uint256 newStake = currentStake + _reward; _setStake(_account, _courtID, _reward, 0, newStake); return true; }contracts/src/arbitration/KlerosCoreBase.sol (1)
815-831: Ensure dispute kits intentionally differentiate PNK and fee coherenceAll existing dispute kits currently inherit the default implementation in DisputeKitClassicBase, which returns the same coherence value for both PNK and fee rewards:
function getDegreeOfCoherenceReward( uint256 _coreDisputeID, uint256 _coreRoundID, uint256 _voteID, uint256 /* _feePerJuror */, uint256 /* _pnkAtStakePerJuror */ ) external view override returns (uint256 pnkCoherence, uint256 feeCoherence) { uint256 coherence = _getDegreeOfCoherence(_coreDisputeID, _coreRoundID, _voteID); return (coherence, coherence); }– DisputeKitClassicBase.sol (lines 527–537)
No dispute kit overrides this function to apply different coherence calculations for fees versus PNK.
• If future or custom dispute kits require distinct logic (e.g. weighting fees differently), implement an override of getDegreeOfCoherenceReward that returns separate
pnkCoherenceandfeeCoherence.
• Otherwise, add a note in IDisputeKit.sol (or the core developer documentation) clarifying that, by default, both basis-point values are identical unless explicitly overridden.contracts/src/kleros-v1/kleros-liquid/KlerosLiquidToV2Governor.sol (4)
29-29: Adopt custom errors and emitOwnerChangedevent; guard against zero owner.
- Replace bare require with a custom error for gas/clarity.
- Emit an
OwnerChangedevent on ownership transfers and reject zero address.Apply within ranges:
modifier onlyByOwner() { - require(owner == msg.sender); + if (msg.sender != owner) revert OwnerOnly(); _; }-function changeOwner(address _owner) external onlyByOwner { - owner = _owner; -} +function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert InvalidOwner(); + address old = owner; + owner = _owner; + emit OwnerChanged(old, _owner); +}Add these declarations near the top of the contract (outside the ranges):
error OwnerOnly(); error InvalidOwner(); event OwnerChanged(address indexed previousOwner, address indexed newOwner);Also applies to: 39-41, 71-75
48-48: Doc nit: replaceuint(-1)withtype(uint256).maxfor 0.8.x.
66-69: Optionally bubble revert reason from arbitrary call.Current message hides underlying reason. Consider reverting with returned data.
- (bool success, ) = _destination.call{value: _amount}(_data); // solium-disable-line security/no-call-value - require(success, "Call execution failed."); + (bool success, bytes memory ret) = _destination.call{value: _amount}(_data); // solium-disable-line security/no-call-value + if (!success) assembly { + revert(add(ret, 0x20), mload(ret)) + }
93-93: Usebytes("")literal for clarity when passing empty calldata.Minor readability gain and avoids any ambiguity in overload resolution.
- klerosLiquid.executeOwnerProposal(address(this), totalFeesForJurors[0], ""); + klerosLiquid.executeOwnerProposal(address(this), totalFeesForJurors[0], bytes(""));Also applies to: 124-124
contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol (5)
145-146: Strengthen ownership pattern: custom errors, zero-address guard, and event emission.
- Use a custom error in
onlyByOwner.- Reject zero address and emit
OwnerChangedinchangeOwner.- Optional: ensure
_ownerprovided at initialize is non-zero.modifier onlyByOwner() { - require(owner == msg.sender); + if (msg.sender != owner) revert OwnerOnly(); _; }-function changeOwner(address _owner) external onlyByOwner { - owner = _owner; -} +function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert InvalidOwner(); + address old = owner; + owner = _owner; + emit OwnerChanged(old, _owner); +}Optionally, in
initialize:- owner = _owner; + if (_owner == address(0)) revert InvalidOwner(); + owner = _owner;Add (outside ranges) at top-level:
error OwnerOnly(); error InvalidOwner(); event OwnerChanged(address indexed previousOwner, address indexed newOwner);Also applies to: 200-203, 276-280, 211-223, 234-235
271-274: Make_datacalldata and bubble revert reason.Cheaper ABI decoding and better error visibility.
-function executeOwnerProposal(address _destination, uint256 _amount, bytes memory _data) external onlyByOwner { - (bool success, ) = _destination.call{value: _amount}(_data); - require(success, "Unsuccessful call"); +function executeOwnerProposal(address _destination, uint256 _amount, bytes calldata _data) external onlyByOwner { + (bool success, bytes memory ret) = _destination.call{value: _amount}(_data); + if (!success) assembly { + revert(add(ret, 0x20), mload(ret)) + } }
325-333: Prefer calldata for fixed-size arrays in external functions for gas savings.No mutation; use calldata.
- function createSubcourt( + function createSubcourt( uint96 _parent, bool _hiddenVotes, uint256 _minStake, uint256 _alpha, uint256 _feeForJuror, uint256 _jurorsForCourtJump, - uint256[4] memory _timesPerPeriod, + uint256[4] calldata _timesPerPeriod, uint256 _sortitionSumTreeK ) external onlyByOwner {-function changeSubcourtTimesPerPeriod(uint96 _subcourtID, uint256[4] memory _timesPerPeriod) external onlyByOwner { +function changeSubcourtTimesPerPeriod(uint96 _subcourtID, uint256[4] calldata _timesPerPeriod) external onlyByOwner {Also applies to: 395-397
271-274: Minor: consider zero-address guard on_destination.Optional sanity check to prevent accidental burns.
- (bool success, bytes memory ret) = _destination.call{value: _amount}(_data); + require(_destination != address(0), "Invalid destination"); + (bool success, bytes memory ret) = _destination.call{value: _amount}(_data);
334-354: Stake hierarchy update is correct; consider unit tests for edge cases.Add tests for:
- Creating a child with exactly equal minStake to parent.
- Failing when child < parent or when parent lowered below any child.
Want me to draft focused Foundry tests for these branches?
| function initialize(address _owner) external reinitializer(1) { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate nonzero owner in initialize()
Prevent accidental zero-owner deployment.
- function initialize(address _owner) external reinitializer(1) {
- owner = _owner;
+ function initialize(address _owner) external reinitializer(1) {
+ if (_owner == address(0)) revert ZeroAddress();
+ owner = _owner;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function initialize(address _owner) external reinitializer(1) { | |
| owner = _owner; | |
| } | |
| function initialize(address _owner) external reinitializer(1) { | |
| if (_owner == address(0)) revert ZeroAddress(); | |
| owner = _owner; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/DisputeTemplateRegistry.sol around lines 43 to 45,
the initialize() function assigns owner without validating the input; add a
nonzero-address check (e.g. require(_owner != address(0), "owner cannot be
zero")) before setting owner to prevent accidental zero-owner deployment and
keep the reinitializer(1) behavior intact.
| /// @param _owner The owner's address. | ||
| function initialize(address _owner) external reinitializer(1) { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against zero owner on initialize()
Avoid locking the module by initializing to address(0).
- function initialize(address _owner) external reinitializer(1) {
- owner = _owner;
+ function initialize(address _owner) external reinitializer(1) {
+ if (_owner == address(0)) revert ZeroAddress();
+ owner = _owner;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @param _owner The owner's address. | |
| function initialize(address _owner) external reinitializer(1) { | |
| owner = _owner; | |
| } | |
| /// @param _owner The owner's address. | |
| function initialize(address _owner) external reinitializer(1) { | |
| if (_owner == address(0)) revert ZeroAddress(); | |
| owner = _owner; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/evidence/EvidenceModule.sol around lines 39 to 42,
the initialize(address _owner) function currently allows address(0) which can
permanently lock the module; add a guard to revert when _owner is address(0)
(e.g., require(_owner != address(0), "EvidenceModule: invalid owner") or use a
custom error) before assigning owner, ensuring the initializer cannot set a zero
address.
| function changeInitialDepositMultiplier(uint256 _initialDepositMultiplier) external onlyOwner { | ||
| initialDepositMultiplier = _initialDepositMultiplier; | ||
| } | ||
|
|
||
| /// @dev Change the proportion of arbitration fees that must be paid as fee stake by the winner of the previous round. | ||
| /// @param _totalCostMultiplier Multiplier of arbitration fees that must be paid as fee stake. In basis points. | ||
| function changeTotalCostMultiplier(uint256 _totalCostMultiplier) external onlyGovernor { | ||
| function changeTotalCostMultiplier(uint256 _totalCostMultiplier) external onlyOwner { | ||
| totalCostMultiplier = _totalCostMultiplier; | ||
| } | ||
|
|
||
| /// @dev Change the time window within which evidence submissions and removals can be contested. | ||
| /// Ongoing moderations will start using the latest bondTimeout available after calling moderate() again. | ||
| /// @param _bondTimeout Multiplier of arbitration fees that must be paid as fee stake. In basis points. | ||
| function changeBondTimeout(uint256 _bondTimeout) external onlyGovernor { | ||
| function changeBondTimeout(uint256 _bondTimeout) external onlyOwner { | ||
| bondTimeout = _bondTimeout; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against misconfiguration of multipliers/timeouts.
A zero totalCostMultiplier can make totalCost zero; later feeRewards - arbitrationCost (Line 260) will underflow and revert. Add sane bounds.
- function changeInitialDepositMultiplier(uint256 _initialDepositMultiplier) external onlyOwner {
+ function changeInitialDepositMultiplier(uint256 _initialDepositMultiplier) external onlyOwner {
+ require(_initialDepositMultiplier <= type(uint256).max, "bad init mult");
initialDepositMultiplier = _initialDepositMultiplier;
}
- function changeTotalCostMultiplier(uint256 _totalCostMultiplier) external onlyOwner {
+ function changeTotalCostMultiplier(uint256 _totalCostMultiplier) external onlyOwner {
+ // Ensure at least covers arbitration cost so escalation can pay fees.
+ require(_totalCostMultiplier >= MULTIPLIER_DIVISOR, "totalCost < arbitrationCost");
totalCostMultiplier = _totalCostMultiplier;
}
- function changeBondTimeout(uint256 _bondTimeout) external onlyOwner {
+ function changeBondTimeout(uint256 _bondTimeout) external onlyOwner {
+ require(_bondTimeout > 0, "timeout=0");
bondTimeout = _bondTimeout;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function changeInitialDepositMultiplier(uint256 _initialDepositMultiplier) external onlyOwner { | |
| initialDepositMultiplier = _initialDepositMultiplier; | |
| } | |
| /// @dev Change the proportion of arbitration fees that must be paid as fee stake by the winner of the previous round. | |
| /// @param _totalCostMultiplier Multiplier of arbitration fees that must be paid as fee stake. In basis points. | |
| function changeTotalCostMultiplier(uint256 _totalCostMultiplier) external onlyGovernor { | |
| function changeTotalCostMultiplier(uint256 _totalCostMultiplier) external onlyOwner { | |
| totalCostMultiplier = _totalCostMultiplier; | |
| } | |
| /// @dev Change the time window within which evidence submissions and removals can be contested. | |
| /// Ongoing moderations will start using the latest bondTimeout available after calling moderate() again. | |
| /// @param _bondTimeout Multiplier of arbitration fees that must be paid as fee stake. In basis points. | |
| function changeBondTimeout(uint256 _bondTimeout) external onlyGovernor { | |
| function changeBondTimeout(uint256 _bondTimeout) external onlyOwner { | |
| bondTimeout = _bondTimeout; | |
| } | |
| function changeInitialDepositMultiplier(uint256 _initialDepositMultiplier) external onlyOwner { | |
| // (Note: this check is effectively a no-op since uint256 is always ≤ type(uint256).max) | |
| require(_initialDepositMultiplier <= type(uint256).max, "bad init mult"); | |
| initialDepositMultiplier = _initialDepositMultiplier; | |
| } | |
| /// @dev Change the proportion of arbitration fees that must be paid as fee stake by the winner of the previous round. | |
| /// @param _totalCostMultiplier Multiplier of arbitration fees that must be paid as fee stake. In basis points. | |
| function changeTotalCostMultiplier(uint256 _totalCostMultiplier) external onlyOwner { | |
| // Ensure at least covers arbitration cost so escalation can pay fees. | |
| require(_totalCostMultiplier >= MULTIPLIER_DIVISOR, "totalCost < arbitrationCost"); | |
| totalCostMultiplier = _totalCostMultiplier; | |
| } | |
| /// @dev Change the time window within which evidence submissions and removals can be contested. | |
| /// Ongoing moderations will start using the latest bondTimeout available after calling moderate() again. | |
| /// @param _bondTimeout Timeout in seconds. | |
| function changeBondTimeout(uint256 _bondTimeout) external onlyOwner { | |
| require(_bondTimeout > 0, "timeout=0"); | |
| bondTimeout = _bondTimeout; | |
| } |
| modifier onlyByOwner() { | ||
| if (address(this) != msg.sender) revert OwnerOnly(); | ||
| _; | ||
| } |
There was a problem hiding this comment.
Critical: onlyByOwner currently blocks governance (checks address(this)).
This makes owner-only functions unusable externally and there’s no owner storage. Introduce an owner variable, set it in constructor, and fix the guard.
contract KlerosGovernor is IArbitrableV2 {
using SafeSend for address payable;
+ address public owner;
@@
- modifier onlyByOwner() {
- if (address(this) != msg.sender) revert OwnerOnly();
+ modifier onlyByOwner() {
+ if (owner != msg.sender) revert OwnerOnly();
_;
}Also extend the constructor to accept _owner and assign it:
- constructor(
+ constructor(
IArbitratorV2 _arbitrator,
bytes memory _arbitratorExtraData,
string memory _templateData,
string memory _templateDataMappings,
uint256 _submissionBaseDeposit,
uint256 _submissionTimeout,
uint256 _executionTimeout,
uint256 _withdrawTimeout,
- address _wNative
+ address _wNative,
+ address _owner
) {
+ owner = _owner;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| modifier onlyByOwner() { | |
| if (address(this) != msg.sender) revert OwnerOnly(); | |
| _; | |
| } | |
| contract KlerosGovernor is IArbitrableV2 { | |
| using SafeSend for address payable; | |
| address public owner; | |
| modifier onlyByOwner() { | |
| if (owner != msg.sender) revert OwnerOnly(); | |
| _; | |
| } | |
| constructor( | |
| IArbitratorV2 _arbitrator, | |
| bytes memory _arbitratorExtraData, | |
| string memory _templateData, | |
| string memory _templateDataMappings, | |
| uint256 _submissionBaseDeposit, | |
| uint256 _submissionTimeout, | |
| uint256 _executionTimeout, | |
| uint256 _withdrawTimeout, | |
| address _wNative, | |
| address _owner | |
| ) { | |
| owner = _owner; | |
| // … rest of constructor … | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosGovernor.sol around lines 83-86 the
onlyByOwner modifier incorrectly compares msg.sender to address(this) and there
is no owner storage; add an address owner state variable (public or private as
desired), extend the contract constructor to accept an address _owner parameter
and assign owner = _owner (or msg.sender fallback if needed), and update the
onlyByOwner modifier to revert unless msg.sender == owner (use the existing
OwnerOnly() revert). Ensure correct visibility and initialization in
constructor.
| function initialize(address _owner) external reinitializer(1) { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate nonzero owner in initialize()
Avoid zero-owner deployments that lock governance.
- function initialize(address _owner) external reinitializer(1) {
- owner = _owner;
+ function initialize(address _owner) external reinitializer(1) {
+ if (_owner == address(0)) revert ZeroAddress();
+ owner = _owner;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function initialize(address _owner) external reinitializer(1) { | |
| owner = _owner; | |
| } | |
| function initialize(address _owner) external reinitializer(1) { | |
| if (_owner == address(0)) revert ZeroAddress(); | |
| owner = _owner; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/PolicyRegistry.sol around lines 50 to 52, the
initialize(address _owner) function currently sets owner without validation
which can allow the zero address and lock governance; update the initializer to
require(_owner != address(0), "PolicyRegistry: owner is zero") (or revert with
an equivalent error) before assigning owner, preserving the reinitializer(1)
modifier and behavior otherwise.
| function changeOwner(address _owner) external onlyByOwner { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add event emission for ownership transfer.
The ownership change should emit an event for transparency and off-chain monitoring.
function changeOwner(address _owner) external onlyByOwner {
+ address previousOwner = owner;
owner = _owner;
+ emit OwnerChanged(previousOwner, _owner);
}Add the event declaration around line 28:
event RequestSent(uint256 indexed requestId);
+event OwnerChanged(address indexed previousOwner, address indexed newOwner);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function changeOwner(address _owner) external onlyByOwner { | |
| owner = _owner; | |
| } | |
| // --- around line 28 --- | |
| // existing event: | |
| event RequestSent(uint256 indexed requestId); | |
| // add this new event: | |
| event OwnerChanged(address indexed previousOwner, address indexed newOwner); | |
| // --- around lines 70–72 --- | |
| function changeOwner(address _owner) external onlyByOwner { | |
| // capture the old owner before updating | |
| address previousOwner = owner; | |
| // perform the ownership transfer | |
| owner = _owner; | |
| // emit an event so off-chain services can track the change | |
| emit OwnerChanged(previousOwner, _owner); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/rng/RandomizerRNG.sol around lines 70 to 72, the changeOwner
function updates owner without emitting an event; declare an
OwnershipTransferred (or OwnerChanged) event near line 28 (e.g., event
OwnershipTransferred(address indexed previousOwner, address indexed newOwner))
and modify changeOwner to emit that event after setting owner (emit
OwnershipTransferred(prevOwner, _owner)); capture the previous owner in a local
variable before assignment and ensure the event uses indexed parameters for
off-chain filtering.
| function changeConsumer(address _consumer) external onlyByOwner { | ||
| consumer = _consumer; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add event emission for consumer change.
Similar to ownership changes, consumer changes should be logged for transparency.
function changeConsumer(address _consumer) external onlyByOwner {
+ address previousConsumer = consumer;
consumer = _consumer;
+ emit ConsumerChanged(previousConsumer, _consumer);
}Add the event declaration around line 29:
+event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function changeConsumer(address _consumer) external onlyByOwner { | |
| consumer = _consumer; | |
| } | |
| // — around line 29, add this event declaration — | |
| event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer); | |
| function changeConsumer(address _consumer) external onlyByOwner { | |
| address previousConsumer = consumer; | |
| consumer = _consumer; | |
| emit ConsumerChanged(previousConsumer, _consumer); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/rng/RandomizerRNG.sol around lines 29 and 76-78, add an event
declaration (e.g., event ConsumerChanged(address indexed previousConsumer,
address indexed newConsumer);) near line 29, then modify the changeConsumer
function to emit ConsumerChanged(consumer, _consumer) immediately after updating
consumer (or emit before assignment if you prefer previous value captured),
ensuring both old and new consumer addresses are included and the event is
indexed for easier filtering.
| constructor(IERC20 _token) { | ||
| token = _token; | ||
| governor = msg.sender; | ||
| owner = msg.sender; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate token address in constructor.
Prevents deployment with zero token address, which would brick transfers.
constructor(IERC20 _token) {
- token = _token;
+ require(address(_token) != address(0), "Token address cannot be zero");
+ token = _token;
owner = msg.sender;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(IERC20 _token) { | |
| token = _token; | |
| governor = msg.sender; | |
| owner = msg.sender; | |
| } | |
| constructor(IERC20 _token) { | |
| require(address(_token) != address(0), "Token address cannot be zero"); | |
| token = _token; | |
| owner = msg.sender; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/token/Faucet.sol around lines 30 to 33, the constructor does
not validate the provided token address; add a check to revert if _token is the
zero address (use require(_token != IERC20(address(0)), "Faucet: token is zero
address") or equivalent) before assigning token and owner to prevent deploying a
faucet with a null token pointer that would break transfers.
| function changeOwner(address _owner) public onlyByOwner { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
Prevent governance lockout: disallow owner = address(0).
Setting owner to zero makes the contract permanently ungovernable.
function changeOwner(address _owner) public onlyByOwner {
- owner = _owner;
+ require(_owner != address(0), "Owner cannot be zero address");
+ owner = _owner;
}Optional outside this range:
event OwnerChanged(address indexed previousOwner, address indexed newOwner);…and emit it in changeOwner.
🤖 Prompt for AI Agents
In contracts/src/token/Faucet.sol around lines 39 to 41, changeOwner currently
allows setting owner to address(0) which would permanently lock governance;
update the function to revert if _owner == address(0) (e.g., require(_owner !=
address(0), "owner zero not allowed")) and then assign owner = _owner. Also add
and emit an OwnerChanged(previousOwner, _owner) event when the owner is updated
to provide on-chain history.
| function withdraw() public onlyByOwner { | ||
| token.transfer(owner, token.balanceOf(address(this))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use SafeERC20 to avoid non-compliant ERC-20 edge cases.
Handles tokens that don’t return bool and standardizes failures.
function withdraw() public onlyByOwner {
- token.transfer(owner, token.balanceOf(address(this)));
+ token.safeTransfer(owner, token.balanceOf(address(this)));
}Add outside this range:
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
contract Faucet {
using SafeERC20 for IERC20;
// ...
}🤖 Prompt for AI Agents
In contracts/src/token/Faucet.sol around lines 47 to 49, the contract uses
token.transfer(...) which can fail with non-compliant ERC-20 tokens; import
OpenZeppelin's SafeERC20 and use it to perform safe transfers. Add the import
statement for "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol" at the
top, add "using SafeERC20 for IERC20;" inside the contract, ensure the token
variable is an IERC20 (or cast it to IERC20), and replace token.transfer(...)
with token.safeTransfer(owner, token.balanceOf(address(this))) so failures are
handled consistently.
… was over 3000 lines
Foundry tests file split into several files
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
contracts/test/foundry/KlerosCore_TestBase.sol (1)
195-196: Optimize supportedDK array allocationThe
supportedDKarray is created with only one element. Consider using a more memory-efficient approach or making this a constant.-uint256[] memory supportedDK = new uint256[](1); -supportedDK[0] = DISPUTE_KIT_CLASSIC; +uint256[] memory supportedDK = new uint256[](1); +supportedDK[0] = DISPUTE_KIT_CLASSIC; +// Consider extracting as a helper: +// function _getSingleDisputeKitArray(uint256 kitId) internal pure returns (uint256[] memory)contracts/test/foundry/KlerosCore_Drawing.t.sol (2)
43-48: Add assertion messages for better test diagnosticsThe vote info assertions could benefit from more descriptive error messages to aid debugging when tests fail.
-assertEq(account, staker1, "Wrong drawn account"); -assertEq(commit, bytes32(0), "Commit should be empty"); -assertEq(choice, 0, "Choice should be empty"); -assertEq(voted, false, "Voted should be false"); +assertEq(account, staker1, string.concat("Wrong drawn account for voteID ", vm.toString(i))); +assertEq(commit, bytes32(0), string.concat("Commit should be empty for voteID ", vm.toString(i))); +assertEq(choice, 0, string.concat("Choice should be empty for voteID ", vm.toString(i))); +assertEq(voted, false, string.concat("Voted should be false for voteID ", vm.toString(i)));
79-90: Magic numbers in court creation should use named constantsThe court creation uses several magic numbers that should be extracted to improve readability and maintainability.
Consider extracting these values as named constants at the test contract level:
+uint256 constant CHILD_COURT_MIN_STAKE = 1000; +uint256 constant CHILD_COURT_ALPHA = 10000; +uint256 constant CHILD_COURT_FEE = 0.03 ether; +uint256 constant CHILD_COURT_JURORS_FOR_JUMP = 50; core.createCourt( GENERAL_COURT, true, // Hidden votes - 1000, // min stake - 10000, // alpha - 0.03 ether, // fee for juror - 50, // jurors for jump + CHILD_COURT_MIN_STAKE, + CHILD_COURT_ALPHA, + CHILD_COURT_FEE, + CHILD_COURT_JURORS_FOR_JUMP,contracts/test/foundry/KlerosCore_Execution.t.sol (1)
99-111: Consider extracting complex event emission patterns into helper functionsThe repeated event emission patterns with similar parameters could be simplified with helper functions to improve test readability.
Consider creating helper functions:
function _expectStakeLocked(address juror, uint256 amount, bool isUnlock) internal { vm.expectEmit(true, true, true, true); emit SortitionModuleBase.StakeLocked(juror, amount, isUnlock); } function _expectTokenAndETHShift( address juror, uint256 disputeID, uint256 roundID, uint256 coherence, int256 pnkAmount, uint256 ethAmount, IERC20 token ) internal { vm.expectEmit(true, true, true, true); emit KlerosCoreBase.TokenAndETHShift(juror, disputeID, roundID, coherence, pnkAmount, ethAmount, token); }contracts/test/foundry/KlerosCore_Staking.t.sol (1)
147-170: Court creation loop could hit gas limitsCreating courts in a loop without checking gas consumption could cause issues in larger test scenarios.
Consider adding a maximum court limit check or measuring gas usage:
+uint256 gasStart = gasleft(); for (uint96 i = GENERAL_COURT; i <= 4; i++) { vm.prank(owner); core.createCourt( // ... parameters ); vm.prank(staker1); core.setStake(i, 2000); } +uint256 gasUsed = gasStart - gasleft(); +assertLt(gasUsed, 5_000_000, "Gas usage exceeded expected threshold");contracts/test/foundry/KlerosCore_Disputes.t.sol (2)
22-22: Improve readability by extracting encoded extra dataThe extra data encoding could be more readable with named variables.
-bytes memory newExtraData = abi.encodePacked(uint256(newCourtID), newNbJurors, newDkID); +bytes memory newExtraData = abi.encodePacked( + uint256(newCourtID), // Court ID + newNbJurors, // Number of jurors + newDkID // Dispute Kit ID +);
132-134: Missing test coverage for successful token transfer caseThe test verifies the
TransferFailederror case but doesn't fully validate the successful path's internal mechanics beyond the final state.Consider adding assertions for intermediate states during successful token dispute creation:
// Before dispute creation uint256 coreBalanceBefore = feeToken.balanceOf(address(core)); uint256 disputerBalanceBefore = feeToken.balanceOf(disputer); vm.prank(disputer); arbitrable.createDispute("Action", 0.18 ether); // Verify transfer occurred assertEq(feeToken.balanceOf(address(core)), coreBalanceBefore + 0.18 ether, "Core should receive fees"); assertEq(feeToken.balanceOf(disputer), disputerBalanceBefore - 0.18 ether, "Disputer should pay fees");contracts/test/foundry/KlerosCore_RNG.t.sol (2)
28-33: Consider using a more realistic minimum amount for staking.While 20000 works for testing, consider using a constant or a more realistic value that aligns with production expectations to make tests more representative.
79-84: Consider extracting common setup into a helper function.Lines 79-84 duplicate the exact same setup from lines 14-17 and 53-57. Consider creating a helper function to reduce duplication:
+function setupRNGFallback() private returns (RNGWithFallback, RNGMock) { + uint256 fallbackTimeout = 100; + RNGMock rngMock = new RNGMock(); + RNGWithFallback rngFallback = new RNGWithFallback(msg.sender, address(sortitionModule), fallbackTimeout, rngMock); + return (rngFallback, rngMock); +}contracts/test/foundry/KlerosCore_Initialization.t.sol (1)
26-32: Consider adding assertions for dispute kit enablement status.While the test validates court parameters, consider also asserting that the dispute kits are properly enabled for each court after initialization.
📜 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 (10)
contracts/test/foundry/KlerosCore_Appeals.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Disputes.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Drawing.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Governance.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Initialization.t.sol(1 hunks)contracts/test/foundry/KlerosCore_RNG.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Staking.t.sol(1 hunks)contracts/test/foundry/KlerosCore_TestBase.sol(1 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(1 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). (6)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
🔇 Additional comments (29)
contracts/test/foundry/KlerosCore_TestBase.sol (2)
127-134: RNG reference inconsistency in initializationThe
initDataSmincludesrngas a parameter, butrngis created before the sortitionModule proxy. This creates a circular dependency where RNG needs sortitionModule's address, but sortitionModule needs RNG's address during initialization.The initialization order should be restructured to avoid this circular dependency. Consider deploying RNG with a placeholder address and updating it after sortitionModule is deployed, or using a two-step initialization pattern.
⛔ Skipped due to learnings
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.
113-113: Ignore this issue The RNG’s consumer is correctly updated viarng.changeConsumer(address(sortitionModule))immediately after deploying the sortition module, so passingaddress(0)in the constructor has no adverse effect.Likely an incorrect or invalid review comment.
contracts/test/foundry/KlerosCore_Execution.t.sol (1)
74-75: Confirm coherence reward uses ONE_BASIS_POINT constant
ONE_BASIS_POINT is defined as 10000; ensure getDegreeOfCoherenceReward’s calculations reference this constant rather than any hardcoded “10000” literals.contracts/test/foundry/KlerosCore_RNG.t.sol (8)
1-9: LGTM! Test file structure and imports are appropriate.The test file follows standard Solidity conventions with proper SPDX license identifier, pragma version, and necessary imports for testing RNG functionality.
13-22: LGTM! RNG fallback initialization assertions are comprehensive.The test properly validates all critical initialization parameters including owner, consumer, RNG, fallback timeout, and initial request timestamp state.
34-38: LGTM! Proper phase transition and timestamp validation.Good test coverage for the RNG request flow, correctly asserting that the request timestamp is set after transitioning to the Generating phase.
40-50: LGTM! Excellent fallback mechanism testing.The test properly validates the fallback behavior by:
- Advancing time past the fallback timeout
- Rolling forward blocks to verify correct blockhash selection
- Checking RNGFallback event emission
- Validating the fallback random number matches the expected blockhash
This provides thorough coverage of the fallback functionality.
52-77: LGTM! Happy path test validates normal RNG flow.The test correctly exercises the standard RNG flow where the mock RNG provides a value before timeout, ensuring no fallback is triggered.
85-92: LGTM! Proper access control validation for consumer-only functions.The test correctly validates that only the designated consumer can call
requestRandomnessandreceiveRandomness.
93-110: LGTM! Comprehensive owner access control testing.The test properly validates owner-only access for
changeOwnerandchangeConsumerfunctions, including ownership transfer scenarios.
111-120: LGTM! Fallback timeout change properly tested with event emission.The test validates both access control and event emission for the
changeFallbackTimeoutfunction.contracts/test/foundry/KlerosCore_Initialization.t.sol (3)
18-25: LGTM! Core initialization parameters properly validated.The test comprehensively validates key initialization parameters including owner, guardian, token addresses, and dispute kit configuration.
49-57: LGTM! Token initialization and distribution properly validated.The test correctly validates PNK token properties and initial balance distribution to stakers.
82-169: LGTM! Comprehensive event emission testing during initialization.The test thoroughly validates all critical events emitted during initialization including DisputeKitCreated, CourtCreated, and DisputeKitEnabled. The use of fresh proxies and mocks ensures isolated testing.
contracts/test/foundry/KlerosCore_Governance.t.sol (5)
15-29: LGTM! Pause functionality and access control properly tested.The test validates both guardian and owner can pause, and properly checks the WhenNotPausedOnly modifier.
49-64: LGTM! Owner proposal execution properly tested.Good coverage of the executeOwnerProposal function including access control and error conditions.
126-185: Comprehensive court creation validation with excellent error coverage.The test thoroughly validates all error conditions including:
- MinStakeLowerThanParentCourt
- UnsupportedDisputeKit
- InvalidForkingCourtAsParent
- WrongDisputeKitIndex
- MustSupportDisputeKitClassic
This provides excellent coverage of edge cases.
363-402: LGTM! Dispute kit enablement properly tested.The test validates all aspects of enabling/disabling dispute kits including access control, boundary checks, and the restriction on disabling the classic dispute kit.
439-471: LGTM! ExtraData parsing thoroughly tested.Good coverage of standard values, malformed data fallback, and custom values with out-of-bounds handling.
contracts/test/foundry/KlerosCore_Voting.t.sol (5)
14-37: LGTM! Commit phase setup properly validates hidden voting flow.The test correctly sets up hidden voting, stakes jurors, creates disputes, and advances through phases.
38-63: LGTM! Commit validation with proper error handling.Good test coverage for NotCommitPeriod error, period transitions, and EmptyCommit validation.
100-123: LGTM! Reveal phase properly validates commitment verification.The test correctly validates HashDoesNotMatchHiddenVoteCommitment errors for both wrong choice and wrong salt, then successfully reveals votes.
161-270: Excellent comprehensive voting test coverage.The test thoroughly validates:
- Period restrictions (NotVotePeriod)
- Drawing completion requirements (DisputeStillDrawing)
- Vote ownership (JurorHasToOwnTheVote)
- Choice validation (ChoiceOutOfBounds)
- Duplicate vote prevention (VoteAlreadyCast)
- Round information updates
- Tie detection and resolution
This provides excellent coverage of the voting mechanism.
387-484: Excellent multi-dispute kit scenario testing.The test comprehensively validates:
- Multiple dispute kit deployment and enablement
- Correct dispute kit selection per dispute
- Mapping between core and local dispute IDs
- Prevention of voting through inactive dispute kits
- Round information per dispute kit
This is critical for validating the multi-DK architecture.
contracts/test/foundry/KlerosCore_Appeals.t.sol (5)
13-103: LGTM! Comprehensive appeal funding test with excellent coverage.The test thoroughly validates:
- Appeal period timing
- Access control (DisputeKitOnly)
- Funding requirements (AppealFeesNotEnough)
- Choice validation (ChoiceOutOfBounds)
- Overpayment handling and reimbursement
- Funded choices tracking
- Duplicate funding prevention
105-147: LGTM! Appeal timeout validation properly implemented.The test correctly validates appeal period timing restrictions for both losing and winning choices.
149-209: LGTM! Full funding scenario without court/DK switch properly tested.The test validates complete appeal funding flow including:
- Balance distributions to DK and core
- Round creation with correct parameters
- Period transitions
- Drawing in the new round
211-326: Excellent court and dispute kit jumping test coverage.This is a critical test that validates:
- Court jumping triggered by appeal
- Dispute kit switching
- CourtJump and DisputeKitJump events
- Dispute creation in new DK
- Prevention of drawing in jumped-from DK
- Drawing in the new round with correct DK
This provides essential coverage for the multi-court/DK architecture.
328-360: LGTM! Quick pass period properly validates early transition.The test correctly validates that the period can advance to execution without waiting for the full appeal period when no appeals are made.
| sortitionModule.withdrawLeftoverPNK(staker1); | ||
|
|
There was a problem hiding this comment.
Incorrect revert expectation with wrong caller
The test expects SortitionModuleOnly error when called by owner, but the comment and context suggest this should be testing that only sortitionModule can call the function. The test setup might be incorrect.
The test should verify that the sortitionModule CAN call this function, not just that others cannot:
vm.expectRevert(SortitionModuleBase.NotEligibleForWithdrawal.selector);
sortitionModule.withdrawLeftoverPNK(staker1);
+// After execution, verify sortitionModule can withdraw
vm.expectEmit(true, true, true, true);
emit SortitionModuleBase.LeftoverPNK(staker1, 1000);
core.execute(disputeID, 0, 6);
+
+// Now the withdrawal should work
+sortitionModule.withdrawLeftoverPNK(staker1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sortitionModule.withdrawLeftoverPNK(staker1); | |
| // Expect withdrawal by non-eligible caller to revert | |
| vm.expectRevert(SortitionModuleBase.NotEligibleForWithdrawal.selector); | |
| sortitionModule.withdrawLeftoverPNK(staker1); | |
| // After execution, verify sortitionModule can withdraw | |
| vm.expectEmit(true, true, true, true); | |
| emit SortitionModuleBase.LeftoverPNK(staker1, 1000); | |
| core.execute(disputeID, 0, 6); | |
| // Now the withdrawal should work | |
| sortitionModule.withdrawLeftoverPNK(staker1); |
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Execution.t.sol around lines 392-393, the
test incorrectly expects a SortitionModuleOnly revert when calling
withdrawLeftoverPNK as the owner; instead, update the test so the success path
is exercised by calling withdrawLeftoverPNK from the registered sortitionModule
address (simulate or set msg.sender to sortitionModule) and assert expected
state changes/events, and add/keep a separate negative assertion that calling
withdrawLeftoverPNK from a non-sortitionModule (e.g., owner or random account)
reverts with SortitionModuleOnly.
| vm.prank(address(core)); | ||
| pinakion.transfer(staker1, 1); // Manually send 1 token to make the withdrawal fail | ||
|
|
||
| vm.expectRevert(KlerosCoreBase.UnstakingTransferFailed.selector); | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 0); | ||
|
|
||
| vm.prank(address(staker1)); | ||
| pinakion.transfer(address(core), 1); // Manually give the token back | ||
| vm.prank(staker1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Token transfer simulation has potential precision issues
Manually transferring tokens to simulate failure conditions could lead to unexpected states. The test transfers 1 wei but doesn't verify the exact balance states before/after.
Add balance assertions and use a more robust approach:
+uint256 coreBalanceBefore = pinakion.balanceOf(address(core));
+uint256 stakerBalanceBefore = pinakion.balanceOf(staker1);
+
vm.prank(address(core));
pinakion.transfer(staker1, 1); // Manually send 1 token to make the withdrawal fail
+assertEq(pinakion.balanceOf(address(core)), coreBalanceBefore - 1, "Core balance should decrease by 1");
+assertEq(pinakion.balanceOf(staker1), stakerBalanceBefore + 1, "Staker balance should increase by 1");
vm.expectRevert(KlerosCoreBase.UnstakingTransferFailed.selector);
vm.prank(staker1);
core.setStake(GENERAL_COURT, 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vm.prank(address(core)); | |
| pinakion.transfer(staker1, 1); // Manually send 1 token to make the withdrawal fail | |
| vm.expectRevert(KlerosCoreBase.UnstakingTransferFailed.selector); | |
| vm.prank(staker1); | |
| core.setStake(GENERAL_COURT, 0); | |
| vm.prank(address(staker1)); | |
| pinakion.transfer(address(core), 1); // Manually give the token back | |
| vm.prank(staker1); | |
| // Record balances before simulating the failure-inducing transfer | |
| uint256 coreBalanceBefore = pinakion.balanceOf(address(core)); | |
| uint256 stakerBalanceBefore = pinakion.balanceOf(staker1); | |
| vm.prank(address(core)); | |
| pinakion.transfer(staker1, 1); // Manually send 1 token to make the withdrawal fail | |
| // Assert that balances moved as expected | |
| assertEq(pinakion.balanceOf(address(core)), coreBalanceBefore - 1, "Core balance should decrease by 1"); | |
| assertEq(pinakion.balanceOf(staker1), stakerBalanceBefore + 1, "Staker balance should increase by 1"); | |
| vm.expectRevert(KlerosCoreBase.UnstakingTransferFailed.selector); | |
| vm.prank(staker1); | |
| core.setStake(GENERAL_COURT, 0); | |
| vm.prank(address(staker1)); | |
| pinakion.transfer(address(core), 1); // Manually give the token back | |
| vm.prank(staker1); |
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Staking.t.sol around lines 111 to 120, the
manual pinakion.transfer(…, 1) usage to simulate a failed unstaking can
introduce precision/implicit-balance assumptions; add explicit balance checks
before and after each transfer (assert pinakion.balanceOf(address(core)) and
pinakion.balanceOf(staker1) equal expected values) and use clearly named
constants for transfer amounts (e.g., ONE_WEI) or transfer the exact stake
amount used in the test to avoid underflow/precision mistakes; ensure you
vm.prank from the correct addresses for each transfer and restore the original
balances after the failure simulation so the test state is deterministic.
| for (uint i = 2; i <= sortitionModule.delayedStakeWriteIndex(); i++) { | ||
| (account, courtID, stake, alreadyTransferred) = sortitionModule.delayedStakes(i); | ||
|
|
||
| assertEq(account, address(0), "Wrong staker account after delayed stake deletion"); | ||
| assertEq(courtID, 0, "Court id should be nullified"); | ||
| assertEq(stake, 0, "No amount to stake"); | ||
| assertEq(alreadyTransferred, false, "Should be false"); | ||
| } |
There was a problem hiding this comment.
Loop boundary issue in delayed stake verification
The loop starts at index 2 but should likely start at index 1 to check all delayed stakes.
-for (uint i = 2; i <= sortitionModule.delayedStakeWriteIndex(); i++) {
+for (uint i = 1; i <= sortitionModule.delayedStakeWriteIndex(); i++) {
(account, courtID, stake, alreadyTransferred) = sortitionModule.delayedStakes(i);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (uint i = 2; i <= sortitionModule.delayedStakeWriteIndex(); i++) { | |
| (account, courtID, stake, alreadyTransferred) = sortitionModule.delayedStakes(i); | |
| assertEq(account, address(0), "Wrong staker account after delayed stake deletion"); | |
| assertEq(courtID, 0, "Court id should be nullified"); | |
| assertEq(stake, 0, "No amount to stake"); | |
| assertEq(alreadyTransferred, false, "Should be false"); | |
| } | |
| for (uint i = 1; i <= sortitionModule.delayedStakeWriteIndex(); i++) { | |
| (account, courtID, stake, alreadyTransferred) = sortitionModule.delayedStakes(i); | |
| assertEq(account, address(0), "Wrong staker account after delayed stake deletion"); | |
| assertEq(courtID, 0, "Court id should be nullified"); | |
| assertEq(stake, 0, "No amount to stake"); | |
| assertEq(alreadyTransferred, false, "Should be false"); | |
| } |
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Staking.t.sol around lines 396 to 403, the
for-loop that verifies delayed stakes begins at index 2 and therefore skips the
first delayed stake; change the loop start from 2 to 1 (for (uint i = 1; i <=
sortitionModule.delayedStakeWriteIndex(); i++)) so it iterates over all delayed
stakes and validates each entry; keep the existing assertions inside the loop
unchanged.
PR-Codex overview
This PR focuses on refactoring the governance structure in smart contracts, replacing the term
governorwithowneracross multiple files. It also introduces a newIRNGinterface for random number generation and updates various contract initializers and access control mechanisms.Detailed summary
governorwithownerin multiple contracts for consistent governance terminology.IRNGinterface for random number generation.ownerinstead ofgovernor.onlyByGovernortoonlyByOwner.OwnerOnlyerror.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Dependency & Tooling