fix(Proxies): reviews #67
Conversation
WalkthroughThis pull request updates several Realitio cross-chain proxy contracts. In the foreign proxies (Arbitrum, Optimism, and ZkSync), the visibility of key arbitration functions is changed from public to external, and the relay logic is adjusted to allow multiple relays. Additionally, a new state variable Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code Definitions (2)contracts/deploy/02-foreign-proxy.js (1)
contracts/deploy/foreign/arbitrum.js (1)
🔇 Additional comments (15)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
contracts/deploy/foreign/arbitrum.js (1)
38-38: Missing comma at the end of the arrayAccording to the ESLint hint, a comma is missing after the array.
- [l2GasLimit, gasPriceBid] + [l2GasLimit, gasPriceBid],🧰 Tools
🪛 ESLint
[error] 38-38: Insert
,(prettier/prettier)
contracts/src/0.8/libraries/SafeSend.sol (2)
17-24: Consider adding error handling for the WETH transferWhile the fallback mechanism is good, there's no error handling if the WETH transfer fails. This could silently mask issues.
function safeSend(address payable _to, uint256 _value, address _wethLike) internal { if (_to.send(_value)) return; WethLike(_wethLike).deposit{value: _value}(); - WethLike(_wethLike).transfer(_to, _value); + bool success = address(WethLike(_wethLike)).call( + abi.encodeWithSelector(WethLike(_wethLike).transfer.selector, _to, _value) + ); + require(success, "SafeSend: WETH transfer failed"); }
18-19: Consider using a NatSpec comment for the functionAdding NatSpec comments would improve documentation and readability.
+/** + * @notice Safely sends ETH to a recipient, with WETH fallback + * @param _to The recipient address + * @param _value The amount to send + * @param _wethLike The address of the wrapped native token contract + */ function safeSend(address payable _to, uint256 _value, address _wethLike) internal { if (_to.send(_value)) return;contracts/src/0.8/RealitioForeignProxyOptimism.sol (1)
29-29: Consider making the gas limit value more explicitThe current comment mentions that setting the gas limit too high results in high fees, but doesn't explain why 200000 was chosen.
Consider adding a brief explanation of how the 200000 value was determined, to help future maintainers understand whether this value needs adjustment.
Also applies to: 28-28
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
contracts/deploy/foreign/arbitrum.js(2 hunks)contracts/deploy/foreign/optimism.js(2 hunks)contracts/deploy/foreign/zksync.js(2 hunks)contracts/deploy/shared/consts.js(2 hunks)contracts/src/0.8/RealitioForeignProxyArbitrum.sol(20 hunks)contracts/src/0.8/RealitioForeignProxyOptimism.sol(18 hunks)contracts/src/0.8/RealitioForeignProxyZkSync.sol(19 hunks)contracts/src/0.8/libraries/SafeSend.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/src/0.8/RealitioForeignProxyZkSync.sol
- contracts/src/0.8/RealitioForeignProxyArbitrum.sol
🧰 Additional context used
🪛 ESLint
contracts/deploy/foreign/optimism.js
[error] 47-47: 'getChainId' is not defined.
(no-undef)
contracts/deploy/foreign/zksync.js
[error] 26-26: 'getChainId' is not defined.
(no-undef)
contracts/deploy/foreign/arbitrum.js
[error] 25-25: 'getChainId' is not defined.
(no-undef)
[error] 38-38: Insert ,
(prettier/prettier)
🔇 Additional comments (16)
contracts/deploy/shared/consts.js (2)
10-13: Well-documented newwNativeconstant for WETH addressesThe addition of the
wNativeconstant with clear comments indicating these are WETH addresses is well-structured and properly documented.
57-57: Properly exported the new constantThe
wNativeconstant is correctly exported as part of the module exports.contracts/deploy/foreign/zksync.js (2)
2-2: Updated imports to includewNativeThe import statement has been correctly updated to include the
wNativeconstant from the shared module.
30-31: AddedwNativeas constructor parameterThe addition of
wNative[chainId]as the first parameter to the contract constructor aligns with the PR's objective to handle wrapped native currency.contracts/deploy/foreign/optimism.js (2)
1-1: Updated imports to includewNativeThe import statement has been correctly updated to include the
wNativeconstant from the shared module.
51-52: AddedwNativeas constructor parameterThe addition of
wNative[chainId]as the first parameter to the contract constructor aligns with the PR's objective to handle wrapped native currency.contracts/deploy/foreign/arbitrum.js (2)
1-1: Updated imports to includewNativeThe import statement has been correctly updated to include the
wNativeconstant from the shared module.
30-31: AddedwNativeas constructor parameterThe addition of
wNative[chainId]as the first parameter to the contract constructor aligns with the PR's objective to handle wrapped native currency.contracts/src/0.8/libraries/SafeSend.sol (1)
1-24: Well-designed safety mechanism for ETH transfersThe
SafeSendlibrary provides a robust fallback mechanism for ETH transfers that might fail, using wrapped native tokens as a backup. The implementation follows good security practices.contracts/src/0.8/RealitioForeignProxyOptimism.sol (7)
16-16: Good integration of SafeSend libraryThe SafeSend library import and usage along with the new wNative state variable provides a more robust mechanism for ETH transfers.
Also applies to: 23-23, 68-68
27-29: Helpful gas limit explanationThe added comments provide clear context about the gas limit considerations for cross-chain messaging.
97-98: Properly implemented wNative parameterThe constructor correctly accepts and initializes the new wNative parameter.
Also applies to: 108-108, 118-118
193-194: Improved ETH transfer safetyReplacing direct send calls with safeSend improves the robustness of the contract by handling failed transfers gracefully.
Also applies to: 220-220, 242-242, 314-314, 355-355
154-154: Consistent usage of MIN_GAS_LIMIT constantConverting the previous immutable variable to a constant and using it consistently throughout the contract is a good practice.
Also applies to: 244-244, 420-420
256-257: Improved documentation for Kleros denomination offsetThe added comments clarify the +1 offset in Kleros denomination compared to Realitio format, which helps prevent potential confusion.
Also applies to: 324-324, 367-367, 479-479, 504-504, 527-527, 554-554
16-16:❓ Verification inconclusive
Verify the wNative addresses on deployment to each chain
Ensure that the correct WETH/WMATIC/etc. addresses are used when deploying to different chains.
Also applies to: 23-23, 68-68, 118-118
🏁 Script executed:
#!/bin/bash # Check if there are any scripts or tests that handle deployment with the wNative parameter echo "Checking deployment scripts for wNative parameter handling:" rg -A 5 -B 5 "wNative" --type jsLength of output: 5430
Action Required: Verify Correct wNative Addresses Across Chains
Please double-check that the native token addresses (WETH, WMATIC, etc.) used in deployments are correct for each chain. Our deployment scripts reference the
wNativemapping (defined incontracts/deploy/shared/consts.js) and passwNative[chainId]to the proxies. In particular, verify the following:
- In
contracts/deploy/shared/consts.js, confirm that the addresses for each chain (e.g., mainnet, sepolia, Optimism, Arbitrum) are up to date and accurate.- Ensure that the deployment scripts (such as those in
contracts/deploy/foreign/optimism.js,contracts/deploy/foreign/zksync.js, andcontracts/deploy/foreign/arbitrum.js) properly usewNative[chainId]so that the correct native asset is deployed on each chain.- The same verification should apply to the lines indicated in
RealitioForeignProxyOptimism.sol(lines 16, 23, 68, and 118).
contracts/deploy/foreign/zksync.js
Outdated
| @@ -23,9 +23,11 @@ async function deployForeignProxy({ deploy, from, parameters, homeNetworkName, h | |||
| const surplus = eth("0.03"); // The surplus will be automatically reimbursed when the dispute is created. | |||
| const l2GasLimit = 1500000; // Gas limit for a tx on L2. | |||
| const l2GasPerPubdataByteLimit = 800; | |||
| const chainId = await getChainId(); | |||
There was a problem hiding this comment.
Missing function definition for getChainId
The getChainId() function is called but not defined or imported in this file.
You need to either import this function or define it. This will cause a runtime error if not addressed.
-const { encodeExtraData, eth, getMetaEvidenceCID, wNative } = require("../shared");
+const { encodeExtraData, eth, getMetaEvidenceCID, wNative, getChainId } = require("../shared");Or, if getChainId is from hardhat:
-const { ethers } = require("hardhat");
+const { ethers, network } = require("hardhat");
+
+async function getChainId() {
+ return network.config.chainId;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const chainId = await getChainId(); | |
| // Assuming this file is contracts/deploy/foreign/zksync.js | |
| // Updated import to include getChainId | |
| const { encodeExtraData, eth, getMetaEvidenceCID, wNative, getChainId } = require("../shared"); | |
| // ... other code in the file ... | |
| async function deployContracts() { | |
| // getChainId is now defined via the import | |
| const chainId = await getChainId(); | |
| console.log("Chain ID:", chainId); | |
| // ... rest of the deployment logic ... | |
| } | |
| deployContracts().catch(console.error); |
| const chainId = await getChainId(); | |
| // Using hardhat's network module for getChainId | |
| const { ethers, network } = require("hardhat"); | |
| // Define getChainId inline | |
| async function getChainId() { | |
| return network.config.chainId; | |
| } | |
| // ... other code in the file ... | |
| async function deployContracts() { | |
| const chainId = await getChainId(); | |
| console.log("Chain ID:", chainId); | |
| // ... rest of the deployment logic ... | |
| } | |
| deployContracts().catch(console.error); |
🧰 Tools
🪛 ESLint
[error] 26-26: 'getChainId' is not defined.
(no-undef)
contracts/deploy/foreign/optimism.js
Outdated
| @@ -44,10 +44,11 @@ async function deployForeignProxy({ deploy, from, parameters, homeNetworkName, h | |||
| const { numberOfJurors, foreignBridge, variant } = parameters; | |||
| const metaEvidence = getMetaEvidenceCID(homeNetworkName); | |||
| const arbitratorExtraData = encodeExtraData(courts.oracle, numberOfJurors); | |||
| const chainId = await getChainId(); | |||
There was a problem hiding this comment.
Missing function definition for getChainId
The getChainId() function is called but not defined or imported in this file.
You need to either import this function or define it. This will cause a runtime error if not addressed.
-const { encodeExtraData, getMetaEvidenceCID, wNative } = require("../shared");
+const { encodeExtraData, getMetaEvidenceCID, wNative, getChainId } = require("../shared");Or, if getChainId is from hardhat:
+const { network } = require("hardhat");
+
+async function getChainId() {
+ return network.config.chainId;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const chainId = await getChainId(); | |
| // At the top of the file in contracts/deploy/foreign/optimism.js | |
| -const { encodeExtraData, getMetaEvidenceCID, wNative } = require("../shared"); | |
| +const { encodeExtraData, getMetaEvidenceCID, wNative, getChainId } = require("../shared"); | |
| ... | |
| // Later in the file | |
| async function deployContract() { | |
| const chainId = await getChainId(); | |
| // rest of deployment logic | |
| } |
| const chainId = await getChainId(); | |
| // At the top of the file in contracts/deploy/foreign/optimism.js | |
| const { encodeExtraData, getMetaEvidenceCID, wNative } = require("../shared"); | |
| +const { network } = require("hardhat"); | |
| +async function getChainId() { | |
| + return network.config.chainId; | |
| +} | |
| ... | |
| // Later in the file | |
| async function deployContract() { | |
| const chainId = await getChainId(); | |
| // rest of deployment logic | |
| } |
🧰 Tools
🪛 ESLint
[error] 47-47: 'getChainId' is not defined.
(no-undef)
contracts/deploy/foreign/arbitrum.js
Outdated
| const chainId = await getChainId(); | ||
|
|
There was a problem hiding this comment.
Missing function definition for getChainId
The getChainId() function is called but not defined or imported in this file.
You need to either import this function or define it. This will cause a runtime error if not addressed.
-const { encodeExtraData, eth, getMetaEvidenceCID, wNative } = require("../shared");
+const { encodeExtraData, eth, getMetaEvidenceCID, wNative, getChainId } = require("../shared");Or, if getChainId is from hardhat:
+const { network } = require("hardhat");
+
+async function getChainId() {
+ return network.config.chainId;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const chainId = await getChainId(); | |
| const { encodeExtraData, eth, getMetaEvidenceCID, wNative, getChainId } = require("../shared"); | |
| async function deploy() { | |
| const chainId = await getChainId(); | |
| // ... rest of the deployment logic | |
| } | |
| deploy().catch(error => { | |
| console.error(error); | |
| process.exit(1); | |
| }); |
| const chainId = await getChainId(); | |
| const { encodeExtraData, eth, getMetaEvidenceCID, wNative } = require("../shared"); | |
| const { network } = require("hardhat"); | |
| async function getChainId() { | |
| return network.config.chainId; | |
| } | |
| async function deploy() { | |
| const chainId = await getChainId(); | |
| // ... rest of the deployment logic | |
| } | |
| deploy().catch(error => { | |
| console.error(error); | |
| process.exit(1); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 25-25: 'getChainId' is not defined.
(no-undef)
Summary by CodeRabbit