fix(Proxy): test update #70
Conversation
WalkthroughThis pull request updates the Hardhat configuration files to change the source directory to a versioned subdirectory ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Contract
participant Proxy as HomeProxy (Optimism/Mock)
participant Messenger as Messenger Contract
participant L1 as L1 Network
User->>Proxy: Calls handleNotifiedRequest/handleRejectedRequest
Proxy->>Proxy: Executes sendToL1(_data)
Proxy->>Messenger: Invokes sendMessage(foreignProxy, _data, MIN_GAS_LIMIT)
Messenger->>L1: Forwards message to L1 network
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 1
🧹 Nitpick comments (3)
contracts/test/foreign-proxy-optimism.test.js (1)
306-307: Removed unnecessary empty line with whitespace.There's an unnecessary whitespace on line 307. This should be removed for cleaner code.
const arbAnswer = zeroPadValue(toBeHex(7), 32); -🧰 Tools
🪛 ESLint
[error] 307-307: Delete
····(prettier/prettier)
contracts/test/foreign-proxy-zksync.test.js (2)
324-324: Minor inconsistency in BigInt literal formatting.While most BigInt conversions use
toBigInt(), some arithmetic operations in assertions directly use BigInt literals (likeoldBalance + toBigInt(8499)). For consistency, consider usingtoBigInt()for all numeric literals in expressions, especially when mixed with variables that might already be BigInt.- oldBalance + toBigInt(8499), + oldBalance + toBigInt(8499),(No changes needed in this specific diff as the code is already using the suggested format - this comment is to highlight the pattern throughout the file)
Also applies to: 368-368, 427-427, 553-553, 899-904
468-468: Remove duplicate 'await' in balance retrieval.There's a redundant
awaitin this line.- const newBalance = await await getBalance(other); + const newBalance = await getBalance(other);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
contracts/hardhat.config.js(1 hunks)contracts/hardhat.config.zksync.js(1 hunks)contracts/src/0.8/RealitioHomeProxyOptimism.sol(4 hunks)contracts/src/0.8/test/arbitrum/MockRealitioHomeProxyArbitrum.sol(1 hunks)contracts/src/0.8/test/optimism/MockRealitioHomeProxyOptimism.sol(1 hunks)contracts/src/0.8/test/polygon/MockRealitioForeignProxyPolygon.sol(1 hunks)contracts/src/0.8/test/polygon/MockRealitioHomeProxyPolygon.sol(2 hunks)contracts/src/0.8/test/zksync/MockRealitioHomeProxyZkSync.sol(1 hunks)contracts/test/foreign-proxy-arbitrum.test.js(27 hunks)contracts/test/foreign-proxy-gnosis.test.js(20 hunks)contracts/test/foreign-proxy-optimism.test.js(6 hunks)contracts/test/foreign-proxy-polygon.test.js(20 hunks)contracts/test/foreign-proxy-zksync.test.js(41 hunks)contracts/test/to-migrate/CrossChainArbitrationProxy.test.js(0 hunks)
💤 Files with no reviewable changes (1)
- contracts/test/to-migrate/CrossChainArbitrationProxy.test.js
🧰 Additional context used
🧬 Code Definitions (4)
contracts/test/foreign-proxy-polygon.test.js (3)
contracts/test/foreign-proxy-gnosis.test.js (6)
ethers(4-4)questionID(10-10)answer(11-11)gasPrice(18-18)oneETH(26-26)ZERO_HASH(27-27)contracts/test/foreign-proxy-arbitrum.test.js (6)
ethers(4-4)questionID(10-10)answer(11-11)gasPrice(26-26)oneETH(33-33)ZERO_HASH(34-34)contracts/test/foreign-proxy-zksync.test.js (6)
ethers(4-4)questionID(10-10)answer(11-11)gasPrice(28-28)oneETH(35-35)ZERO_HASH(36-36)
contracts/test/foreign-proxy-gnosis.test.js (3)
contracts/test/foreign-proxy-polygon.test.js (6)
ethers(4-4)questionID(10-10)answer(11-11)gasPrice(18-18)oneETH(25-25)ZERO_HASH(26-26)contracts/test/foreign-proxy-arbitrum.test.js (6)
ethers(4-4)questionID(10-10)answer(11-11)gasPrice(26-26)oneETH(33-33)ZERO_HASH(34-34)contracts/test/foreign-proxy-zksync.test.js (6)
ethers(4-4)questionID(10-10)answer(11-11)gasPrice(28-28)oneETH(35-35)ZERO_HASH(36-36)
contracts/test/foreign-proxy-arbitrum.test.js (3)
contracts/test/foreign-proxy-gnosis.test.js (5)
ethers(4-4)questionID(10-10)answer(11-11)oneETH(26-26)ZERO_HASH(27-27)contracts/test/foreign-proxy-polygon.test.js (5)
ethers(4-4)questionID(10-10)answer(11-11)oneETH(25-25)ZERO_HASH(26-26)contracts/test/foreign-proxy-zksync.test.js (5)
ethers(4-4)questionID(10-10)answer(11-11)oneETH(35-35)ZERO_HASH(36-36)
contracts/test/foreign-proxy-zksync.test.js (3)
contracts/test/foreign-proxy-gnosis.test.js (5)
ethers(4-4)questionID(10-10)answer(11-11)oneETH(26-26)ZERO_HASH(27-27)contracts/test/foreign-proxy-polygon.test.js (5)
ethers(4-4)questionID(10-10)answer(11-11)oneETH(25-25)ZERO_HASH(26-26)contracts/test/foreign-proxy-arbitrum.test.js (5)
ethers(4-4)questionID(10-10)answer(11-11)oneETH(33-33)ZERO_HASH(34-34)
🪛 ESLint
contracts/hardhat.config.zksync.js
[error] 27-27: Insert ,
(prettier/prettier)
contracts/test/foreign-proxy-optimism.test.js
[error] 307-307: Delete ····
(prettier/prettier)
🔇 Additional comments (49)
contracts/src/0.8/test/zksync/MockRealitioHomeProxyZkSync.sol (1)
10-10: Contract renamed to follow consistent naming conventionThe contract name has been changed from
MockZkHomeProxytoMockRealitioHomeProxyZkSync, which aligns with the naming convention adopted across other mock contracts in the project. This change improves code readability by clearly indicating both the purpose (Realitio Home Proxy) and the target chain (ZkSync).contracts/hardhat.config.js (1)
34-34:❓ Verification inconclusive
Updated source path to versioned directory
The source path has been modified from
"./src"to"./src/0.8", indicating a reorganization of source files by Solidity version. This change aligns with the PR objectives to update the project structure and ensures that the compiler looks in the correct directory for Solidity 0.8.x files.You may want to verify that all necessary files have been moved to this new directory structure:
🏁 Script executed:
#!/bin/bash # Check if any Solidity files still exist in the old src directory find ./src -maxdepth 1 -name "*.sol" | grep -v "src/0.8"Length of output: 97
Verify Solidity Files in the New Directory
The change in
hardhat.config.jscorrectly updates the source path from"./src"to"./src/0.8", which aligns with the project’s refactoring. However, the verification script produced an error indicating that the old./srcdirectory does not exist. This means either:
- All Solidity files have already been moved to
./src/0.8, or- The
./srcdirectory was never used.To be sure the necessary files are in place, please manually verify that the
./src/0.8directory exists and that it contains all the expected Solidity files. You might also consider running an additional check like the following:#!/bin/bash # Check if the new versioned directory exists and list Solidity files within it if [ -d "./src/0.8" ]; then echo "Directory exists: ./src/0.8" find ./src/0.8 -type f -name "*.sol" else echo "Directory ./src/0.8 does not exist" fiOnce you confirm this, the reorganization can be considered complete.
contracts/src/0.8/test/polygon/MockRealitioForeignProxyPolygon.sol (1)
10-10: Contract renamed to follow consistent naming conventionThe contract name has been changed from
MockForeignArbitrationProxyWithAppealstoMockRealitioForeignProxyPolygon, which aligns with the naming convention used across other mock contracts in this PR. This rename improves code clarity by explicitly indicating both the contract's purpose (Realitio Foreign Proxy) and the target chain (Polygon).contracts/src/0.8/test/arbitrum/MockRealitioHomeProxyArbitrum.sol (1)
9-9: Contract renamed to follow consistent naming conventionThe contract name has been changed from
MockRealitioHomeProxytoMockRealitioHomeProxyArbitrum, which aligns with the naming convention adopted for other mock contracts in this PR. This rename improves code organization and readability by clearly indicating both the contract's purpose (Realitio Home Proxy) and the target chain (Arbitrum).contracts/src/0.8/test/polygon/MockRealitioHomeProxyPolygon.sol (3)
6-6: Updated import statement to reflect renamed mock contract.The import statement has been updated to use the new contract name
MockRealitioForeignProxyPolygon, which follows a more consistent naming convention.
12-12: Renamed contract to follow consistent naming convention.The contract name has been updated from
MockHomeArbitrationProxytoMockRealitioHomeProxyPolygon, which provides better context about what the mock is representing (Realitio, Home Proxy, Polygon network).
23-23: Updated reference to use the renamed mock contract.This line correctly updates the reference to use the renamed
MockRealitioForeignProxyPolygoncontract to match the import statement change.contracts/src/0.8/RealitioHomeProxyOptimism.sol (3)
55-55: Made modifier virtual to support extension in derived contracts.The
onlyForeignProxymodifier is now marked asvirtual, allowing it to be overridden in derived contracts like the newly introduced mock contract.
126-126: Refactored message sending to use a common internal function.Direct calls to
messenger.sendMessagehave been replaced with calls to the newsendToL1function, promoting code reuse and making it easier to override this functionality in derived contracts.Also applies to: 149-149
221-227: Added internal function to encapsulate L1 messaging logic.The new
sendToL1internal virtual function centralizes the logic for sending messages to L1, improving maintainability and allowing for customization in derived contracts.contracts/hardhat.config.zksync.js (1)
31-31: Updated source directory path to versioned subdirectory.The sources path has been changed from "./src" to "./src/0.8" to organize code by Solidity version, which aligns with the PR objective of updating the tests.
contracts/test/foreign-proxy-optimism.test.js (7)
21-21: Added constant for the Optimism cross-domain messenger address.A new constant
messengerhas been added with the canonical address for the Optimism cross-domain messenger precompile, which is used in the updated assertions.
58-58: Added assertion for the wrapped native token address.New assertion checks that
foreignProxy.wNative()returns the expectedotheraddress, improving test coverage.
70-71: Updated messenger assertions to use the new constant.The assertions now check that
homeProxy.messenger()matches the constantmessengerand thathomeProxy.mockMessenger()matchesmockMessenger.target, providing better validation of the contract initialization.
278-278: Updated expected arbitration status for failed dispute creation.The expected status after a failed dispute creation has been changed from
5to4, likely reflecting changes in the enum values in the contract.
312-320: Updated ruling handling logic and assertions.The ruling handling logic has been updated to properly check events and status values after applying a ruling. The expected status after a ruling is now
3instead of4, aligning with contract changes.
827-840: Updated contract factory name and deployment parameters.The test now uses
MockRealitioHomeProxyOptimismfactory and has updated the deployment parameters to match the new contract interfaces and requirements.
859-861: Added helper function for retrieving account balances.A new
getBalancehelper function has been added to standardize balance retrieval across the test file, improving code maintainability.contracts/test/foreign-proxy-gnosis.test.js (7)
2-4: Updated testing dependencies to use latest Hardhat utilitiesThe imports have been modernized to use
@nomicfoundation/hardhat-network-helpersfor time manipulation instead of the deprecatedethereum-waffle. Additionally, the code now uses the newer ethers utility functions (toBigInt,ZeroAddress,zeroPadValue,toBeHex).
10-11: Updated constant initializations to use new ethers utility functionsConstants are now properly initialized using the newer ethers utilities such as
zeroPadValue,toBeHex, andtoBigInt. This reflects the changes in the ethers library and improves consistency across the codebase.Also applies to: 18-18, 24-27
51-51: Updated contract address references from.addressto.targetReferences to contract addresses have been updated from the deprecated
.addressproperty to the newer.targetproperty. This aligns with changes in the ethers.js contract interface in newer versions.Also applies to: 57-60
97-98: Updated balance checks using new getBalance utilityBalance verification now uses the custom
getBalancefunction that standardizes how balances are retrieved across all tests. The test also correctly usestoBigIntfor numerical comparisons.Also applies to: 142-146
229-229: Updated ZERO_HASH parameter in reportArbitrationAnswerThe function call now correctly uses
ZeroAddressinstead of an address constant, ensuring compatibility with the function's expected parameter types.
702-706: Contract deployment updated for compatibility with new contract factoriesThe contract deployment logic has been updated to use the newer ethers.js patterns for creating contract addresses, replacing the now deprecated pattern. This ensures that contract deployments work correctly with the latest ethers.js version.
736-738: Added standardized getBalance helper functionA new helper function has been added to standardize balance retrieval across test cases. This improves code maintainability and ensures consistent handling of account balances throughout the test suite.
async function getBalance(account) { return account.provider.getBalance(await account.getAddress()); }contracts/src/0.8/test/optimism/MockRealitioHomeProxyOptimism.sol (1)
1-31: Well-structured mock contract for testing cross-domain messagingThis new mock contract properly bypasses the Optimism cross-domain messaging pattern for testing purposes. The contract correctly:
- Overrides the
onlyForeignProxymodifier to use the mock messenger- Overrides the
sendToL1function to use the mock messenger- Stores references to the mock messenger
The implementation follows best practices for creating testable contract variants by leveraging inheritance and overriding critical virtual functions from the parent contract.
contracts/test/foreign-proxy-polygon.test.js (5)
2-4: Updated testing dependencies to use latest Hardhat utilitiesThe imports have been modernized to use
@nomicfoundation/hardhat-network-helpersfor time manipulation instead of the deprecatedethereum-waffle. Additionally, the code now uses the newer ethers utility functions (toBigInt,ZeroAddress,zeroPadValue,toBeHex).
10-11: Updated constant initializations to use new ethers utility functionsConstants are now properly initialized using the newer ethers utilities such as
zeroPadValue,toBeHex,parseEtherandtoBigInt. This reflects the changes in the ethers library and improves consistency across the codebase.Also applies to: 18-18, 25-26
49-49: Updated contract address references and chain ID formattingReferences to contract addresses have been updated from
.addressto.target, and the foreign chain ID is now properly zero-padded for consistency with other implementations. This aligns with changes in the ethers.js contract interface.Also applies to: 55-61
695-717: Updated mock contract deployment and connectionThe test now correctly uses the mock contract implementations for Polygon (
MockRealitioForeignProxyPolygonandMockRealitioHomeProxyPolygon). Additionally, the FxChild and FxRoot tunnels are properly initialized with cross-references after deployment.
727-729: Added standardized getBalance helper functionA new helper function has been added to standardize balance retrieval across test cases. This improves code maintainability and ensures consistent handling of account balances throughout the test suite.
async function getBalance(account) { return account.provider.getBalance(await account.getAddress()); }contracts/test/foreign-proxy-arbitrum.test.js (7)
2-4: Updated testing dependencies to use latest Hardhat utilitiesThe imports have been modernized to use
@nomicfoundation/hardhat-network-helpersfor time manipulation instead of the deprecatedethereum-waffle. Additionally, the code now uses the newer ethers utility functions (toBigInt,ZeroAddress,zeroPadValue,toBeHex).
10-11: Updated constant initializations to use new ethers utility functionsConstants are now properly initialized using the newer ethers utilities such as
zeroPadValue,toBeHex,parseEtherandtoBigInt. This reflects the changes in the ethers library and improves consistency across the codebase.Also applies to: 26-26, 33-34
62-62: Updated contract address references to use target instead of addressReferences to contract addresses have been updated from
.addressto.targetthroughout the test file. This aligns with changes in the ethers.js contract interface in newer versions.Also applies to: 68-82
90-93: Added proper mock contract initialization checksThe test now verifies the relationships between mock contracts with appropriate assertions, ensuring that the test environment is properly set up before running tests.
947-950: Updated contract address prediction logicThe contract deployment logic has been updated to use the newer ethers.js patterns for creating contract addresses, replacing the now deprecated pattern. This ensures that contract deployments work correctly with the latest ethers.js version.
952-964: Updated contract constructor parametersThe contract constructor parameters have been updated to match the new contract interface, properly using the new
targetproperty and organizing parameters in a clear, structured manner.
988-990: Added standardized getBalance helper functionA new helper function has been added to standardize balance retrieval across test cases. This improves code maintainability and ensures consistent handling of account balances throughout the test suite.
async function getBalance(account) { return account.provider.getBalance(await account.getAddress()); }contracts/test/foreign-proxy-zksync.test.js (11)
2-4: Library and utility function updates look good.The change from
ethereum-waffleto@nomicfoundation/hardhat-network-helpersis appropriate, as this is the recommended testing library with newer Hardhat versions. The specific utility function imports from ethers are also well-structured.
10-11: Good update of byte string formatting using newer ethers.js methods.The change from previous methods to
zeroPadValue(toBeHex(...), 32)aligns with the ethers.js v6 API which deprecates older methods likehexZeroPad. This format is also used consistently across multiple test files.Also applies to: 22-22
28-28: Appropriate migration from BigNumber to native BigInt.The code now uses
toBigIntinstead ofBigNumber, which aligns with modern JavaScript's native BigInt support and the updated ethers.js API.Also applies to: 157-157
35-36: Good constant additions for clarity.Adding constants like
oneETHandZERO_HASHimproves code readability and maintainability. The use ofethers.parseEther("1")is the correct way to represent 1 ETH in wei with ethers.js v6.
60-60: Contract reference updates from .address to .target look good.The consistent change from
.addressto.targetfor contract references reflects the updated property name in ethers.js v6 for deployed contract instances. This change is applied consistently throughout the file.Also applies to: 66-80
1057-1059: Updated event parsing function signature.The
getEmittedEventfunction now includes an additional parameter for the interface. This is a good improvement as it makes the function more flexible and explicit about which interface to use for parsing.
1105-1107: Good addition of a helper function for balance retrieval.The new
getBalancefunction centralizes the logic for retrieving account balances, which improves code maintainability and consistency. This function is appropriately used throughout the test file wherever balance checks are needed.
148-149: Balance checking updates are consistent with ethers.js v6.The changes to how balances are retrieved and compared align with ethers.js v6 API, which returns native BigInt values. The new
getBalanceutility function helps maintain consistency throughout the tests.Also applies to: 155-156, 302-303, 323-324, 361-362, 368-369
153-154: Transaction fee calculation properly updated.The changes to how transaction fees are calculated (
(await tx.wait()).gasUsed * gasPrice) maintain the same logic while adapting to the updated ethers.js v6 API.Also applies to: 466-467, 550-551
197-198: Event parsing updates correctly implemented.The updated method for extracting emitted events from transaction receipts uses the enhanced
getEmittedEventfunction, which now accepts the interface parameter for more robust event parsing.Also applies to: 261-262, 316-317, 356-357, 399-400
1071-1094: Contract deployment code properly updated.The changes to contract deployment code correctly use
.targetfor contract addresses and maintain the same logical structure while adapting to the updated ethers.js v6 API.
Removes
ethereum-waffleand updates the tests.Removes the old gnosis test file to avoid duplication
Summary by CodeRabbit
Chores
Refactor
Tests