RNG Fallback #2054
RNG Fallback #2054
Conversation
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughReplaces the old RNG ABI with a new IRNG interface, removes block-based lookahead/timing, adds RNGWithFallback (primary+fallback) and related tests, updates RNG implementations and SortitionModule(s) to a parameterless consumer-based API, and rewires deployments/tests accordingly while deleting some legacy deploy scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant SortitionModule
participant RNGWithFallback
participant PrimaryRNG as Primary RNG
participant Chain as Blockchain
SortitionModule->>RNGWithFallback: requestRandomness()
RNGWithFallback->>PrimaryRNG: requestRandomness()
Note right of RNGWithFallback #e6f7ff: record requestTimestamp
SortitionModule->>RNGWithFallback: receiveRandomness()
RNGWithFallback->>PrimaryRNG: receiveRandomness()
alt Primary RNG returns non-zero
PrimaryRNG-->>RNGWithFallback: randomNumber
RNGWithFallback-->>SortitionModule: randomNumber
else Timeout elapsed
RNGWithFallback->>Chain: derive fallback := blockhash(block.number-1)
RNGWithFallback-->>SortitionModule: fallback randomNumber (emit RNGFallback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contracts/test/foundry/KlerosCore.t.sol (1)
276-278: Consider using consistent rngLookahead values across tests.The
test_initialize_eventsfunction usesrngLookahead = 20while the mainsetUpuses600. Consider using the same value for consistency, or add a comment explaining why a different value is needed for event testing.- rngLookahead = 20; + rngLookahead = 600; // Use consistent timing across tests rng = new BlockHashRNG(msg.sender, address(sortitionModule), rngLookahead);contracts/test/rng/index.ts (1)
38-44: Consider using different addresses for governor and consumer in tests.While using the same address for both roles works for testing, consider using different addresses to better simulate production scenarios and ensure role separation is properly tested.
- const [deployer] = await ethers.getSigners(); + const [deployer, consumer] = await ethers.getSigners(); await deployments.delete("BlockHashRNG"); await deployments.deploy("BlockHashRNG", { from: deployer.address, - args: [deployer.address, deployer.address, 10], // governor, consumer, lookaheadTime (seconds) + args: [deployer.address, consumer.address, 10], // governor, consumer, lookaheadTime (seconds) });contracts/src/rng/RNGWithFallback.sol (1)
169-173: Document implications of emergencyReset.While useful for recovery,
emergencyResetcould cause loss of pending randomness requests. Consider adding clear documentation about when this should be used and its implications./// @dev Emergency reset the RNG. /// Useful for the governor to ensure that re-requesting a random number will not be blocked by a previous request. +/// WARNING: This will clear any pending randomness requests, potentially causing loss of randomness. +/// Only use in emergency situations when the RNG is stuck. function emergencyReset() external onlyByGovernor {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
contracts/deploy/00-chainlink-rng.ts(1 hunks)contracts/deploy/00-home-chain-arbitration-neo.ts(3 hunks)contracts/deploy/00-home-chain-arbitration.ts(2 hunks)contracts/deploy/00-home-chain-resolver.ts(0 hunks)contracts/deploy/00-rng.ts(2 hunks)contracts/deploy/change-sortition-module-rng.ts(1 hunks)contracts/src/arbitration/SortitionModule.sol(2 hunks)contracts/src/arbitration/SortitionModuleBase.sol(5 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(2 hunks)contracts/src/rng/BlockhashRNG.sol(1 hunks)contracts/src/rng/ChainlinkRNG.sol(8 hunks)contracts/src/rng/IRNG.sol(1 hunks)contracts/src/rng/IncrementalNG.sol(1 hunks)contracts/src/rng/RNG.sol(0 hunks)contracts/src/rng/RNGWithFallback.sol(1 hunks)contracts/src/rng/RandomizerRNG.sol(6 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)contracts/test/arbitration/draw.ts(1 hunks)contracts/test/arbitration/staking-neo.ts(2 hunks)contracts/test/arbitration/staking.ts(3 hunks)contracts/test/foundry/KlerosCore.t.sol(34 hunks)contracts/test/integration/index.ts(1 hunks)contracts/test/proxy/index.ts(1 hunks)contracts/test/rng/index.ts(5 hunks)contracts/test/utils/getActualAddress.ts(1 hunks)cspell.json(1 hunks)
💤 Files with no reviewable changes (2)
- contracts/deploy/00-home-chain-resolver.ts
- contracts/src/rng/RNG.sol
🧰 Additional context used
🧠 Learnings (24)
📓 Common 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.
contracts/deploy/00-chainlink-rng.ts (1)
Learnt from: jaybuidl
PR: #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.
contracts/test/utils/getActualAddress.ts (8)
Learnt from: jaybuidl
PR: #1620
File: contracts/test/arbitration/draw.ts:84-84
Timestamp: 2024-11-05T11:32:11.238Z
Learning: In TypeScript code using ethers.js version 6, contract.target should be used instead of contract.address to access a contract's address.
Learnt from: Harman-singh-waraich
PR: #1687
File: web/src/context/AtlasProvider.tsx:225-244
Timestamp: 2024-10-15T16:18:32.543Z
Learning: In web/src/context/AtlasProvider.tsx, the atlasUri variable comes from environment variables and does not change, so it does not need to be included in dependency arrays.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: Harman-singh-waraich
PR: #1727
File: web/src/utils/atlas/updateEmail.ts:34-37
Timestamp: 2024-10-28T12:20:19.884Z
Learning: In web/src/utils/atlas/updateEmail.ts, the error coming from the GraphQLError array already has the necessary structure, so additional specificity in error handling is unnecessary.
Learnt from: jaybuidl
PR: #1964
File: contracts/scripts/shutter.ts:3-3
Timestamp: 2025-04-30T22:12:25.476Z
Learning: In Node.js, importing the crypto module as import crypto from "crypto" is a valid approach when using methods like crypto.randomBytes(). This import style works fine and doesn't need to be changed to a destructured import when these methods are used.
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:1-1
Timestamp: 2024-10-22T09:38:20.093Z
Learning: In the TypeScript file kleros-sdk/src/dataMappings/utils/actionTypes.ts, the Abi type is parsed later in the action functions, so importing Abi from viem in this file is unnecessary.
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/sdk.ts:1-3
Timestamp: 2024-10-22T10:23:15.789Z
Learning: In kleros-sdk/src/sdk.ts, the PublicClient type is used and should not be flagged as unused.
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/requests/gqlClient.ts:18-18
Timestamp: 2024-10-24T08:16:02.749Z
Learning: In this TypeScript project, when a file (such as kleros-sdk/src/requests/gqlClient.ts) exports only a single entity, it's acceptable to use default exports instead of named exports.
contracts/test/arbitration/staking-neo.ts (9)
Learnt from: jaybuidl
PR: #1620
File: contracts/test/arbitration/draw.ts:84-84
Timestamp: 2024-11-05T11:32:11.238Z
Learning: In TypeScript code using ethers.js version 6, contract.target should be used instead of contract.address to access a contract's address.
Learnt from: jaybuidl
PR: #1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In contracts/config/courts.v2.mainnet-neo.json, the minStake parameter is denominated in PNK, not ETH.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: jaybuidl
PR: #1746
File: contracts/config/courts.v2.mainnet-neo.json:3-5
Timestamp: 2024-11-19T17:18:39.007Z
Learning: In contracts/config/courts.v2.mainnet-neo.json, the General Court (id: 1) intentionally references itself as its parent ("parent": 1). This self-reference is acceptable and should not be flagged as an issue in future reviews.
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/sdk.ts:1-3
Timestamp: 2024-10-22T10:23:15.789Z
Learning: In kleros-sdk/src/sdk.ts, the PublicClient type is used and should not be flagged as unused.
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/requests/gqlClient.ts:18-18
Timestamp: 2024-10-24T08:16:02.749Z
Learning: In this TypeScript project, when a file (such as kleros-sdk/src/requests/gqlClient.ts) exports only a single entity, it's acceptable to use default exports instead of named exports.
Learnt from: Harman-singh-waraich
PR: #1846
File: prettier-config/index.js:26-26
Timestamp: 2025-01-23T08:14:47.397Z
Learning: The prettier-plugin-solidity plugin is installed in the kleros-v2 repository, even though it's not visible in the sandbox environment's node_modules (which is expected as node_modules is not committed to the repository).
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/deploy/00-home-chain-arbitration-neo.ts (7)
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/utils/getDispute.ts:38-40
Timestamp: 2024-10-21T10:32:16.970Z
Learning: The variables 'arbitrableChainID' and 'externalDisputeID' are required by the context to have uppercase 'ID', so they should remain unchanged even if the corresponding source properties use 'Id'.
Learnt from: jaybuidl
PR: #1746
File: contracts/config/courts.v2.mainnet-neo.json:3-5
Timestamp: 2024-11-19T17:18:39.007Z
Learning: In contracts/config/courts.v2.mainnet-neo.json, the General Court (id: 1) intentionally references itself as its parent ("parent": 1). This self-reference is acceptable and should not be flagged as an issue in future reviews.
Learnt from: jaybuidl
PR: #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.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: Harman-singh-waraich
PR: #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.
Learnt from: tractorss
PR: #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.
Learnt from: jaybuidl
PR: #1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In contracts/config/courts.v2.mainnet-neo.json, the minStake parameter is denominated in PNK, not ETH.
contracts/deploy/change-sortition-module-rng.ts (3)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
Learnt from: jaybuidl
PR: #1582
File: web-devtools/src/app/(main)/ruler/RulingModes.tsx:233-233
Timestamp: 2024-10-09T10:19:11.816Z
Learning: In 'web-devtools/src/app/(main)/ruler/RulingModes.tsx', the label 'Random Preset' is correct and should not be changed to 'Automatic Random'.
contracts/deploy/00-home-chain-arbitration.ts (3)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
contracts/test/arbitration/staking.ts (6)
Learnt from: jaybuidl
PR: #1620
File: contracts/test/arbitration/draw.ts:84-84
Timestamp: 2024-11-05T11:32:11.238Z
Learning: In TypeScript code using ethers.js version 6, contract.target should be used instead of contract.address to access a contract's address.
Learnt from: jaybuidl
PR: #1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In contracts/config/courts.v2.mainnet-neo.json, the minStake parameter is denominated in PNK, not ETH.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/requests/gqlClient.ts:18-18
Timestamp: 2024-10-24T08:16:02.749Z
Learning: In this TypeScript project, when a file (such as kleros-sdk/src/requests/gqlClient.ts) exports only a single entity, it's acceptable to use default exports instead of named exports.
Learnt from: jaybuidl
PR: #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.
Learnt from: Harman-singh-waraich
PR: #1846
File: prettier-config/index.js:26-26
Timestamp: 2025-01-23T08:14:47.397Z
Learning: The prettier-plugin-solidity plugin is installed in the kleros-v2 repository, even though it's not visible in the sandbox environment's node_modules (which is expected as node_modules is not committed to the repository).
contracts/deploy/00-rng.ts (3)
Learnt from: jaybuidl
PR: #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.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/test/arbitration/dispute-kit-gated.ts (5)
Learnt from: Harman-singh-waraich
PR: #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.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: Harman-singh-waraich
PR: #1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the CustomContextInputs component located at web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx, the DisputeRequestParams array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/src/arbitration/SortitionModuleNeo.sol (3)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
contracts/test/proxy/index.ts (3)
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:1-1
Timestamp: 2024-10-22T09:38:20.093Z
Learning: In the TypeScript file kleros-sdk/src/dataMappings/utils/actionTypes.ts, the Abi type is parsed later in the action functions, so importing Abi from viem in this file is unnecessary.
Learnt from: jaybuidl
PR: #1620
File: contracts/test/arbitration/draw.ts:84-84
Timestamp: 2024-11-05T11:32:11.238Z
Learning: In TypeScript code using ethers.js version 6, contract.target should be used instead of contract.address to access a contract's address.
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/requests/gqlClient.ts:18-18
Timestamp: 2024-10-24T08:16:02.749Z
Learning: In this TypeScript project, when a file (such as kleros-sdk/src/requests/gqlClient.ts) exports only a single entity, it's acceptable to use default exports instead of named exports.
contracts/test/integration/index.ts (7)
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/sdk.ts:1-3
Timestamp: 2024-10-22T10:23:15.789Z
Learning: In kleros-sdk/src/sdk.ts, the PublicClient type is used and should not be flagged as unused.
Learnt from: jaybuidl
PR: #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.
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/requests/gqlClient.ts:18-18
Timestamp: 2024-10-24T08:16:02.749Z
Learning: In this TypeScript project, when a file (such as kleros-sdk/src/requests/gqlClient.ts) exports only a single entity, it's acceptable to use default exports instead of named exports.
Learnt from: jaybuidl
PR: #1964
File: contracts/scripts/shutter.ts:3-3
Timestamp: 2025-04-30T22:12:25.476Z
Learning: In Node.js, importing the crypto module as import crypto from "crypto" is a valid approach when using methods like crypto.randomBytes(). This import style works fine and doesn't need to be changed to a destructured import when these methods are used.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
Learnt from: Harman-singh-waraich
PR: #1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the Coherency component (web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx), totalResolvedVotes is always greater than or equal to totalCoherentVotes. When both are zero, 0/0 results in NaN, which is acceptable in this context.
contracts/test/arbitration/draw.ts (6)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1582
File: web-devtools/src/app/(main)/ruler/RulingModes.tsx:233-233
Timestamp: 2024-10-09T10:19:11.816Z
Learning: In 'web-devtools/src/app/(main)/ruler/RulingModes.tsx', the label 'Random Preset' is correct and should not be changed to 'Automatic Random'.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
Learnt from: jaybuidl
PR: #1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In contracts/config/courts.v2.mainnet-neo.json, the minStake parameter is denominated in PNK, not ETH.
Learnt from: jaybuidl
PR: #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.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
contracts/src/rng/IncrementalNG.sol (2)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/src/arbitration/SortitionModuleBase.sol (2)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/src/rng/IRNG.sol (2)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/src/arbitration/SortitionModule.sol (2)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/test/foundry/KlerosCore.t.sol (3)
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/src/rng/BlockhashRNG.sol (2)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/src/rng/RandomizerRNG.sol (2)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/src/rng/RNGWithFallback.sol (1)
Learnt from: jaybuidl
PR: #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.
contracts/src/rng/ChainlinkRNG.sol (2)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/test/rng/index.ts (4)
Learnt from: jaybuidl
PR: #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.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In useGenesisBlock.ts, within the useEffect hook, the conditions (isKlerosUniversity, isKlerosNeo, isTestnetDeployment) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: Harman-singh-waraich
PR: #1744
File: web/src/hooks/queries/useHomePageBlockQuery.ts:71-71
Timestamp: 2024-11-19T05:29:56.238Z
Learning: In web/src/hooks/queries/useHomePageBlockQuery.ts, the non-null assertions on blockNumber! and genesisBlock! within queryFn are safe because isEnabled ensures that queryFn only runs when either blockNumber or genesisBlock is defined.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (63)
contracts/test/proxy/index.ts (1)
7-8: Avoid deep-linking intotypechain-types– prefer barrel exports for stabilityPointing directly to
../../typechain-types/src/proxy/mock/by-rewrite/UpgradedByRewrite.solmakes the test depend on the internal folder structure generated by TypeChain.
A future refactor (e.g. changingoutDir, renaming the solidity file, or upgrading TypeChain) will break this path silently while the re-exported barrel../../typechain-typeswould stay intact.Two low-friction options:
-import { UpgradedByRewrite as UpgradedByRewriteV1 } from "../../typechain-types/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol"; +import type { UpgradedByRewrite as UpgradedByRewriteV1 } from "../../typechain-types";or add a manual re-export in
typechain-types/index.tsduring code-gen to expose the duplicate names cleanly (e.g.UpgradedByRewriteV1,UpgradedByRewriteV2).Either keeps the import resilient and removes the brittle deep path.
[ suggest_optional_refactor ]cspell.json (1)
38-38: LGTM! Necessary addition for the RNG interface refactoring.Adding "IRNG" to the spell checker dictionary is correct and necessary to support the interface rename from
RNGtoIRNGthroughout the codebase.contracts/deploy/00-chainlink-rng.ts (1)
74-74: Good addition! Helpful deployment guidance.Adding the console message with the Chainlink VRF registration link provides useful post-deployment guidance for users.
contracts/test/utils/getActualAddress.ts (1)
11-11: LGTM! Updated to modern import assertion syntax.The change from
assert: { type: "json" }towith: { type: "json" }aligns with the current JavaScript/TypeScript standard for import assertions.contracts/test/arbitration/dispute-kit-gated.ts (2)
64-64: LGTM! Correctly updated for simplified RNG interface.The method call now correctly passes only the RNG address, reflecting the removal of the RNG lookahead parameter from the interface.
141-144: LGTM! Simplified block mining logic.The removal of the RNG lookahead-based block mining loop and replacement with single block mining correctly reflects the interface changes where timing is now based on timestamps rather than block lookahead.
contracts/test/arbitration/staking-neo.ts (2)
59-65: LGTM! Improved TypeScript type safety.Using generic type parameters with
ethers.getContract<T>()is cleaner and provides better type safety than explicit type assertions.
108-108: LGTM! Correctly simplified block mining logic.The change from mining multiple blocks based on RNG lookahead to mining a single block correctly reflects the removal of the lookahead parameter from the RNG interface.
contracts/test/arbitration/draw.ts (2)
84-84: LGTM: Simplified RNG configuration callThe removal of the second argument from
changeRandomNumberGeneratorcorrectly aligns with the new IRNG interface that eliminates the lookahead parameter.
177-177: LGTM: Simplified phase transition logicThe removal of the block mining loop that previously handled RNG lookahead is correct. The new IRNG interface no longer requires block-based timing, making this simplification appropriate.
contracts/src/rng/IRNG.sol (1)
1-13: LGTM: Well-designed RNG interfaceThe new IRNG interface is clean and simplified compared to the previous RNG interface. The removal of block-based parameters and the clear function signatures align well with the PR objectives. The documentation is appropriate and the interface follows Solidity best practices.
contracts/deploy/change-sortition-module-rng.ts (2)
26-27: LGTM: Updated to use new consumer role methodThe change from
changeSortitionModuletochangeConsumercorrectly reflects the refactoring of RNG contracts where the authorized role was renamed fromsortitionModuletoconsumer.
29-30: LGTM: Simplified RNG configurationThe removal of the second argument from
changeRandomNumberGeneratoris consistent with the elimination of the lookahead parameter in the new IRNG interface.contracts/deploy/00-rng.ts (3)
10-10: LGTM: Transition to time-based lookaheadThe change from
RNG_LOOKAHEAD(block-based) toRNG_LOOKAHEAD_TIME(time-based) with a 30-minute value aligns with the RNG refactoring that moves away from block-based timing mechanisms.
34-38: LGTM: Updated BlockHashRNG constructor argumentsThe constructor arguments correctly reflect the new BlockHashRNG interface that takes a governor, consumer, and time-based lookahead parameter instead of the previous block-based approach.
42-42: LGTM: Simplified sortition module configurationThe single-argument call to
changeRandomNumberGeneratoris consistent with the removal of the lookahead parameter from the sortition module interface.contracts/deploy/00-home-chain-arbitration.ts (2)
55-55: LGTM: Updated SortitionModule constructorThe removal of the RNG lookahead parameter from the SortitionModule constructor correctly aligns with the interface changes that eliminate block-based timing dependencies.
88-93: LGTM: Improved RNG consumer managementThe change to use
changeConsumerinstead ofchangeSortitionModuleis correct, and the conditional check before calling the method is a good optimization that avoids unnecessary transactions.contracts/test/integration/index.ts (1)
206-208: LGTM! Improved type safety with explicit parameter typing.The explicit typing of the function parameter enhances type safety and makes the expected structure clear, which is a good practice in TypeScript.
contracts/src/arbitration/SortitionModule.sol (3)
5-5: LGTM! Interface abstraction improvement.The change from concrete RNG type to IRNG interface provides better abstraction and aligns with the PR's objective to introduce a composite RNG with fallback logic.
32-32: LGTM! Parameter updated to match new interface.The change from RNG to IRNG parameter and removal of _rngLookahead aligns with the PR objectives to use the new IRNG interface and remove block-based lookahead logic.
34-34: LGTM! Base initializer call updated correctly.The removal of _rngLookahead parameter from the base initializer call is consistent with the function signature changes and the broader refactor to remove block-based RNG timing.
contracts/deploy/00-home-chain-arbitration-neo.ts (3)
13-13: LGTM! Cleanup of unused deployment method.Removing the unused
executemethod from the destructured deployments object improves code clarity by only importing what's needed.
52-52: LGTM! Deployment arguments updated to match contract changes.The removal of RNG_LOOKAHEAD parameter from the SortitionModuleNeo deployment arguments correctly aligns with the contract's updated initialize() function signature.
86-91: LGTM! RNG linkage updated to use consumer pattern.The changes from
changeSortitionModule/sortitionModule()tochangeConsumer/consumer()correctly implement the new RNG consumer role pattern mentioned in the PR objectives.contracts/test/arbitration/staking.ts (3)
30-34: LGTM! Improved TypeScript contract retrieval with generics.Using TypeScript generics instead of type casting provides better type safety and is more idiomatic. This is a good improvement over the previous
as ContractTypeapproach.
57-57: LGTM! Block mining simplified after RNG lookahead removal.The single
evm_minecall replaces the previous lookahead-based block mining logic, correctly reflecting the shift from block-based to time-based randomness readiness.
394-395: LGTM! Consistent simplification of block mining logic.The single block mine after phase transition maintains consistency with the updated RNG timing model throughout the test suite.
contracts/src/arbitration/SortitionModuleNeo.sol (3)
5-5: LGTM! Import statement updated consistently with interface refactor.The change to import IRNG interface instead of concrete RNG type follows the same pattern as SortitionModule.sol and supports the overall interface abstraction effort.
42-42: LGTM! Parameter signature updated consistently with base module.The change from RNG to IRNG parameter and removal of _rngLookahead maintains consistency with SortitionModule.sol while preserving the Neo-specific parameters.
46-46: LGTM! Base initializer call updated to match interface changes.The removal of _rngLookahead parameter from the base initializer call is consistent with the updated SortitionModuleBase interface and follows the same pattern as SortitionModule.sol.
contracts/src/rng/IncrementalNG.sol (4)
4-4: LGTM! Interface import updated correctly.The import has been properly updated to use the new
IRNGinterface.
6-9: LGTM! Contract declaration and comments updated appropriately.The contract now correctly implements the
IRNGinterface instead of inheriting from the deprecatedRNGclass. The comments have been updated to reflect the simplified interface.
17-19: LGTM! Function signature simplified correctly.The
requestRandomness()function signature has been properly simplified by removing the unused_blockparameter, aligning with the newIRNGinterface. The no-op implementation remains appropriate for this testing contract.
23-27: LGTM! Function signature simplified correctly.The
receiveRandomness()function signature has been properly simplified by removing the unused_blockparameter. The incremental logic remains intact and appropriate for testing purposes.contracts/src/arbitration/SortitionModuleBase.sol (5)
10-10: LGTM! Import updated to new IRNG interface.The import has been correctly updated to use the new
IRNGinterface.
54-54: LGTM! Storage variable type updated correctly.The
rngstorage variable has been properly updated to use theIRNGinterface type.
105-105: LGTM! Constructor parameter updated correctly.The constructor parameter has been properly updated to accept an
IRNGinstance instead of the deprecatedRNGtype.
152-159: LGTM! Function updated with simplified RNG interface.The
changeRandomNumberGeneratorfunction has been correctly updated:
- Parameter type changed to
IRNG- Function name and comments updated appropriately
- Call to
requestRandomness()simplified by removing the block parameterThe logic maintains the same behavior while using the simplified interface.
172-172: LGTM! RNG method calls simplified correctly.The calls to
requestRandomness()andreceiveRandomness()have been properly simplified by removing the block parameters, aligning with the newIRNGinterface. This removes the unnecessary block-based timing complexity while maintaining the same phase transition logic.Also applies to: 175-175
contracts/test/foundry/KlerosCore.t.sol (6)
55-55: Comment accurately reflects the time-based approach.The comment update from block-based to time-based properly documents the semantic change in the RNG lookahead mechanism.
95-96: Significant increase in RNG lookahead time.The rngLookahead value has been increased from 20 to 600 seconds (10 minutes), which is a more realistic timing for RNG operations. This aligns with the shift to timestamp-based randomness and should provide adequate time for RNG fulfillment in production-like scenarios.
106-111: Simplified SortitionModule initialization.The removal of
rngLookaheadfrom the SortitionModule initialization parameters correctly reflects the architectural change where RNG timing is now managed at the RNG contract level rather than the consumer level.
116-117: Proper RNG consumer linkage with governor privileges.The explicit
changeConsumercall with governor privileges correctly implements the new RNG permission model, ensuring only authorized consumers can request randomness.
287-298: Consistent RNG initialization pattern in event tests.The initialization sequence correctly follows the new pattern: first initialize the SortitionModule, then link it as the RNG consumer with governor privileges.
1006-1006: Consistent implementation of timestamp-based RNG readiness.All instances of
vm.roll(block.number + rngLookahead + 1)have been correctly replaced withvm.warp(block.timestamp + rngLookahead), implementing the shift from block-based to timestamp-based RNG mechanics throughout the test suite.Also applies to: 1053-1053, 1080-1080, 1136-1136, 1424-1424, 1483-1483, 1526-1526, 1636-1636, 1661-1661, 1772-1772, 1799-1799, 1840-1840, 1889-1889, 1979-1979, 2023-2023, 2119-2119, 2197-2197, 2231-2231, 2243-2243, 2349-2349, 2424-2424, 2470-2470, 2525-2525, 2615-2615, 2669-2669, 2708-2708, 2762-2762, 2805-2805, 2899-2899
contracts/test/rng/index.ts (3)
24-30: LGTM! Test correctly updated for new IRNG interface.The removal of numeric arguments from
receiveRandomnesscalls aligns with the simplified IRNG interface.
96-109: LGTM! ChainlinkRNG tests properly updated.Good use of the shared
abiCoderinstance and correct removal of unused parameters.
155-202: LGTM! RandomizerRNG tests correctly updated.Tests properly handle the simplified interface and maintain good coverage for both single and multiple request scenarios.
contracts/src/rng/RandomizerRNG.sol (3)
5-10: LGTM! Correct interface migration to IRNG.The contract properly implements the new IRNG interface as part of the broader RNG refactoring.
16-16: LGTM! Consumer role properly implemented.The migration from
sortitionModuletoconsumeris complete and consistent throughout the contract, maintaining proper access control.Also applies to: 44-47, 54-62, 74-78
102-107: LGTM! Randomness functions correctly simplified.The removal of block parameters aligns with the IRNG interface. The lack of concurrent request handling is acceptable as confirmed by previous learnings that only the SortitionModule (now consumer) calls these functions without concurrent requests.
Also applies to: 124-126
contracts/src/rng/ChainlinkRNG.sol (3)
8-12: LGTM! Proper interface implementation and consistent event naming.The contract correctly implements IRNG while maintaining Chainlink VRF integration through VRFConsumerBaseV2Plus inheritance. Event parameter naming is consistent.
Also applies to: 32-38
18-18: LGTM! Consumer role properly implemented with correct access control.The migration from sortitionModule to consumer is complete and maintains proper authorization checks.
Also applies to: 49-52, 94-98
135-153: LGTM! VRF v2.5 integration with native ETH payment.The implementation correctly uses Chainlink VRF v2.5 with native ETH payment enabled. The lack of concurrent request handling aligns with the design constraint that only one consumer makes requests.
contracts/src/rng/BlockhashRNG.sol (3)
13-50: LGTM! Well-structured contract with appropriate L2 considerations.Good use of
immutableforlookaheadTimeand clear role separation. The timestamp-based approach is appropriate for L2 environments with sporadic block production.
109-119: LGTM! Useful view functions for checking randomness readiness.The view functions properly handle the case where no request has been made and provide clear information about when randomness will be available.
79-101: Ignore blockhash and timestamp‐collision concernsThe two raised issues don’t apply here:
- Using
blockhash(block.number - 1)always returns the hash of the immediately previous block, which is guaranteed to be within the last 256 blocks, so it will never be 0 due to the EVM’s 256-block limit.- The contract only tracks a single outstanding request via
requestTimestamp(overwritten on each call) and restricts calls to the designatedconsumer, so you cannot have concurrent requests mapping to the same key—no collision risk.No changes needed.
Likely an incorrect or invalid review comment.
contracts/src/rng/RNGWithFallback.sol (5)
42-49: LGTM! Proper initialization with input validation.Good validation of the default RNG address and proper initialization of the RNG array.
147-158: Missing validation in removeLastRngFallback.The function doesn't check if the RNG being removed has any pending requests or if it's currently being used. This could lead to failed randomness retrieval.
Consider adding validation to ensure safe removal.
function removeLastRngFallback() external onlyByGovernor { require(rngs.length > 1, "No fallback RNG"); + require(currentRngIndex < rngs.length - 1, "Cannot remove active RNG");⛔ 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.Learnt from: jaybuidl PR: kleros/kleros-v2#1778 File: contracts/src/rng/RandomizerRNG.sol:21-21 Timestamp: 2024-12-07T11:41:15.835Z Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
129-136: Risk in changeDefaultRng when taking over pending requests.Calling
_requestRandomness(DEFAULT_RNG)during RNG change could fail if the new RNG is not ready or properly configured, potentially leaving the contract in an inconsistent state.Consider adding try-catch or validation before attempting to take over requests.
⛔ 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.Learnt from: jaybuidl PR: kleros/kleros-v2#1778 File: contracts/src/rng/RandomizerRNG.sol:21-21 Timestamp: 2024-12-07T11:41:15.835Z Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
84-109: Issues with fallback mechanism in receiveRandomness.Several concerns:
- Makes external calls without reentrancy protection
- When falling back, immediately calls
_requestRandomnesswhich might fail if the next RNG requires specific conditions- No validation that the fallback RNG is operational before switching
Consider adding more robust error handling and validation of RNG availability.
⛔ 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.Learnt from: jaybuidl PR: kleros/kleros-v2#1778 File: contracts/src/rng/RandomizerRNG.sol:21-21 Timestamp: 2024-12-07T11:41:15.835Z Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
70-80: Potential reentrancy vulnerability in requestRandomness.The contract updates state variables before making an external call to
rngs[_rngIndex].requestRandomness(). A malicious RNG could potentially call back into this contract.Consider using reentrancy guards or following checks-effects-interactions pattern more strictly.
+import "@openzeppelin/contracts/utils/ReentrancyGuard.sol"; -contract RNGWithFallback is IRNG { +contract RNGWithFallback is IRNG, ReentrancyGuard { - function requestRandomness() external override onlyByConsumer { + function requestRandomness() external override onlyByConsumer nonReentrant { - function receiveRandomness() external override onlyByConsumer returns (uint256 randomNumber) { + function receiveRandomness() external override onlyByConsumer nonReentrant returns (uint256 randomNumber) {⛔ 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.Learnt from: jaybuidl PR: kleros/kleros-v2#1778 File: contracts/src/rng/RandomizerRNG.sol:21-21 Timestamp: 2024-12-07T11:41:15.835Z Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
unknownunknown1
force-pushed
the
feat/rng-fallback2
branch
from
5db2a51 to
8d9d2cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
contracts/deploy/00-chainlink-rng.ts(1 hunks)contracts/deploy/00-home-chain-arbitration-neo.ts(3 hunks)contracts/deploy/00-home-chain-arbitration.ts(2 hunks)contracts/deploy/00-home-chain-resolver.ts(0 hunks)contracts/deploy/00-rng.ts(2 hunks)contracts/deploy/change-sortition-module-rng.ts(1 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(3 hunks)contracts/src/arbitration/SortitionModule.sol(2 hunks)contracts/src/arbitration/SortitionModuleBase.sol(5 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(2 hunks)contracts/src/rng/BlockhashRNG.sol(1 hunks)contracts/src/rng/ChainlinkRNG.sol(8 hunks)contracts/src/rng/IRNG.sol(1 hunks)contracts/src/rng/IncrementalNG.sol(1 hunks)contracts/src/rng/RNG.sol(0 hunks)contracts/src/rng/RNGWithFallback.sol(1 hunks)contracts/src/rng/RandomizerRNG.sol(6 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)contracts/test/arbitration/draw.ts(1 hunks)contracts/test/arbitration/staking-neo.ts(2 hunks)contracts/test/arbitration/staking.ts(3 hunks)contracts/test/foundry/KlerosCore.t.sol(35 hunks)contracts/test/integration/index.ts(1 hunks)contracts/test/proxy/index.ts(1 hunks)contracts/test/rng/index.ts(5 hunks)cspell.json(1 hunks)
💤 Files with no reviewable changes (2)
- contracts/deploy/00-home-chain-resolver.ts
- contracts/src/rng/RNG.sol
✅ Files skipped from review due to trivial changes (5)
- contracts/deploy/00-chainlink-rng.ts
- contracts/src/rng/IRNG.sol
- contracts/test/proxy/index.ts
- contracts/test/rng/index.ts
- contracts/src/arbitration/KlerosCoreNeo.sol
🚧 Files skipped from review as they are similar to previous changes (17)
- cspell.json
- contracts/test/arbitration/dispute-kit-gated.ts
- contracts/deploy/change-sortition-module-rng.ts
- contracts/deploy/00-rng.ts
- contracts/deploy/00-home-chain-arbitration.ts
- contracts/test/integration/index.ts
- contracts/test/arbitration/draw.ts
- contracts/test/arbitration/staking.ts
- contracts/test/arbitration/staking-neo.ts
- contracts/src/rng/IncrementalNG.sol
- contracts/deploy/00-home-chain-arbitration-neo.ts
- contracts/src/arbitration/SortitionModuleBase.sol
- contracts/test/foundry/KlerosCore.t.sol
- contracts/src/arbitration/SortitionModuleNeo.sol
- contracts/src/arbitration/SortitionModule.sol
- contracts/src/rng/RandomizerRNG.sol
- contracts/src/rng/ChainlinkRNG.sol
🧰 Additional context used
🧠 Learnings (3)
📓 Common 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.
📚 Learning: in the `contracts/src/rng/chainlinkrng.sol` contract, the `requestrandomness` function is restricted...
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Applied to files:
contracts/src/rng/BlockhashRNG.solcontracts/src/rng/RNGWithFallback.sol
📚 Learning: only the `sortitionmodule` can call `requestrandomness`, and it doesn't send concurrent requests, so...
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
Applied to files:
contracts/src/rng/BlockhashRNG.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (12)
contracts/src/rng/BlockhashRNG.sol (7)
5-5: LGTM: Import aligns with new interface pattern.The import of
IRNG.solcorrectly replaces the previous RNG interface inheritance.
10-22: Excellent storage design for L2 compatibility.The transition from block-based to timestamp-based randomness is well-architected:
- Clear separation of governor/consumer roles
lookaheadTimeas immutable provides gas efficiency- Random number caching via mapping enables future retrieval beyond blockhash window
- L2 compatibility rationale is well-documented
28-36: Proper access control implementation.The modifiers correctly implement role-based access control with clear error messages and standard Solidity patterns.
72-75: LGTM: Simple and effective request implementation.The function correctly implements the timestamp-based approach for L2 compatibility. Overwriting previous requests is acceptable given the single-consumer model.
77-101: Excellent implementation with comprehensive edge case handling.The function properly handles all scenarios:
- Early validation for missing requests and timing constraints
- Efficient caching mechanism to preserve randomness beyond blockhash window
- Correct use of
blockhash(block.number - 1)for randomness generation- Graceful degradation returning 0 when randomness unavailable
The timestamp-based approach with caching elegantly solves L2 block production irregularities.
107-112: LGTM: Clear readiness check implementation.The function correctly mirrors the timing logic from
receiveRandomnessand provides useful external visibility.
114-119: LGTM: Consistent timestamp calculation.The function provides consistent timing information that aligns with the core randomness logic.
contracts/src/rng/RNGWithFallback.sol (5)
1-11: LGTM!The contract structure, imports, and interface implementation are well-organized and follow Solidity best practices.
13-17: LGTM!The state variables are well-designed with appropriate visibility and immutability where needed. Making
rngimmutable is a good security practice.
23-24: LGTM!The events are well-designed for monitoring fallback occurrences and governance changes.
47-55: LGTM!The access control modifiers are implemented correctly with clear error messages.
62-65: Add request state validation.The function should validate that no request is already pending to prevent overwriting
requestTimestampand align with the PR objective of preventing repeated calls.function requestRandomness() external override onlyByConsumer { + require(requestTimestamp == 0, "Request already pending"); requestTimestamp = block.timestamp; rng.requestRandomness(); }⛔ 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.Learnt from: jaybuidl PR: kleros/kleros-v2#1778 File: contracts/src/rng/RandomizerRNG.sol:21-21 Timestamp: 2024-12-07T11:41:15.835Z Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
|
Updated the RNG contracts according to my own feedback
|
… deprecated for later removal
jaybuidl
force-pushed
the
feat/rng-fallback2
branch
from
8d9d2cb to
937fb97
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
contracts/src/rng/RNGWithFallback.sol (3)
34-41: Add validation for governor and consumer addresses.The constructor validates the RNG address but lacks validation for governor and consumer addresses, which could lead to deployment with invalid addresses and break governance functionality.
constructor(address _governor, address _consumer, uint256 _fallbackTimeoutSeconds, IRNG _rng) { + require(_governor != address(0), "Invalid governor"); + require(_consumer != address(0), "Invalid consumer"); require(address(_rng) != address(0), "Invalid default RNG");
69-82: Fix critical logical issues in fallback mechanism.Multiple critical issues exist:
- No validation that a request was made (
requestTimestampcould be 0)requestTimestampis never cleared, preventing future requests- Returns 0 when RNG returns 0 but timeout hasn't elapsed, which may cause consumer to revert
function receiveRandomness() external override onlyByConsumer returns (uint256 randomNumber) { + require(requestTimestamp != 0, "No request pending"); + // Try to get random number from the RNG contract randomNumber = rng.receiveRandomness(); // If we got a valid number, clear the request if (randomNumber != 0) { + requestTimestamp = 0; return randomNumber; } else if (block.timestamp > requestTimestamp + fallbackTimeoutSeconds) { // If the timeout is exceeded, try the fallback randomNumber = uint256(blockhash(block.number - 1)); + requestTimestamp = 0; emit RNGFallback(); + return randomNumber; } - return randomNumber; + + // If RNG returns 0 but timeout not exceeded, revert to avoid ambiguity + revert("RNG returned invalid value, timeout not exceeded"); }
90-105: Add input validation to governance functions.The governance functions lack validation for critical parameters, which could result in invalid contract states.
function changeGovernor(address _newGovernor) external onlyByGovernor { + require(_newGovernor != address(0), "Invalid governor"); governor = _newGovernor; } function changeConsumer(address _consumer) external onlyByGovernor { + require(_consumer != address(0), "Invalid consumer"); consumer = _consumer; } function changeFallbackTimeout(uint256 _fallbackTimeoutSeconds) external onlyByGovernor { + require(_fallbackTimeoutSeconds > 0, "Invalid timeout"); fallbackTimeoutSeconds = _fallbackTimeoutSeconds; emit FallbackTimeoutChanged(_fallbackTimeoutSeconds); }contracts/src/rng/BlockhashRNG.sol (2)
42-50: Add input validation to constructor.The constructor lacks validation for critical parameters which could lead to deployment issues, as previously identified.
Apply this diff to add proper validation:
constructor(address _governor, address _consumer, uint256 _lookaheadTime) { + require(_governor != address(0), "Governor cannot be zero address"); + require(_consumer != address(0), "Consumer cannot be zero address"); + require(_lookaheadTime > 0, "Lookahead time must be positive"); governor = _governor; consumer = _consumer; lookaheadTime = _lookaheadTime; }
56-66: Add address validation to governance functions.The governance functions lack input validation which could result in accidentally setting invalid addresses, as previously identified.
Apply this diff to add validation:
function changeGovernor(address _governor) external onlyByGovernor { + require(_governor != address(0), "Governor cannot be zero address"); governor = _governor; } function changeConsumer(address _consumer) external onlyByGovernor { + require(_consumer != address(0), "Consumer cannot be zero address"); consumer = _consumer; }
🧹 Nitpick comments (1)
contracts/src/rng/RNGWithFallback.sol (1)
78-78: Consider blockhash availability limitation.Using
blockhash(block.number - 1)may return 0 if the block is more than 256 blocks old, which could happen if the fallback timeout is very long on chains with fast block times.Consider adding a check or using an alternative fallback mechanism:
- randomNumber = uint256(blockhash(block.number - 1)); + randomNumber = uint256(blockhash(block.number - 1)); + if (randomNumber == 0) { + // Fallback to a combination of block properties if blockhash unavailable + randomNumber = uint256(keccak256(abi.encodePacked(block.timestamp, block.difficulty, block.coinbase))); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
contracts/deploy/00-chainlink-rng.ts(1 hunks)contracts/deploy/00-home-chain-arbitration-neo.ts(2 hunks)contracts/deploy/00-home-chain-arbitration.ts(2 hunks)contracts/deploy/00-home-chain-resolver.ts(0 hunks)contracts/deploy/00-rng.ts(2 hunks)contracts/deploy/change-sortition-module-rng.ts(1 hunks)contracts/src/arbitration/SortitionModule.sol(2 hunks)contracts/src/arbitration/SortitionModuleBase.sol(5 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(2 hunks)contracts/src/rng/BlockhashRNG.sol(1 hunks)contracts/src/rng/ChainlinkRNG.sol(8 hunks)contracts/src/rng/IRNG.sol(1 hunks)contracts/src/rng/IncrementalNG.sol(1 hunks)contracts/src/rng/RNG.sol(0 hunks)contracts/src/rng/RNGWithFallback.sol(1 hunks)contracts/src/rng/RandomizerRNG.sol(6 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)contracts/test/arbitration/draw.ts(1 hunks)contracts/test/arbitration/staking-neo.ts(2 hunks)contracts/test/arbitration/staking.ts(3 hunks)contracts/test/foundry/KlerosCore.t.sol(35 hunks)contracts/test/integration/index.ts(1 hunks)contracts/test/proxy/index.ts(1 hunks)contracts/test/rng/index.ts(5 hunks)cspell.json(1 hunks)
💤 Files with no reviewable changes (2)
- contracts/deploy/00-home-chain-resolver.ts
- contracts/src/rng/RNG.sol
✅ Files skipped from review due to trivial changes (2)
- contracts/test/proxy/index.ts
- contracts/deploy/00-chainlink-rng.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- cspell.json
- contracts/test/arbitration/staking-neo.ts
- contracts/src/arbitration/SortitionModuleNeo.sol
- contracts/src/rng/IRNG.sol
- contracts/test/arbitration/draw.ts
- contracts/src/rng/RandomizerRNG.sol
- contracts/deploy/00-home-chain-arbitration-neo.ts
- contracts/deploy/00-rng.ts
- contracts/src/rng/IncrementalNG.sol
- contracts/test/arbitration/dispute-kit-gated.ts
- contracts/src/arbitration/SortitionModule.sol
- contracts/test/foundry/KlerosCore.t.sol
- contracts/test/arbitration/staking.ts
- contracts/src/rng/ChainlinkRNG.sol
- contracts/test/integration/index.ts
- contracts/deploy/00-home-chain-arbitration.ts
- contracts/deploy/change-sortition-module-rng.ts
- contracts/src/arbitration/SortitionModuleBase.sol
- contracts/test/rng/index.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common 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.
📚 Learning: in the `contracts/src/rng/chainlinkrng.sol` contract, the `requestrandomness` function is restricted...
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Applied to files:
contracts/src/rng/BlockhashRNG.solcontracts/src/rng/RNGWithFallback.sol
📚 Learning: only the `sortitionmodule` can call `requestrandomness`, and it doesn't send concurrent requests, so...
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
Applied to files:
contracts/src/rng/BlockhashRNG.solcontracts/src/rng/RNGWithFallback.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). (3)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
contracts/src/rng/BlockhashRNG.sol (6)
5-5: LGTM! Correct implementation of new IRNG interface.The import of
IRNGand contract declaration correctly implement the new standardized interface, aligning with the broader RNG system refactor described in the PR objectives.Also applies to: 13-13
18-22: Well-designed storage structure for timestamp-based randomness.The storage variables correctly implement the new timestamp-based approach with appropriate access controls and caching mechanism. Using
immutableforlookaheadTimeis a good gas optimization.
28-36: LGTM! Proper access control implementation.The modifiers correctly implement role-based access control with clear error messages, following standard Solidity patterns.
73-75: LGTM! Correct implementation of timestamp-based randomness request.The function correctly implements the new IRNG interface and timestamp-based approach by recording the current block timestamp to initiate the lookahead period.
79-101: LGTM! Well-implemented timestamp-based randomness retrieval with appropriate caching.The function correctly implements the new timing mechanism with proper checks for request status and time passage. The use of
blockhash(block.number - 1)is correct sinceblockhash(block.number)would always return 0. The caching strategy appropriately handles zero values by not caching them, allowing retry attempts if randomness is temporarily unavailable.
109-119: LGTM! Useful view functions with proper edge case handling.Both functions correctly implement their intended logic and properly handle the edge case where no request has been made. These functions provide valuable information for external callers to determine randomness readiness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/src/rng/IRNG.sol (2)
7-8: Document consumer restriction and in-flight dedup semantics on requestRandomness.Given the repo’s explicit constraint of a single consumer with no concurrent requests, encode that expectation in the interface docs to prevent misuse by future integrators and to align implementations.
Apply this diff to enrich the Natspec:
- /// @dev Request a random number. + /// @dev Request a random number. + /// Implementations should restrict this to the designated consumer (e.g., SortitionModule) + /// and MUST NOT create a new request while one is already pending.
10-12: Clarify state-mutating behavior and sentinel semantics in receiveRandomness docs.Some implementations may need to update internal state (e.g., finalize fallback) during receive; make that explicit. Also tighten the wording on the sentinel 0 behavior.
Apply this diff:
- /// @dev Receive the random number. - /// @return randomNumber Random Number. If the number is not ready or has not been required 0 instead. + /// @dev Receive the random number. + /// May mutate internal state (e.g., finalize fallback or clear a pending request). + /// @return randomNumber Random number; returns 0 if the number is not ready or has not been requested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/rng/IRNG.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
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.
📚 Learning: 2024-12-07T11:39:10.927Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Applied to files:
contracts/src/rng/IRNG.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
🔇 Additional comments (1)
contracts/src/rng/IRNG.sol (1)
6-9: LGTM: Minimal, clear interface aligned with PR goals.The two-method interface matches the new no-block-parameter design and suits the single-consumer assumption from previous decisions. Good abstraction boundary for implementers.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
contracts/deploy/00-rng-chainlink.ts (2)
7-14: Guard against conflicting RNG providers (Chainlink vs Randomizer)As written, both RNG providers can be deployed in the same run, which can lead to miswiring: RNGWithFallback is a single instance and will remain bound to the first RNG it was deployed with. If both providers run, the second provider may set its consumer to RNGWithFallback while RNGWithFallback still points at the first provider internally, causing requests to be sent to the wrong RNG.
Add an early guard to skip this script when RandomizerRNG is present (and mirror the guard in the Randomizer script to skip when ChainlinkRNG is present).
Apply this diff to add a guard:
const deployRng: DeployFunction = async (hre: HardhatRuntimeEnvironment) => { - const { getNamedAccounts, getChainId, ethers } = hre; + const { getNamedAccounts, getChainId, ethers, deployments } = hre; + // Avoid conflicting RNG providers. + const randomizer = await deployments.getOrNull("RandomizerRNG"); + if (randomizer) { + console.log("RandomizerRNG found; skipping ChainlinkRNG deployment."); + return; + }
65-77: Do not use the Mock coordinator on live networks
ChainlinkVRFCoordinatoris deployed here as a Mock earlier in the script. PassingChainlinkVRFCoordinator.targetinto ChainlinkRNG will silently wire a Mock on live networks. That will break randomness. Gate the Mock to Hardhat and require the real coordinator address on supported networks (env/config).Suggested refactor contained to this segment:
- const rng = await getContractOrDeploy(hre, "ChainlinkRNG", { + // Use a real coordinator on live networks; only use the Mock on Hardhat. + const coordinatorAddress = + chainId === HomeChains.HARDHAT + ? (await hre.ethers.getContract("ChainlinkVRFCoordinator")).target + : (process.env.CHAINLINK_VRF_COORDINATOR as string); + if (chainId !== HomeChains.HARDHAT && !coordinatorAddress) { + throw new Error("Missing CHAINLINK_VRF_COORDINATOR env var for this network"); + } + const rng = await getContractOrDeploy(hre, "ChainlinkRNG", { from: deployer, args: [ deployer, - deployer, // The consumer is configured as the RNGWithFallback later - ChainlinkVRFCoordinator.target, + deployer, // The consumer is configured as the RNGWithFallback later + coordinatorAddress, keyHash, subscriptionId, requestConfirmations, callbackGasLimit, ], log: true, });If you prefer config instead of env vars, I can wire it to your deployments config files.
contracts/deploy/00-rng-randomizer.ts (1)
7-14: Guard against conflicting RNG providers (Randomizer vs Chainlink)As in the Chainlink script, deploying both providers in one run leads to mismatched wiring with a single RNGWithFallback instance. Add a guard to skip this script when ChainlinkRNG is present.
Apply this diff:
const deployRng: DeployFunction = async (hre: HardhatRuntimeEnvironment) => { - const { getNamedAccounts, getChainId, ethers } = hre; + const { getNamedAccounts, getChainId, ethers, deployments } = hre; + // Avoid conflicting RNG providers. + const chainlink = await deployments.getOrNull("ChainlinkRNG"); + if (chainlink) { + console.log("ChainlinkRNG found; skipping RandomizerRNG deployment."); + return; + }
🧹 Nitpick comments (4)
contracts/deploy/00-home-chain-resolver.ts (1)
4-4: Good switch to idempotent deployment helperReplacing direct deploy with getContractOrDeploy improves idempotency across repeated runs. One nit: the destructured
deploy(Line 8) is now unused.If you want, I can submit a quick cleanup patch to remove the unused variable.
contracts/deploy/00-rng-chainlink.ts (2)
60-64: Use HTTPS and consider gating when loggingNit: switch the registration URL to HTTPS. Also, this message will show on any network when ChainlinkRNG is absent; consider logging only on networks where manual registration is relevant.
Apply this diff:
- console.log("Register this Chainlink consumer here: http://vrf.chain.link/"); + console.log("Register this Chainlink consumer here: https://vrf.chain.link/");
79-89: Make fallback timeout configurable per networkHardcoding 30 minutes may not fit all networks/ops requirements. Suggest making it configurable via env or config with a sensible default.
Apply this diff:
- const fallbackTimeoutSeconds = 30 * 60; // 30 minutes + const fallbackTimeoutSeconds = Number(process.env.RNG_FALLBACK_TIMEOUT_SECONDS ?? 30 * 60); // default 30mcontracts/deploy/00-rng-randomizer.ts (1)
33-43: Make fallback timeout configurable per networkSame suggestion as the Chainlink script: parameterize the 30-minute default.
Apply this diff:
- const fallbackTimeoutSeconds = 30 * 60; // 30 minutes + const fallbackTimeoutSeconds = Number(process.env.RNG_FALLBACK_TIMEOUT_SECONDS ?? 30 * 60); // default 30m
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
contracts/deploy/00-ethereum-pnk.ts(0 hunks)contracts/deploy/00-home-chain-arbitration-neo.ts(4 hunks)contracts/deploy/00-home-chain-arbitration.ts(3 hunks)contracts/deploy/00-home-chain-pnk-faucet.ts(0 hunks)contracts/deploy/00-home-chain-resolver.ts(2 hunks)contracts/deploy/00-rng-chainlink.ts(3 hunks)contracts/deploy/00-rng-randomizer.ts(2 hunks)contracts/deploy/00-rng.ts(0 hunks)contracts/test/rng/index.ts(7 hunks)
💤 Files with no reviewable changes (3)
- contracts/deploy/00-home-chain-pnk-faucet.ts
- contracts/deploy/00-ethereum-pnk.ts
- contracts/deploy/00-rng.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-07T11:39:10.927Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Applied to files:
contracts/deploy/00-rng-chainlink.tscontracts/deploy/00-home-chain-arbitration-neo.tscontracts/deploy/00-home-chain-arbitration.tscontracts/deploy/00-rng-randomizer.tscontracts/test/rng/index.ts
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
Applied to files:
contracts/deploy/00-home-chain-arbitration-neo.ts
🧬 Code Graph Analysis (4)
contracts/deploy/00-rng-chainlink.ts (1)
contracts/deploy/utils/getContractOrDeploy.ts (1)
getContractOrDeploy(6-20)
contracts/deploy/00-home-chain-arbitration.ts (4)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deployments/utils.ts (1)
deployments(3-16)contracts/deployments/devnet.viem.ts (1)
klerosCoreAddress(12320-12322)contracts/deployments/testnet.viem.ts (1)
klerosCoreAddress(10190-10192)
contracts/deploy/00-home-chain-resolver.ts (1)
contracts/deploy/utils/getContractOrDeploy.ts (1)
getContractOrDeploy(6-20)
contracts/deploy/00-rng-randomizer.ts (1)
contracts/deploy/utils/getContractOrDeploy.ts (1)
getContractOrDeploy(6-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Analyze (javascript)
🔇 Additional comments (22)
contracts/deploy/00-home-chain-resolver.ts (1)
18-22: Idempotent deploy of DisputeResolver looks correctUsing getContractOrDeploy with from/args/log preserves behavior and prevents accidental redeployments. No functional issues spotted.
contracts/deploy/00-home-chain-arbitration.ts (4)
9-9: Type import update aligns with RNGWithFallback integrationImporting RNGWithFallback here matches the new wiring in this script and across the PR. Looks good.
52-56: Correct wiring: SortitionModule now consumes RNGWithFallbackPassing
rngWithFallback.targetinto SortitionModule aligns with the new IRNG consumer model and centralizes RNG logic via the fallback wrapper. Good move.
82-87: Typed DisputeKit retrieval + conditional changeCore is soundFetching the typed contract and only calling
changeCorewhen needed avoids redundant txs. This is clean.
89-94: Conditional RNG consumer update is correctEnsures RNGWithFallback’s consumer is the newly deployed SortitionModule without unnecessary transactions. This matches the single-consumer assumption from prior learnings.
contracts/deploy/00-home-chain-arbitration-neo.ts (3)
9-9: Type import update aligns with RNGWithFallback integration (Neo)Importing RNGWithFallback is consistent with the new RNG consumer model.
47-58: Correct wiring: SortitionModuleNeo now consumes RNGWithFallbackReplacing
rng.targetwithrngWithFallback.targetand removing lookahead is consistent with the new IRNG interface and fallback design. Looks good.
95-100: Conditional RNG consumer update is correct (Neo)Ensures RNGWithFallback’s consumer is set to SortitionModuleNeo only when needed. Matches the one-consumer constraint.
contracts/deploy/00-rng-chainlink.ts (1)
91-98: Consumer wiring is correct and idempotentSetting the ChainlinkRNG consumer to RNGWithFallback only if needed avoids redundant transactions. This aligns with the single-consumer model.
contracts/deploy/00-rng-randomizer.ts (1)
45-51: Consumer wiring is correct and idempotentThe consumer change is only applied when necessary, which avoids redundant txs. Good.
contracts/test/rng/index.ts (12)
2-2: LGTM: Import additions for deployment functionalityThe addition of
deploymentsandgetNamedAccountsimports aligns with the new deployment-based test setup approach used throughout the file.
14-14: LGTM: Global deployer variable for test consistencyAdding the global
deployervariable enables consistent access to the deployer address across test suites.
25-31: LGTM: Transition to parameterless receiveRandomness APIThe test correctly adapts to the new IRNG interface by using
receiveRandomness()without parameters andstaticCallto read the current value without state changes. The incremental behavior is properly validated.
39-46: LGTM: Deployment-based test setup with proper configurationThe switch to using Hardhat deployments with explicit constructor arguments (governor, consumer, lookaheadTime) provides better test isolation and aligns with the new deployment patterns. The 10-second lookahead time is appropriate for testing.
48-65: LGTM: Comprehensive test for time-based randomness readinessThis test properly validates the new request-driven flow:
- Requests randomness first
- Checks readiness state before and after time advancement
- Uses
isRandomnessReady()to poll readiness status- Validates non-zero random number generation
The test flow aligns well with the BlockHashRNG's time-based readiness mechanism.
67-82: LGTM: Edge case validation for BlockHashRNGBoth new test cases properly validate the expected behavior:
- Returns 0 when no randomness has been requested
- Returns 0 when insufficient time has elapsed
These tests ensure robustness and prevent incorrect usage patterns.
89-99: LGTM: Proper test setup with consumer configurationThe setup correctly:
- Uses
getNamedAccounts()to obtain the deployer address- Configures the RNG contract with
changeConsumer(deployer)after deployment- Follows the new consumer-based authorization pattern
103-114: LGTM: Streamlined ChainlinkRNG test with parameterless APIThe test correctly adapts to the new interface:
- Uses parameterless
requestRandomness()- Uses parameterless
receiveRandomness()to get the result- Maintains proper event validation and expected value calculation using the shared
abiCoderinstance
118-144: LGTM: Multiple request handling validationThis test properly validates that only the last fulfilled randomness is returned when multiple requests are made, which aligns with the centralized fallback logic mentioned in the PR objectives. The test correctly uses the shared
abiCoderinstance for consistent expected value calculations.
152-162: LGTM: Consistent RandomizerRNG setupThe setup mirrors the ChainlinkRNG pattern with proper deployment fixture usage and consumer configuration, maintaining consistency across RNG implementations.
169-178: LGTM: RandomizerRNG test adaptationThe test correctly implements the parameterless API pattern while maintaining proper validation of the randomizer relay mechanism and expected random number generation.
182-211: LGTM: Consistent multiple request handlingThis test maintains the same validation pattern as ChainlinkRNG for multiple requests, ensuring that only the last fulfilled randomness is returned. This consistency is important for the unified IRNG interface behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/test/foundry/KlerosCore.t.sol (1)
56-56: Consider renaming for clarityThe variable name
rngLookaheadnow represents "Time in seconds" according to the comment, but the name still suggests a block-based lookahead. Consider renaming torngReadyTimeorrngWaitTimeto better reflect its timestamp-based nature.- uint256 rngLookahead; // Time in seconds + uint256 rngReadyTime; // Time in seconds
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/arbitration/SortitionModuleBase.sol(5 hunks)contracts/test/arbitration/staking-neo.ts(2 hunks)contracts/test/foundry/KlerosCore.t.sol(35 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/test/arbitration/staking-neo.ts
- 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). (2)
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
contracts/test/foundry/KlerosCore.t.sol (4)
1020-1021: Consistent timestamp-based RNG readiness patternThe pattern of using
vm.warp(block.timestamp + rngLookahead)after callingpassPhase()to reach the Generating phase is correctly applied throughout the test file. This aligns with the timestamp-based RNG readiness implementation.Also applies to: 1067-1068, 1094-1095, 1150-1151, 1438-1439, 1474-1475, 1517-1518, 1560-1561, 1670-1671, 1695-1696, 1806-1807, 1833-1834, 1874-1875, 1923-1924, 2013-2014, 2057-2058, 2158-2159, 2236-2237, 2270-2271, 2282-2283, 2388-2389, 2463-2464, 2509-2510, 2564-2565, 2654-2655, 2708-2709, 2747-2748, 2801-2802, 2843-2844, 2943-2944
112-119: Simplified SortitionModule initializationThe SortitionModule initialization correctly uses the shortened signature without the
rngLookaheadparameter, passing only therngaddress. This aligns with the PR's changes to remove lookahead from interfaces.
123-124: Consumer linkage after proxy creationGood practice to set the RNG consumer after the sortition module proxy is created. This ensures the correct address is registered.
299-306: Consistent initialization pattern in events testThe initialization pattern in the events test correctly mirrors the main setup, maintaining consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
contracts/test/foundry/KlerosCore.t.sol (2)
99-101: BlockHashRNG constructed with uninitialized consumer (still address(0)) — make intent explicitYou’re passing
address(sortitionModule)beforesortitionModuleis assigned. While you correct it later viarng.changeConsumer(...), this is brittle and obscures intent.Make it explicit that the consumer is initially unset to avoid confusion and future breakage if constructor semantics change.
Apply this diff:
- rngLookahead = 30; - rng = new BlockHashRNG(msg.sender, address(sortitionModule), rngLookahead); + rngLookahead = 30; + // sortitionModule is not initialized yet; set consumer explicitly after proxy creation. + rng = new BlockHashRNG(msg.sender, address(0), rngLookahead);
286-288: Same constructor-ordering issue for BlockHashRNG in this test caseSame as earlier:
address(sortitionModule)is stilladdress(0)here. Make the zero-consumer intent explicit and assign the real consumer after proxy initialization.- rngLookahead = 20; - rng = new BlockHashRNG(msg.sender, address(sortitionModule), rngLookahead); + rngLookahead = 20; + // sortitionModule will be set as consumer after initialization + rng = new BlockHashRNG(msg.sender, address(0), rngLookahead);
🧹 Nitpick comments (7)
contracts/src/test/RNGMock.sol (4)
6-9: Nit: tighten comment and clarify 0-as-failure semanticsMinor: trailing semicolon in the inline comment on Line 8. Also consider noting that 0 may be used as a failure sentinel by consumers (per SM.receiveRandomness behavior) to avoid confusion in tests.
Apply this small doc fix:
- uint256 public randomNumber; // The number to return; + uint256 public randomNumber; // The number to return
10-12: Prefer clearer API and add an event for traceability in testsConsider renaming setRN to setRandomNumber and emitting an event to make test traces clearer and assertions easier.
Within these lines:
- function setRN(uint256 _rn) external { - randomNumber = _rn; - } + function setRandomNumber(uint256 rn) external { + randomNumber = rn; + emit RandomNumberSet(rn); + }Additionally add the event declaration near the state variable (outside the selected lines):
// Place below the state variable declaration event RandomNumberSet(uint256 rn);Note: If renaming would churn too many tests, keep setRN and alias the new name:
function setRN(uint256 rn) external { setRandomNumber(rn); }
14-14: Emit an event on requestRandomness for observabilityNo-op is fine, but emitting an event helps verify calls in tests without relying on side effects.
Apply:
- function requestRandomness() external override {} + function requestRandomness() external override { + emit RandomnessRequested(); + }And add this event declaration (outside the selected lines, near other events):
event RandomnessRequested();
16-18: Nit: name the return value for readabilityNaming the return value makes traces slightly clearer and aligns with common NatSpec patterns.
- function receiveRandomness() external view override returns (uint256) { - return randomNumber; - } + function receiveRandomness() external view override returns (uint256 rn) { + rn = randomNumber; + }contracts/test/foundry/KlerosCore.t.sol (3)
115-121: Nit: passaddress(rng)to ABI encoding for clarityThe signature expects an
address. Passing the contract type works due to implicit conversion, but being explicit improves readability and avoids confusion with function overloads.- bytes memory initDataSm = abi.encodeWithSignature( - "initialize(address,address,uint256,uint256,address)", - governor, - address(proxyCore), - minStakingTime, - maxDrawingTime, - rng - ); + bytes memory initDataSm = abi.encodeWithSignature( + "initialize(address,address,uint256,uint256,address)", + governor, + address(proxyCore), + minStakingTime, + maxDrawingTime, + address(rng) + );Repeat similarly in the later initialization.
Also applies to: 301-309
3058-3062: Consider asserting request lifecycle reset on success pathAfter a successful fulfillment path, it’s helpful to assert that
requestTimestamp()resets (or otherwise reflects a non-pending state), to guard against future regressions that leave the RNG permanently “busy”.rngMock.setRN(123); sortitionModule.passPhase(); // Drawing phase assertEq(sortitionModule.randomNumber(), 123, "Wrong random number"); + // Optional: ensure no request is left pending after success. + assertEq(rngFallback.requestTimestamp(), 0, "Request timestamp should reset after fulfillment");
1235-1243: Use uint256 for loop variable (consistency and best practice)The codebase predominantly uses
uint256. Align here for consistency and to avoid subtle type promotion mismatches.- for (uint i = 2; i <= sortitionModule.delayedStakeWriteIndex(); i++) { + for (uint256 i = 2; i <= sortitionModule.delayedStakeWriteIndex(); i++) {
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
contracts/src/test/RNGMock.sol(1 hunks)contracts/test/foundry/KlerosCore.t.sol(37 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-07T11:39:10.927Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Applied to files:
contracts/src/test/RNGMock.solcontracts/test/foundry/KlerosCore.t.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (7)
contracts/src/test/RNGMock.sol (3)
1-5: LGTM on SPDX, pragma, and import pathLicense header, compiler version (^0.8.24), and the relative import path look correct for this repo layout.
6-19: Minimal, purpose-fit test double — looks goodSimple, deterministic mock implementing IRNG for fallback tests is appropriate. With optional events, this becomes even easier to assert against without adding state complexity.
7-18: IRNG interface coverage confirmed
The IRNG interface incontracts/src/rng/IRNG.soldeclares only
function requestRandomness() external;function receiveRandomness() external returns (uint256);and
RNGMockimplements both with matchingexternalvisibility. The addedviewonreceiveRandomnessis permissible since it only reads state. No additional stubs are required for compilation.contracts/test/foundry/KlerosCore.t.sol (4)
125-127: Good: consumer set after SortitionModule is deployedSetting the consumer post-proxy initialization under governor privileges ensures the RNG is properly permissioned and avoids having to re-deploy RNG. This wiring is clean.
2998-3035: Verified RNG fallback event and blockhash source – test is correctBoth assumptions in the test are confirmed in RNGWithFallback.sol:
- The contract declares and emits
event RNGFallback()when falling back.- It falls back to
uint256(blockhash(block.number - 1))as the random source.No changes needed.
3070-3105: Revert checks correctly match string-based requiresThe RNGWithFallback contract uses
•require(msg.sender == governor, "Governor only")(line 48)
•require(msg.sender == consumer, "Consumer only")(line 53)No custom errors are defined, so asserting exact string messages with
vm.expectRevert(bytes("Consumer only")); vm.expectRevert(bytes("Governor only"));is correct and need not be changed.
1022-1023: No off-by-one for RNG lookahead – ready check uses ≥We verified in contracts/src/rng/BlockhashRNG.sol that readiness is checked with
return block.timestamp >= requestTimestamp + lookaheadTime;so warping to exactly
block.timestamp + rngLookaheadsatisfies the condition.
RNGWithFallback’s strict “>” only applies to its fallback timeout, which isn’t used by these lookahead-based tests. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/rng/BlockhashRNG.sol (1)
1-3: Fix RNG filename casing to match contract nameThe file
contracts/src/rng/BlockhashRNG.sol(lowercase “h”) definescontract BlockHashRNGand is imported in tests asBlockHashRNG.sol, causing a mismatch on case-sensitive filesystems.Please:
- Rename
contracts/src/rng/BlockhashRNG.sol→contracts/src/rng/BlockHashRNG.sol- Ensure any imports referencing this file use the updated path. (Current tests already import
BlockHashRNG.sol.)This will align the filesystem name with the contract and import statements, preventing build failures on Linux/macOS.
♻️ Duplicate comments (4)
contracts/src/rng/BlockhashRNG.sol (2)
58-66: Add zero-address checks in governance settersPrevent accidentally bricking the contract by setting governor/consumer to address(0).
function changeGovernor(address _governor) external onlyByGovernor { - governor = _governor; + require(_governor != address(0), "Governor cannot be zero address"); + governor = _governor; } function changeConsumer(address _consumer) external onlyByGovernor { - consumer = _consumer; + require(_consumer != address(0), "Consumer cannot be zero address"); + consumer = _consumer; }If you intentionally want to support “disabling” the consumer (set to 0), keep the consumer check relaxed and add a NatSpec note documenting that behavior.
46-50: Validate governor in constructor (allow consumer to be zero for wiring)Guard against accidental zero governor. Keep consumer unvalidated to support the test wiring pattern (set later via changeConsumer).
Suggested patch:
constructor(address _governor, address _consumer, uint256 _lookaheadTime) { - governor = _governor; + require(_governor != address(0), "Governor cannot be zero address"); + governor = _governor; consumer = _consumer; lookaheadTime = _lookaheadTime; }Note: If you also want to forbid lookaheadTime = 0, add
require(_lookaheadTime > 0, "lookaheadTime must be > 0");. Given the current design, zero could be used to allow “immediate” readiness; decide based on intended semantics.contracts/test/foundry/KlerosCore.t.sol (2)
99-101: Initialize BlockHashRNG with a zero consumer to avoid passing an uninitialized addressAt this point, sortitionModule is not yet set, so address(sortitionModule) is zero. You already rebind the consumer later; make that intent explicit.
- rngLookahead = 30; - rng = new BlockHashRNG(msg.sender, address(sortitionModule), rngLookahead); + rngLookahead = 30; + // sortitionModule will be set as consumer after initialization + rng = new BlockHashRNG(msg.sender, address(0), rngLookahead);
286-288: Do the same explicit zero-consumer init here for consistency and claritySame rationale as above; avoid implying a valid consumer at construction time.
- rngLookahead = 20; - rng = new BlockHashRNG(msg.sender, address(sortitionModule), rngLookahead); + rngLookahead = 20; + // sortitionModule will be set as consumer after initialization + rng = new BlockHashRNG(msg.sender, address(0), rngLookahead);
🧹 Nitpick comments (16)
contracts/src/rng/RandomizerRNG.sol (9)
26-34: Fix VRF wording and align event parameter naming (singular vs plural)Comments still reference “VRF Coordinator” and the event arg is named “randomWords” while only one value is emitted.
Apply:
- /// @dev Emitted when a request is sent to the VRF Coordinator + /// @dev Emitted when a request is sent to the Randomizer.ai oracle @@ - /// @param randomWords The random values answering the request. - event RequestFulfilled(uint256 indexed requestId, uint256 randomWords); + /// @param randomWord The random value answering the request. + event RequestFulfilled(uint256 indexed requestId, uint256 randomWord);Note: renaming the event parameter is ABI-name-only; types and indexed fields don’t change, but adjust off-chain consumers if they rely on the field name.
53-62: Optional: Guard against zero-addresses in constructorPrevent accidental bricking by validating _governor, _consumer and _randomizer.
Apply:
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 ZeroAddress(); + governor = _governor; + consumer = _consumer; + randomizer = _randomizer; callbackGasLimit = 50000; }And add this error in the Errors section:
error RandomizerOnly(); +error ZeroAddress();
68-73: Emit event when governor changesEmitting events for on-chain configuration changes improves observability and off-chain indexing.
First, declare the event (see separate comment for event declarations). Then:
function changeGovernor(address _governor) external onlyByGovernor { - governor = _governor; + address prev = governor; + governor = _governor; + emit GovernorChanged(prev, _governor); }
74-78: Emit event when consumer changesTrack consumer changes for auditing and off-chain services.
function changeConsumer(address _consumer) external onlyByGovernor { - consumer = _consumer; + if (_consumer == address(0)) revert ZeroAddress(); + address prev = consumer; + consumer = _consumer; + emit ConsumerChanged(prev, _consumer); }
80-85: Emit event when callback gas limit changesExpose gas limit changes to indexers/dashboards.
function setCallbackGasLimit(uint256 _callbackGasLimit) external onlyByGovernor { - callbackGasLimit = _callbackGasLimit; + uint256 prev = callbackGasLimit; + callbackGasLimit = _callbackGasLimit; + emit CallbackGasLimitSet(prev, _callbackGasLimit); }
86-90: Emit event and validate address when randomizer changesAvoid setting a zero randomizer and emit an event for traceability.
function setRandomizer(address _randomizer) external onlyByGovernor { - randomizer = IRandomizer(_randomizer); + if (_randomizer == address(0)) revert ZeroAddress(); + address prev = address(randomizer); + randomizer = IRandomizer(_randomizer); + emit RandomizerChanged(prev, _randomizer); }
26-34: Add events for governance/config changesAdd these to the Events section to support the previous suggestions.
event RequestSent(uint256 indexed requestId); +event GovernorChanged(address indexed previousGovernor, address indexed newGovernor); +event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer); +event RandomizerChanged(address indexed previousRandomizer, address indexed newRandomizer); +event CallbackGasLimitSet(uint256 previousLimit, uint256 newLimit);
112-116: Optionally ignore stale callbacks and avoid double castIf governance can re-request, older callbacks can be ignored to save a write and event. Also, avoid casting twice.
function randomizerCallback(uint256 _id, bytes32 _value) external { if (msg.sender != address(randomizer)) revert RandomizerOnly(); - randomNumbers[_id] = uint256(_value); - emit RequestFulfilled(_id, uint256(_value)); + if (_id != lastRequestId) { + // Optional: ignore stale callback for an invalidated request + return; + } + uint256 rnd = uint256(_value); + randomNumbers[_id] = rnd; + emit RequestFulfilled(_id, rnd); }Note: If you rely on storing historical results per id for diagnostics, keep the current behavior.
128-133: Errors section: add ZeroAddress and (optionally) RequestAlreadyInProgress if you adopt the guardsKeep error taxonomy consistent and near usage sites.
error RandomizerOnly(); +error ZeroAddress(); +// error RequestAlreadyInProgress(); // uncomment if you add a concurrency guardcontracts/src/rng/BlockhashRNG.sol (2)
95-101: Nit: guard against underflow and document zero-return edge case
blockhash(block.number - 1)is fine in practice but can underflow on block 0 (dev nets) and will return 0 on some L2s sporadically. We already return 0 to signal “not available,” but an explicit guard makes behavior clearer.- // Use last block hash for randomness - randomNumber = uint256(blockhash(block.number - 1)); + // Use last block hash for randomness + if (block.number == 0) { + return 0; + } + randomNumber = uint256(blockhash(block.number - 1));
10-13: Clarify NatSpec wording about “retrieving blockhash after the time window”The contract caches the computed random number after the lookahead window; it does not retrieve the historical blockhash tied to the original request’s time. Suggest rewording to avoid confusion.
Proposed wording:
- “Allows caching the computed random number for future use (so subsequent calls can retrieve the same value even after the blockhash window).”
contracts/src/rng/ChainlinkRNG.sol (5)
58-82: Add basic constructor sanity checks for non-zero critical addressesOptional hardening: guard against accidental zero addresses for governor, consumer, and coordinator. Keeps the contract in a valid state from deployment.
- ) VRFConsumerBaseV2Plus(_vrfCoordinator) { - governor = _governor; + ) VRFConsumerBaseV2Plus(_vrfCoordinator) { + require(_governor != address(0), "governor=0"); + require(_consumer != address(0), "consumer=0"); + require(_vrfCoordinator != address(0), "coordinator=0"); + governor = _governor; consumer = _consumer; keyHash = _keyHash; subscriptionId = _subscriptionId; requestConfirmations = _requestConfirmations; callbackGasLimit = _callbackGasLimit; }If you prefer custom errors over strings, I can propose corresponding error types and apply them consistently across the file.
94-99: Emit events and zero-check on consumer/governor changesEmitting change events improves off-chain observability. Also consider guarding against setting the consumer to the zero address unless intentional.
Add events in the events section:
- event RequestFulfilled(uint256 indexed _requestId, uint256 _randomWord); + event RequestFulfilled(uint256 indexed _requestId, uint256 _randomWord); + event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer); + event GovernorChanged(address indexed previousGovernor, address indexed newGovernor);Emit them on change:
function changeGovernor(address _governor) external onlyByGovernor { - governor = _governor; + address old = governor; + governor = _governor; + emit GovernorChanged(old, _governor); }function changeConsumer(address _consumer) external onlyByGovernor { - consumer = _consumer; + require(_consumer != address(0), "consumer=0"); + address old = consumer; + consumer = _consumer; + emit ConsumerChanged(old, _consumer); }
135-153: Make nativePayment configurable (chain-dependent)nativePayment: true is great on chains where VRF v2.5 supports native token payments (e.g., Arbitrum). On other deployments, LINK payment may be required. Make it governance-configurable to avoid redeploys.
Add a state flag (packs well with existing slot):
uint16 public requestConfirmations; // How many confirmations the Chainlink node should wait before responding. - // 22 bytes remaining in slot + // 21 bytes remaining in slot + bool public nativePayment; // Pay in native token (true) vs LINK (false). uint32 public callbackGasLimit; // Gas limit for the Chainlink callback.Initialize in the constructor:
) VRFConsumerBaseV2Plus(_vrfCoordinator) { + nativePayment = true; // default to native; can be toggled by governor governor = _governor; consumer = _consumer;Use it in the request:
- extraArgs: VRFV2PlusClient._argsToBytes( - // Set nativePayment to true to pay for VRF requests with ETH instead of LINK - VRFV2PlusClient.ExtraArgsV1({nativePayment: true}) - ) + extraArgs: VRFV2PlusClient._argsToBytes( + VRFV2PlusClient.ExtraArgsV1({nativePayment: nativePayment}) + )Add a governor-controlled setter (outside the shown range):
function changeNativePayment(bool _nativePayment) external onlyByGovernor { nativePayment = _nativePayment; }
158-162: Ignore stale fulfillments to avoid writing irrelevant stateIf governance or the consumer re-requests before fulfillment, older callbacks will arrive later and are irrelevant. Short-circuiting stale fulfillments prevents unnecessary writes and ambiguity.
- function fulfillRandomWords(uint256 _requestId, uint256[] calldata _randomWords) internal override { - // Access control is handled by the parent VRFCoordinator.rawFulfillRandomWords() - randomNumbers[_requestId] = _randomWords[0]; - emit RequestFulfilled(_requestId, _randomWords[0]); - } + function fulfillRandomWords(uint256 _requestId, uint256[] calldata _randomWords) internal override { + // Access control is handled by the parent VRFCoordinator.rawFulfillRandomWords() + if (_requestId != lastRequestId) return; + randomNumbers[_requestId] = _randomWords[0]; + emit RequestFulfilled(_requestId, _randomWords[0]); + }
17-26: Action: Bound on-chain storage growth for RNG mappingCurrently, randomNumbers grows unbounded, yet IRNG only ever needs the latest value (concurrency isn’t supported). You have two paths:
Minimal change—keep the mapping but delete the previous entry on each fulfill:
• contracts/src/rng/ChainlinkRNG.sol:- function fulfillRandomWords(uint256 _requestId, uint256[] calldata _randomWords) internal override { - randomNumbers[_requestId] = _randomWords[0]; - emit RequestFulfilled(_requestId, _randomWords[0]); - } + function fulfillRandomWords(uint256 _requestId, uint256[] calldata _randomWords) internal override { + // Keep mapping size at one entry + uint256 prevId = lastRequestId; + if (prevId != 0) { + delete randomNumbers[prevId]; + } + randomNumbers[_requestId] = _randomWords[0]; + lastRequestId = _requestId; + emit RequestFulfilled(_requestId, _randomWords[0]); + }This caps storage to one mapping slot without breaking existing consumers.
Full refactor—replace the mapping with lastRandomNumber and update all consumers:
• Remove:- mapping(uint256 => uint256) public randomNumbers;• Add:
+ uint256 public lastRandomNumber;• Change fulfillRandomWords to set lastRandomNumber (and ignore stale requests).
• Change receiveRandomness() to return lastRandomNumber.
• Update these script calls:
- contracts/scripts/keeperBot.ts (around line 261):
replaceawait chainlinkRng.randomNumbers(requestID)→await chainlinkRng.receiveRandomness()- contracts/scripts/console-init.ts (around line 153):
replaceawait rng.randomNumbers(requesterID)→await rng.receiveRandomness()Either approach prevents unbounded state growth; choose based on how much refactoring you’re willing to undertake.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
contracts/src/rng/BlockhashRNG.sol(1 hunks)contracts/src/rng/ChainlinkRNG.sol(7 hunks)contracts/src/rng/IRNG.sol(1 hunks)contracts/src/rng/RNGWithFallback.sol(1 hunks)contracts/src/rng/RandomizerRNG.sol(7 hunks)contracts/test/foundry/KlerosCore.t.sol(37 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/src/rng/IRNG.sol
- contracts/src/rng/RNGWithFallback.sol
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-07T11:39:10.927Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Applied to files:
contracts/src/rng/BlockhashRNG.solcontracts/src/rng/RandomizerRNG.solcontracts/src/rng/ChainlinkRNG.solcontracts/test/foundry/KlerosCore.t.sol
📚 Learning: 2024-12-07T11:41:15.835Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
Applied to files:
contracts/src/rng/RandomizerRNG.solcontracts/src/rng/ChainlinkRNG.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). (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
contracts/src/rng/RandomizerRNG.sol (4)
5-10: IRNG migration and consumer-based API adoption look goodContract now implements IRNG and removes the block parameter, aligning with the PR’s new interface and consumer model.
122-126: receiveRandomness implementation is consistent with IRNG expectationsReturns 0 until the last request has been fulfilled; simple and correct.
102-107: No concurrent randomness requests: guard enforced in SortitionModule.passPhaseThe
SortitionModuleorchestrates RNG calls via itspassPhase()flow—only transitioning from staking to generating once per dispute batch and back—sorequestRandomness()can’t be reentered until the previous request is fulfilled. Since only theSortitionModule(viaonlyByConsumer) invokesRandomizerRNG.requestRandomness, no additional guard is needed here.
39-46: GovernorOnly and ConsumerOnly are declared — no change requiredFound both declarations in:
- contracts/src/rng/IRNG.sol:14 — error GovernorOnly();
- contracts/src/rng/IRNG.sol:15 — error ConsumerOnly();
contracts/test/foundry/KlerosCore.t.sol (2)
1023-1023: LGTM: timestamp-driven pacing replaces block-number rolls cleanlySwitching from vm.roll to vm.warp with rngLookahead consistently aligns with the new IRNG semantics and Arbitrum’s timestamp-based readiness.
Also applies to: 1069-1069, 1097-1097, 1153-1153, 1441-1441, 1476-1476, 1520-1520, 1563-1563, 1809-1809, 1877-1877, 193-193
2998-3062: RNG fallback path test looks solidHappy-path and timeout fallback are covered, including event assertions and permission checks. After fixing the IRNG import, these tests should compile and exercise the intended behavior end-to-end.
Optional: After adjusting imports, run Foundry tests locally to ensure selectors (IRNG.ConsumerOnly, IRNG.GovernorOnly) resolve correctly.
contracts/src/rng/ChainlinkRNG.sol (5)
8-13: IRNG migration + VRF v2.5 base adoption looks correctThe inheritance switch to IRNG and VRFConsumerBaseV2Plus, plus the import of IRNG, align with the PR objectives and the new consumer-based API.
31-39: Event schema changes look goodUnderscored parameter names and event emissions for request/fulfillment are consistent and helpful for off-chain indexing.
170-172: receiveRandomness semantics match IRNG expectationReturning 0 while pending and the actual value once fulfilled is consistent with the new interface.
44-52: Custom Errors Verified in IRNG.solThe
GovernorOnlyandConsumerOnlyerrors are declared in contracts/src/rng/IRNG.sol (lines 14–15), so these modifiers will compile successfully. No changes required.
100-106: CoordinatorSet event is inherited and already declared upstreamThe
CoordinatorSetevent is defined in theIVRFMigratableConsumerV2Plusinterface, whichVRFConsumerBaseV2Plusinherits. EmittingCoordinatorSet(_vrfCoordinator)inChainlinkRNG.solis therefore valid and does not require a redeclaration or a new event.
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)
167-173: Apply CEI to prevent re-entrant double-request of RNGCurrently requestRandomness() is called before changing phase to generating. A re-entrant RNG could re-enter passPhase while phase is still staking and trigger a second request, contradicting the “no concurrency” invariant stated in this PR.
Set the phase before the external call to enforce single-flight requests even under re-entrancy. If requestRandomness() reverts, the phase change reverts too.
Apply this diff:
if (phase == Phase.staking) { if (block.timestamp - lastPhaseChange < minStakingTime) revert MinStakingTimeNotPassed(); if (disputesWithoutJurors == 0) revert NoDisputesThatNeedJurors(); - rng.requestRandomness(); - phase = Phase.generating; + phase = Phase.generating; + rng.requestRandomness();Optional defense-in-depth: make passPhase nonReentrant if you anticipate future callbacks or RNG upgrades that could introduce re-entrancy.
♻️ Duplicate comments (3)
contracts/test/foundry/KlerosCore.t.sol (3)
99-101: BlockHashRNG constructed with uninitialized sortitionModule (consumer ends up as address(0)).At this point, sortitionModule hasn’t been assigned yet, so address(sortitionModule) is address(0). You correctly fix the consumer right after initialization via changeConsumer(), but passing an erroneous consumer in the constructor is misleading and may violate potential non-zero checks.
Safer to construct with a null consumer explicitly:
- rngLookahead = 30; - rng = new BlockHashRNG(msg.sender, address(sortitionModule), rngLookahead); + rngLookahead = 30; + rng = new BlockHashRNG(msg.sender, address(0), rngLookahead);The subsequent rng.changeConsumer(address(sortitionModule)) is fine to keep.
286-288: Second occurrence: BlockHashRNG constructed before sortitionModule is set.Same issue as above; address(sortitionModule) is still zero here. You update it later via changeConsumer(), but the constructor consumer is wrong.
Apply:
- rngLookahead = 20; - rng = new BlockHashRNG(msg.sender, address(sortitionModule), rngLookahead); + rngLookahead = 20; + rng = new BlockHashRNG(msg.sender, address(0), rngLookahead);
15-16: Fix invalid IRNG import (will not compile as-is).IRNG is defined in IRNG.sol, not RNGWithFallback.sol. Importing IRNG from RNGWithFallback will fail.
Apply this diff:
-import {RNGWithFallback, IRNG} from "../../src/rng/RNGWithFallback.sol"; -import {RNGMock} from "../../src/test/RNGMock.sol"; +import {RNGWithFallback} from "../../src/rng/RNGWithFallback.sol"; +import {IRNG} from "../../src/rng/IRNG.sol"; +import {RNGMock} from "../../src/test/RNGMock.sol";
🧹 Nitpick comments (6)
contracts/test/foundry/KlerosCore.t.sol (2)
115-121: Nit: encode rng as address explicitly for readability.abi.encodeWithSignature specifies the last arg as address; passing rng (a contract type) relies on implicit conversion. Casting explicitly can improve clarity.
Example:
- rng + address(rng)Also applies to: 302-308
3077-3101: Optional: strengthen idempotency coverage for passPhase() under RNG fallback.Given the PR goal to prevent repeated RNG requests on repeated passPhase() calls, consider asserting that a second passPhase() during Generating does not change requestTimestamp and does not emit any additional RNG request/fallback events.
You could add after the first Generating transition:
- Call passPhase() again immediately and assert it reverts with RandomNumberNotReady and that rngFallback.requestTimestamp() remains unchanged.
contracts/src/arbitration/SortitionModuleBase.sol (4)
53-58: Deprecated storage: keep for layout compatibility, but plan removal windowKeeping randomNumberRequestBlock and rngLookahead as DEPRECATED is fine for proxy layout stability. Two suggestions:
- Document the removal plan and version gate in the deployment checklist.
- Optionally convert these into a reserved storage gap on the next redeploy to avoid future churn.
102-116: Guard against zero-address RNG in initializeA zero RNG bricks passPhase at runtime. Add a simple guard in the initializer.
Apply this diff:
function __SortitionModuleBase_initialize( @@ - ) internal onlyInitializing { + ) internal onlyInitializing { + require(address(_rng) != address(0), "RNG address cannot be zero"); governor = _governor; core = _core; minStakingTime = _minStakingTime; maxDrawingTime = _maxDrawingTime; lastPhaseChange = block.timestamp; rng = _rng; delayedStakeReadIndex = 1; }
154-160: Harden RNG changes: zero-address guard + no-op if same provider (optional: emit event)To avoid footguns and noisy transactions:
- Reject address(0).
- No-op if the new RNG equals the current one.
- Optionally emit an event for off-chain monitoring.
Apply this diff:
function changeRandomNumberGenerator(IRNG _rng) external onlyByGovernor { - rng = _rng; - if (phase == Phase.generating) { - rng.requestRandomness(); - } + if (address(_rng) == address(0)) revert InvalidRNGAddress(); + if (_rng == rng) return; + IRNG old = rng; + rng = _rng; + if (phase == Phase.generating) { + rng.requestRandomness(); + } + // Optional: track changes on-chain for indexers / dashboards + // emit RNGChanged(address(old), address(_rng)); }Add these outside the shown range:
// Events (put near other events) event RNGChanged(address indexed oldRNG, address indexed newRNG); // Errors (put near other errors) error InvalidRNGAddress();
174-176: Avoid unnecessary SSTORE on the “not ready” pathDeferring the storage write until after validating the returned value saves one SSTORE when the random number isn’t ready.
• File contracts/src/arbitration/SortitionModuleBase.sol, around line 174
Replace:- randomNumber = rng.receiveRandomness(); - if (randomNumber == 0) revert RandomNumberNotReady(); + uint256 rn = rng.receiveRandomness(); + if (rn == 0) revert RandomNumberNotReady(); + randomNumber = rn;• All IRNG implementations have been checked and correctly return
0(not revert) when randomness isn’t ready—no changes needed there.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
contracts/src/arbitration/SortitionModuleBase.sol(5 hunks)contracts/test/foundry/KlerosCore.t.sol(37 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-07T11:39:10.927Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Applied to files:
contracts/src/arbitration/SortitionModuleBase.solcontracts/test/foundry/KlerosCore.t.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (6)
contracts/test/foundry/KlerosCore.t.sol (4)
58-58: Good migration to time-based pacing (vm.warp) with rngLookahead seconds.Switching from block-number gating to timestamp-based gating lines up with Arbitrum semantics and the PR objectives. The use of rngLookahead as seconds is consistent throughout.
Also applies to: 1023-1023, 1069-1069, 1096-1096, 1153-1153, 1441-1441, 1477-1477, 1520-1520, 1563-1563, 1674-1674, 1699-1699, 1810-1810, 1837-1837, 1878-1878, 1927-1927, 2017-2017, 2061-2061, 2162-2162, 2240-2240, 2274-2274, 2286-2286, 2415-2415, 2505-2505, 2551-2551, 2606-2606, 2696-2696, 2749-2749, 2985-2985
3067-3074: Fallback path behavior looks correct and is well asserted.Rolling a few blocks before the fallback and asserting RNGFallback emission plus blockhash-based RN is solid. Nice coverage.
99-101: BlockHashRNG lookahead is in seconds, tests are correctBlockHashRNG’s constructor and documentation explicitly treat the third parameter as a time lookahead in seconds (not blocks), and its readiness check uses
block.timestamp. Your Foundry tests usevm.warpto advance the timestamp byrngLookahead, which satisfies the contract’s gating logic. No additionalvm.rollor block‐number advances are needed.
3049-3051: Ignore IRNG cast suggestion
ThechangeRandomNumberGeneratorfunction inSortitionModuleBase.solexpects theRNGinterface (not an undefinedIRNG), along with auint256lookahead. TheRNGWithFallbackcontract implementsRNG, so passingrngFallbackdirectly is already type-correct under Solidity’s implicit conversions. You can safely remove the cast recommendation toIRNG.Likely an incorrect or invalid review comment.
contracts/src/arbitration/SortitionModuleBase.sol (2)
10-10: IRNG migration import looks goodSwitching to IRNG aligns with the PR’s interface refactor and has no storage-layout impact.
55-55: Public rng type migration is soundIRNG public rng is a drop-in replacement at the storage level (address), so proxy layout is preserved.
|
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
contracts/CHANGELOG.md (5)
14-14: Name the mediator (RNGWithFallback) and the timeout basis for precision.Explicitly mentioning RNGWithFallback and that the timeout is measured with block.timestamp improves clarity.
-- Make the primary VRF-based RNG fall back to `BlockhashRNG` if the VRF request is not fulfilled within a timeout ([#2054](https://github.com/kleros/kleros-v2/issues/2054)) +- Introduce `RNGWithFallback`, which falls back from the primary VRF-based RNG to `BlockhashRNG` if a request is not fulfilled before a timeout (measured with `block.timestamp`) ([#2054](https://github.com/kleros/kleros-v2/issues/2054)) + - RNGs are single-consumer and do not support concurrent requests; governance-triggered re-requests invalidate previous ones ([#2054](https://github.com/kleros/kleros-v2/issues/2054))
15-15: Avoid “3rd”, clarify mechanism and scope.Spell out “third” and briefly describe the auth mechanism (consumer allowlist). If it applies beyond Chainlink, consider broadening the wording.
-- 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 (restrict to an allowlisted consumer) to prevent third parties from depleting Chainlink VRF subscription funds ([#2054](https://github.com/kleros/kleros-v2/issues/2054))
16-16: Clarify timestamp usage to avoid implying it impacts randomness quality.Suggest noting that timestamp is used for readiness/timeout checks; randomness remains blockhash-based.
-- Use `block.timestamp` rather than `block.number` for `BlockhashRNG` for better reliability on Arbitrum as block production is sporadic depending on network conditions. ([#2054](https://github.com/kleros/kleros-v2/issues/2054)) +- Use `block.timestamp` rather than `block.number` in `BlockhashRNG` readiness/timeout checks for better reliability on Arbitrum where block production is sporadic; randomness remains based on `blockhash`. ([#2054](https://github.com/kleros/kleros-v2/issues/2054))
12-12: Optional: note file removal for the rename.Minor clarity tweak: mention RNG.sol removal alongside the rename.
-- **Breaking:** Rename the interface from `RNG` to `IRNG` ([#2054](https://github.com/kleros/kleros-v2/issues/2054)) +- **Breaking:** Rename interface `RNG` to `IRNG` (remove `RNG.sol`) ([#2054](https://github.com/kleros/kleros-v2/issues/2054))
13-13: Cross-reference consumer changes and internal tracking.Tie the param removal to consumer updates and BlockhashRNG’s internal tracking to guide upgraders.
-- **Breaking:** Remove the `_block` parameter from `IRNG.requestRandomness()` and `IRNG.receiveRandomness()`, not needed for the primary VRF-based RNG ([#2054](https://github.com/kleros/kleros-v2/issues/2054)) +- **Breaking:** Remove the `_block` parameter from `IRNG.requestRandomness()` and `IRNG.receiveRandomness()`; API is consumer-based and `BlockhashRNG` now tracks its request timestamp internally. Consumers (e.g., SortitionModule, RNG implementations) updated accordingly. ([#2054](https://github.com/kleros/kleros-v2/issues/2054))
📜 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 (1)
contracts/CHANGELOG.md(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). (3)
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (1)
contracts/CHANGELOG.md (1)
12-16: Docs align with PR intent; nice and clear.These bullets accurately reflect the IRNG rename, signature changes, fallback behavior, auth, and timestamp rationale.
✅ Actions performedComments resolved and changes approved. |
This PR is a redo of #966 and #1782 as they had deviated too much from dev. It was originally based atop the
feat/chainlink-vrf-as-rng2branch (PR #1778).This PR introduces a
RNGWithFallbackresponsible for the fallback logic.We ensure that repeated calls to
SM.passPhase()does not result in repeated calls to the default RNG or the fallback RNG as it would keep overwriting therequestIDfor VRF-based RNGs or therequestBlockfor the BlockhashRNG.The RNGs are permissioned to prevent 3rd parties from using up the funds on the RNG oracle accounts with Chainlink or Randomizer.
The RNGs do not support concurrent requests, it is not necessary since only 1 consumer is allowed (the sortition module), with the notable exception of some governance function triggering a re-request, which invalidates the previous one.
We rely on
block.timestamprather thanblock.numberbecause it is a more reliable measure of the elapsed time on Arbitrum, as block production is sporadic depending on network conditions.ABI changes
RNGinterface renamedIRNG_blockparameter from theIRNGinterface which is not necessary for VRFs, only for the BlockHashRNG which now tracks it internallyPR-Codex overview
This PR focuses on refactoring the random number generator (RNG) implementation in the Kleros project, introducing a new interface
IRNG, and enhancing the RNG architecture with fallback mechanisms.Detailed summary
RNG.soland related deployment scripts.IRNGinterface for RNG functionalities.RNGWithFallbackcontract for fallback mechanisms.IncrementalNG,ChainlinkRNG, andRandomizerRNGto implementIRNG.Summary by CodeRabbit
New Features
Refactor
Chores
Tests
Documentation