Deploy berachain #978
Deploy berachain #978
Conversation
…racts into deploy-berachain-lf-12119
|
Warning Rate limit exceeded@0xDEnYO has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request makes extensive updates to integrate support for the “berachain” network. It changes Solidity version pragmas in several test files from a fixed version to a caret version (^0.8.17), allowing for broader compiler compatibility. Numerous configuration files (JSON and TOML) are updated to include new berachain entries—such as chain IDs, addresses, endpoints, and formatting adjustments—and deployment logs and facet version data are extended with new berachain details. No changes were made to public entity declarations. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🔭 Outside diff range comments (1)
script/deploy/_targetState.json (1)
1-60:🧩 Analysis chain
Mainnet Configuration – Facet Version Updates and Duplicate Key Issue
In the mainnet block the facet version updates (e.g. updating CalldataVerificationFacet to "1.2.0", ERC20Proxy to "1.1.0", Executor to "2.1.0", etc.) appear intentional and consistent. However, the static analysis flagged a duplicate key issue for "GasZipFacet" (observed on lines 41–42). In JSON objects duplicate keys cause only the last declaration to be effective, which can lead to unintended behavior or parse errors.Consider removing the redundant entry. For example, you might apply a diff such as:
- "GasZipFacet": "2.0.2", - "GasZipFacet": "2.0.2", + "GasZipFacet": "2.0.2",Please verify that the remaining entry is in the correct order and that no other keys are unintentionally overridden.
🏁 Script executed:
#!/bin/bash # Verify duplicate "GasZipFacet" entries in the target JSON file. # This command will list all occurrences of "GasZipFacet" along with their line numbers. grep -n "GasZipFacet" script/deploy/_targetState.jsonLength of output: 1280
Duplicate Key in JSON – Multiple GasZipFacet Entries Found
We've confirmed that the "GasZipFacet" key appears twice in multiple sections of script/deploy/_targetState.json (e.g., lines 41–42, 99–100, 223–224, and several other pairs). Duplicate keys in JSON cause only the last declaration to be effective, which can introduce unintended behavior during parsing.• Multiple duplicate pairs of "GasZipFacet": "2.0.2" were found throughout the file.
• The review comment’s suggestion (removing the redundant entry) applies not only to the snippet shown but to all duplicate occurrences.It is recommended to remove the redundant duplicate entries to ensure that each key is unique and to avoid possible misconfiguration issues.
🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🧹 Nitpick comments (2)
config/stargate.json (2)
216-216: Placeholder in Composers Section for Berachain
The "berachain" composer address is currently set as a placeholder ("0x0000000000000000000000000000000000000000"). If a concrete address becomes available, update this entry. Otherwise, document the placeholder status for future reference.
264-264: Placeholder in Routers Section for Berachain
The "berachain" router is also set to a placeholder address ("0x0000000000000000000000000000000000000000"). Confirm if this is temporary and, if so, consider adding a TODO comment for updating this field after obtaining the actual router address.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
archive/test/LiFiDiamondImmutable.t.sol(1 hunks)config/amarok.json(2 hunks)config/dexs.json(2 hunks)config/global.json(1 hunks)config/networks.json(1 hunks)config/permit2Proxy.json(1 hunks)config/relay.json(1 hunks)config/stargate.json(4 hunks)config/symbiosis.json(1 hunks)config/tokenwrapper.json(1 hunks)deployments/_deployments_log_file.json(25 hunks)deployments/berachain.diamond.json(1 hunks)deployments/berachain.json(1 hunks)foundry.toml(2 hunks)networks(1 hunks)script/deploy/_targetState.json(57 hunks)test/solidity/Facets/GasZipFacet.t.sol(1 hunks)test/solidity/Facets/RelayFacet.t.sol(1 hunks)test/solidity/Helpers/WithdrawablePeriphery.t.sol(1 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(1 hunks)test/solidity/Periphery/Permit2Proxy.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- networks
- test/solidity/Facets/RelayFacet.t.sol
- test/solidity/Facets/GasZipFacet.t.sol
- test/solidity/Periphery/Permit2Proxy.t.sol
- test/solidity/Periphery/GasZipPeriphery.t.sol
- test/solidity/Helpers/WithdrawablePeriphery.t.sol
🧰 Additional context used
🪛 Biome (1.9.4)
script/deploy/_targetState.json
[error] 263-263: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 300-300: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 538-538: expected , but instead found "ReceiverStargateV2"
Remove "ReceiverStargateV2"
(parse)
[error] 537-537: The key ReceiverStargateV2 was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 706-706: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 815-815: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 1023-1023: The key ReceiverStargateV2 was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 1347-1347: expected , but instead found "GasZipFacet"
Remove "GasZipFacet"
(parse)
[error] 1346-1346: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 1463-1463: expected , but instead found "GasZipFacet"
Remove "GasZipFacet"
(parse)
[error] 1462-1462: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🪛 GitHub Actions: JSON Checker
script/deploy/_targetState.json
[error] 537-537: Parse error on line 537: Expecting 'EOF', '}', ':', ',', ']', got 'STRING'
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (45)
archive/test/LiFiDiamondImmutable.t.sol (2)
13-134: LGTM! Comprehensive test coverage.The test implementation thoroughly covers critical aspects of the diamond proxy pattern including:
- Deployment validation
- Delegate call functionality
- Ownership controls
- Function selector handling
- ETH receiving capability
2-2: Verify compiler version compatibility with project requirements.The change from fixed version
0.8.17to^0.8.17allows compilation with newer 0.8.x versions. While this provides flexibility, it's important to ensure this aligns with:
- Project's security requirements
- Audit recommendations
- CI pipeline configurations
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if there are any compiler version constraints in foundry.toml or hardhat.config echo "Checking compiler version constraints in configuration files..." fd -e toml -e js -x cat {} 2>/dev/null | grep -i "solidity" # Check if other contracts in the project use fixed versions echo -e "\nChecking Solidity versions used in other contracts..." fd -e sol | xargs grep "pragma solidity" 2>/dev/null | sort | uniq -cLength of output: 28691
Compiler Version Consistency Verified
The file under review now uses the caret version (
^0.8.17), which aligns with the majority of test and deployment files. Our repository-wide check confirms that most test files and deployment scripts have already moved to using^0.8.17, supporting flexibility with newer 0.8.x releases.A few source files (e.g., some in archive/src/Facets and archive/src/Periphery) still specify the version as a fixed
0.8.17;—if this discrepancy is intentional based on audit or design requirements, no immediate action is needed. Otherwise, you might want to review those cases for consistency across the project.config/permit2Proxy.json (1)
8-8: New Berachain Entry Added.
A new entry for "berachain" with the address "0x000000000022D473030F116dDEE9F6B43aC78BA3" has been introduced. Please verify this is the correct and intended contract address for Berachain, and that its formatting is consistent with the other network entries.deployments/berachain.json (1)
1-27: Berachain Deployment Configuration Introduced.
This new JSON file maps various facets (e.g., DiamondCutFacet, OwnershipFacet, etc.) to their respective contract addresses for the Berachain deployment. Please double-check that each of the provided addresses is correct and that key facets (especially those related to upgradeability and ownership) align with the intended deployment targets.config/global.json (1)
2-4: Berachain create3Factory Entry Added.
The "berachain" key has been added under the create3Factory object with the address "0x8b60A21A0e2852cCC944971486868beBB9f32287". Ensure that this address is accurate and matches the deployment specifications for Berachain.config/tokenwrapper.json (1)
5-5: New Token Wrapper Entry for Berachain.
A new network entry for "berachain" with the address "0x6969696969696969696969696969696969696969" has been added. Please verify that this address is correct and that it fits the same format or convention as the other network entries in this configuration file.deployments/berachain.diamond.json (1)
1-72: Berachain Diamond Deployment Definition Added.
This file defines the facets and periphery components for the LiFiDiamond deployment on Berachain. All the facets (with their respective names and version numbers) and periphery settings (including addresses for critical components like ERC20Proxy, Executor, FeeCollector, etc.) have been added. Please ensure the following:
- Each contract address is verified and correct for the intended Berachain deployment.
- Version numbers for the facets (ranging from "1.0.0" to "1.2.0") accurately reflect the deployed implementations.
- For periphery components where empty strings are provided (e.g., "GasZipPeriphery", "LiFuelFeeCollector", "ReceiverAcrossV3", "RelayerCelerIM"), confirm that these values are intentionally left blank (for example, because they are optional or will be set later).
config/symbiosis.json (1)
22-25: New Berachain Entry Added
The new "berachain" section is added with the specified "metaRouter" and "gateway" addresses. Verify that these addresses match the intended deployment values and are consistent with similar network entries.config/relay.json (1)
17-21: Berachain Relay Configuration Introduced
The addition of the "berachain" entry with chainId 80094, a specific "relayReceiver", and the standard "relaySolver" address is clear and aligns with the expected pattern. Confirm that the relayReceiver address is correct for the berachain network.foundry.toml (2)
32-33: RPC Endpoint for Berachain Added
The new RPC endpoint for berachain (berachain = "${ETH_NODE_URI_BERACHAIN}") has been integrated into the [rpc_endpoints] section. Ensure that the environment variable ETH_NODE_URI_BERACHAIN is set appropriately in deployment configurations.
83-83: Etherscan Configuration for Berachain Added
The berachain Etherscan configuration, including the API key placeholder, URL, and chain identifier (80094), follows the structure used for other networks. Double-check that the URL and chain value align with the berachain network’s explorer requirements.config/amarok.json (1)
38-43: Berachain Section Integration in Amarok Configuration
The new "berachain" section with chainId 80094, a placeholder "connextHandler" (set to all zeros), an empty "domain", and an empty "tokensToApprove" array is added correctly. Verify with your deployment process if the placeholder values need to be updated later.config/stargate.json (2)
10-10: Berachain EndpointV2 Entry Added
The "berachain" entry under the "endpointV2" section is included with the address "0x6F475642a6e85809B1c36Fa62763669b1b48DD5B". Confirm that this address corresponds to the deployed berachain contract for endpoint interactions.
52-52: Berachain Token Messaging Entry Added
The "berachain" entry in the "tokenMessaging" section (address "0xAf5191B0De278C7286d6C7CC6ab6BB8A73bA2Cd6") is added as expected. Make sure that this address is consistent with the network’s messaging configuration.config/networks.json (1)
128-145: New Berachain Network Configuration Added
This block correctly defines the "berachain" network with all required fields (name, chainId, nativeAddress, nativeCurrency, wrappedNativeAddress, status, type, rpcUrl, verificationType, explorerUrl, explorerApiUrl, multicallAddress, safeApiUrl, safeAddress, safeWebUrl, and gasZipChainId). Please verify that the provided endpoints (RPC, explorer, and safe services) and addresses (e.g. safeAddress and wrappedNativeAddress) are accurate and in line with the official Berachain documentation. Note that "multicallAddress" is left as an empty string; if Berachain does support multicall functionality, consider including the appropriate address or a clarifying comment.config/dexs.json (1)
199-203: New Berachain DEX Addresses Added
The new "berachain" entry is added with three Ethereum addresses in the expected format. Ensure that these addresses correspond to the correct DEX deployments on the Berachain network. Also, double-check that the addresses are in the intended alphabetical position relative to the other network entries, and that they follow any standard conventions used in similar sections.script/deploy/_targetState.json (4)
1-1619: General Duplicate Keys Across Environments
Across multiple network configurations (e.g. mainnet, polygon, and others), several duplicate key declarations have been identified – particularly for keys like "GasZipFacet" and in some cases for "ReceiverStargateV2". Static analysis reports errors (e.g. at lines 263, 300, 537, 706, 815, 1023, 1347, and 1463) indicating that duplicate keys exist and that extra occurrences may lead to parsing issues or unintended overrides.Please review all JSON objects and ensure that for every parent object each key appears only once. Consolidating or removing duplicates will help avoid parsing ambiguities and potential pipeline failures.
🧰 Tools
🪛 Biome (1.9.4)
[error] 538-538: expected
,but instead found"ReceiverStargateV2"Remove "ReceiverStargateV2"
(parse)
[error] 1347-1347: expected
,but instead found"GasZipFacet"Remove "GasZipFacet"
(parse)
[error] 1463-1463: expected
,but instead found"GasZipFacet"Remove "GasZipFacet"
(parse)
[error] 41-41: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 99-99: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 223-223: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 263-263: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 300-300: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 339-339: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 390-390: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 443-443: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 537-537: The key ReceiverStargateV2 was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 706-706: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 815-815: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 1023-1023: The key ReceiverStargateV2 was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 1346-1346: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 1462-1462: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🪛 GitHub Actions: JSON Checker
[error] 537-537: Parse error on line 537: Expecting 'EOF', '}', ':', ',', ']', got 'STRING'
1587-1617: New "berachain" Network Configuration Added
The new berachain block (lines 1587–1617) introduces deployment settings specific to the berachain network. The facet versions—such as setting CalldataVerificationFacet to "1.2.0", ERC20Proxy to "1.1.0", Executor to "2.1.0", FeeCollector to "1.0.1", and the addition of RelayFacet (set to "1.0.0")—are consistent with the updates in other environments.Please verify that all entries in this block adhere to the established naming and versioning conventions, and that no duplicate keys exist within the new block. This new configuration appears properly structured.
535-540:⚠️ Potential issueParse Error – Missing Comma or Extra Duplicate Key?
The GitHub Actions JSON Checker reported a parse error at line 537 (expecting a comma, '}', ':', or ']', but found a string). This error is most likely due to the duplicate key declaration for "ReceiverStargateV2" or a missing comma between keys. Please inspect the lines around 535–540 and remove any redundant "ReceiverStargateV2" entry or insert the missing punctuation as needed.A careful reordering or removal of the duplicate entry should resolve the parse error.
🧰 Tools
🪛 Biome (1.9.4)
[error] 538-538: expected
,but instead found"ReceiverStargateV2"Remove "ReceiverStargateV2"
(parse)
[error] 537-537: The key ReceiverStargateV2 was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🪛 GitHub Actions: JSON Checker
[error] 537-537: Parse error on line 537: Expecting 'EOF', '}', ':', ',', ']', got 'STRING'
61-113: Polygon Configuration – Facet Version Updates and Duplicate Key
In the polygon section, several facets have their versions updated (e.g. CalldataVerificationFacet to "1.2.0", ERC20Proxy to "1.1.0", Executor to "2.1.0", etc.). Similar to mainnet, a duplicate "GasZipFacet" occurs (notably at lines 99–100). Please remove the redundant duplicate to ensure valid JSON parsing and avoid ambiguity.A suggested diff might be:
- "GasZipFacet": "2.0.2", - "GasZipFacet": "2.0.2", + "GasZipFacet": "2.0.2",Ensure the unique key survives and the intended value is preserved.
🧰 Tools
🪛 Biome (1.9.4)
[error] 99-99: The key GasZipFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
deployments/_deployments_log_file.json (25)
763-777: New berachain entry added for a facet.A new "berachain" block for the production environment with version "1.0.0" has been introduced. All keys (ADDRESS, OPTIMIZER_RUNS, TIMESTAMP, CONSTRUCTOR_ARGS, SALT, VERIFIED) follow the same structure as existing entries. Please verify that the TIMESTAMP, SALT formats, and overall values align with the deployment conventions.
1527-1541: New berachain entry for second facet.This addition introduces a "berachain" entry at version "1.0.0" with the expected deployment metadata. The details (ADDRESS, OPTIMIZER_RUNS, TIMESTAMP, etc.) seem consistent with other entries. Please double-check that the provided values match deployment records.
2290-2304: New berachain deployment entry for next facet.The block for "berachain" (version "1.0.0") is added with consistent formatting and field requirements. Verify that the hexadecimal values and timestamp follow your project’s standards.
3355-3369: New berachain deployment entry with version "1.0.1".This block adds a "berachain" deployment with version "1.0.1". All required fields are present and consistently structured. It is recommended to confirm that the version numbering and configuration details are correctly maintained across all facets.
4119-4133: New berachain entry for the associated facet.The inserted deployment record (version "1.0.0") accurately follows the existing JSON schema. Ensure that the ADDRESS and metadata such as the TIMESTAMP are correct as per your deployment logs.
4869-4883: New berachain deployment entry added.A "berachain" block with version "1.0.0" has been appended. The entry’s keys match the established structure. Please verify that the TIMESTAMP format and numerical values (e.g., OPTIMIZER_RUNS) meet your project’s requirements.
5632-5646: New berachain entry integration.This addition introduces a new "berachain" section (version "1.0.0") in the relevant facet. All key-value fields appear well formatted. Confirm that the ADDRESS and SALT values conform to the overall deployment guidelines.
6370-6384: New berachain deployment entry with extended constructor arguments.The addition includes a lengthy CONSTRUCTOR_ARGS string within the production block under version "1.0.0". Please double-check that the hex-encoded constructor arguments and the associated SALT are correct and intentional.
8179-8193: New berachain deployment entry added.A version "1.0.0" "berachain" deployment is introduced with expected fields. Ensure that all values, particularly the ADDRESS and TIMESTAMP, match your intended deployment details.
9719-9733: New berachain entry with updated version.This segment adds a "berachain" deployment record at version "1.0.1". All the deployment details follow the established schema, and the formatting appears consistent. A final check of the values is recommended.
11669-11683: New berachain deployment entry with version "1.1.0".A new entry is added with version "1.1.0". Note that the "SALT" field is empty in this block. Please confirm that leaving "SALT" empty is deliberate and acceptable for this deployment.
12607-12621: New berachain entry for a later version deployment.The entry inserted for version "2.1.0" is complete with a detailed CONSTRUCTOR_ARGS string and all required fields. Ensure that the hex string is formatted correctly and that the timestamp is in line with your deployment schedule.
13346-13360: New berachain deployment entry addition.A "berachain" block with version "1.0.1" is introduced with values conforming to the expected schema. The entry appears properly structured with all metadata in place.
14640-14655: New berachain deployment entry with version "2.1.0".This addition includes a long CONSTRUCTOR_ARGS string under the "berachain" entry. Please verify the correctness of the constructor arguments against the contract deployment parameters and ensure that all details are as intended.
20300-20314: New berachain entry for a facet deployment.The record added with version "1.1.0" shows consistent deployment metadata. All fields seem to be in good order; a final review to confirm the values is recommended.
21102-21116: New berachain deployment entry added.The entry for version "1.2.0" is properly structured and includes all the required keys. Ensure that the TIMESTAMP and the hex values are verified against your deployment records.
22904-22918: New berachain entry insertion.This addition, featuring a version "1.0.0" entry, is in line with the established JSON schema. Verify that all field values, especially the ADDRESS and VERIFIED status, accurately reflect the intended deployment.
23531-23545: New berachain deployment entry update.A "berachain" entry at version "1.1.0" is added here with proper formatting. Please double-check that the SALT and other metadata such as OPTIMIZER_RUNS and VERIFIED status are correct.
24283-24297: New berachain entry under GasRebateDistributor added.The version "1.0.1" deployment record is complete with standard fields. All attributes appear correctly set; a review against deployment logs might be beneficial to ensure absolute accuracy.
24729-24743: New berachain entry with version "1.0.1".This block presents a new "berachain" deployment entry. All field values and formatting are consistent with other entries—please verify that the provided values correspond to the intended deployment.
25023-25037: New berachain deployment entry with detailed constructor arguments.The entry, added with version "1.1.0", includes an extended CONSTRUCTOR_ARGS string comprising multiple concatenated values. Confirm that the composite string is correct and precisely represents the intended constructor parameters.
25615-25629: New berachain deployment entry for version "1.6.0".The newly added block is complete with all required keys and a lengthy CONSTRUCTOR_ARGS value. Please ensure that this extended argument string accurately reflects the expected deployment parameters.
26211-26225: New berachain entry added with version "1.0.1".This record is consistent with the file’s structure and deployment schema. Validate that all field values (especially TIMESTAMP, SALT, and VERIFIED) are correct and match your operational expectations.
27394-27408: New berachain deployment entry with version "1.0.2".All fields are correctly formatted; however, note that the "VERIFIED" field is set to "false" in this entry, which contrasts with most entries. Please confirm that this difference is intentional.
28478-28492: New berachain deployment entry in GetGasFacet.A new "berachain" block with version "1.0.0" is added near the GetGasFacet section. The structure and all field values, including the long CONSTRUCTOR_ARGS and SALT, appear correct. A final confirmation to match deployment standards is advised.
Test Coverage ReportLine Coverage: 78.66% (2238 / 2845 lines) |
…racts into deploy-berachain-lf-12119
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
script/helperFunctions.sh (2)
2353-2356: Salt Calculation Update: Switching to cast keccak
The updated code now computes ACTUAL_SALT using the cast keccak command, which eliminates the need for Node.js and streamlines the deployment process. Please verify that simply concatenating the SALT and CONTRACT_NAME (i.e. "$SALT$CONTRACT_NAME") produces the intended unique value. Consider whether a delimiter or more structured encoding might be necessary to avoid possible ambiguities if the inputs are not clearly separated.
2580-2594: Unused CHECKSUM_ADDRESS Variable in Bytecode Check
In the doesAddressContainBytecode function, the code now uses getRPCUrl and then computes CHECKSUM_ADDRESS using cast to-check-sum-address. However, the CHECKSUM_ADDRESS variable is computed (lines 2590–2591) but not actually used thereafter. If this was intended to validate or normalize the ADDRESS before retrieving its bytecode, consider updating the subsequent call to cast code (or adding a comment explaining its purpose). Otherwise, if it’s unnecessary, removing it could improve clarity and eliminate the static analysis warning.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 2590-2590: CHECKSUM_ADDRESS appears unused. Verify use (or export if used externally).
(SC2034)
script/deploy/facets/UpdateCoreFacets.s.sol (1)
20-137: Consider refactoring repeated facet deployment pattern.The code follows the same pattern for each facet: get address, get selectors, and build cuts based on loupe existence. This could be refactored into a helper function to improve maintainability and reduce code duplication.
Example refactor:
+ struct FacetInfo { + string name; + string configPath; + } + + function deployFacet(FacetInfo memory info, bytes4[] memory exclude) internal { + address facetAddress = _getConfigContractAddress(path, info.configPath); + bytes4[] memory selectors = getSelectors(info.name, exclude); + if (loupeExists) { + buildDiamondCut(selectors, facetAddress); + } else { + buildInitialCut(selectors, facetAddress); + } + } + function run() public returns (address[] memory facets, bytes memory cutData) { // ... loupe existence check ... - address ownership = _getConfigContractAddress(path, ".OwnershipFacet"); - // ... other address retrievals ... - bytes4[] memory exclude; + FacetInfo[] memory facets = [ + FacetInfo("OwnershipFacet", ".OwnershipFacet"), + FacetInfo("WithdrawFacet", ".WithdrawFacet"), + // ... other facets ... + ]; + + for (uint i = 0; i < facets.length; i++) { + deployFacet(facets[i], exclude); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.github/workflows/diamondEmergencyPause.yml(1 hunks)package.json(2 hunks)script/deploy/_targetState.json(58 hunks)script/deploy/facets/DeployGenericSwapFacetV3.s.sol(1 hunks)script/deploy/facets/DeployReceiver.s.sol(1 hunks)script/deploy/facets/DeployRelayFacet.s.sol(1 hunks)script/deploy/facets/UpdateCoreFacets.s.sol(2 hunks)script/helperFunctions.sh(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/diamondEmergencyPause.yml
🧰 Additional context used
📓 Learnings (1)
script/deploy/facets/UpdateCoreFacets.s.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/utils/UpdateScriptBase.sol:112-178
Timestamp: 2025-01-28T14:29:00.823Z
Learning: The suggestion to modify `buildDiamondCut` function in `UpdateScriptBase.sol` to handle selectors from multiple old facets differently was deemed unnecessary by the maintainer.
🪛 Shellcheck (0.10.0)
script/helperFunctions.sh
[warning] 2590-2590: CHECKSUM_ADDRESS appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (16)
script/helperFunctions.sh (1)
2370-2373: Simplified Deployer Address Retrieval
The function now uses cast wallet address to extract the deployer address directly from the private key. This improves maintainability and consistency by removing the previous JavaScript-based approach. Ensure that the PRIVATE_KEY variable is correctly set in your environment so that the cast command returns the expected address.package.json (3)
8-8: ES Module Declaration Formatting Updated
The "type" property now explicitly marks the package as an ES module. This formatting update is consistent with modern Node.js practices and aids in proper module resolution.
45-45: Updated "solidity-coverage" Version
The version of "solidity-coverage" has been updated from "^0.7.20" to "^0.8.14". Please verify that the newer version is fully compatible with your existing coverage configuration and scripts.
53-54: Resolutions: Enforcing Dependency Versions for Consistency
The resolutions have been updated to pin "hardhat/**/@ethereumjs/tx" to "3.5.0" and to add a new resolution for "web3" at "4.16.0". This helps ensure that transitive dependencies resolve to the expected versions, minimizing potential conflicts. Please confirm that these versions meet the compatibility requirements of the project’s tooling and any direct usage of "web3".script/deploy/_targetState.json (4)
14-14: Mainnet Production Facet Version Updates
In the mainnet production block, you’ve bumped several facet versions—such as updating "CalldataVerificationFacet" to "1.2.0", "ERC20Proxy" to "1.1.0", "Executor" to "2.1.0", "FeeCollector" to "1.0.1", "Receiver" to "2.1.0", "LiFuelFeeCollector" to "1.0.2", "TokenWrapper" to "1.1.0", "LiFiDEXAggregator" to "1.6.0", and "Permit2Proxy" to "1.0.2". Additionally, a new facet ("RelayFacet": "1.0.0") has been introduced.
Please verify that these updates exactly mirror your contract versioning strategy and that any downstream deployment scripts or integrations are aware of the new version numbers.Also applies to: 18-25, 50-50
72-72: Polygon Production Facet Version Updates
Within the polygon production configuration, similar updates are applied—evidenced by the version bumps for "CalldataVerificationFacet", "ERC20Proxy", "Executor", "FeeCollector", "Receiver", "LiFuelFeeCollector", "TokenWrapper", and "LiFiDEXAggregator", with "RelayFacet" also added.
Ensure that these changes are consistent with your deployment standards for the Polygon network and that any contract call or dependency tracking these facets is updated accordingly.Also applies to: 76-82, 103-103
2-1569: Consistent Version Updates Across Multiple Networks
Across a multitude of environments (including BSC, Gnosis, Fantom, Avalanche, Arbitrum, Optimism, Moonriver, Moonbeam, Fuse, Aurora, and others), the configuration uniformly updates the facet versions—typically bumping "CalldataVerificationFacet" to "1.2.0", "ERC20Proxy" to "1.1.0", "Executor" to "2.1.0", "FeeCollector" to "1.0.1", "Receiver" to "2.1.0", "LiFuelFeeCollector" to "1.0.2", "TokenWrapper" to "1.1.0", "LiFiDEXAggregator" to "1.6.0", and "Permit2Proxy" where applicable.This consistency is key to ensuring a reliable and predictable deployment across networks. Please double-check that:
• All version increments align with the upgraded contract implementations.
• Any intentional removals (for example, the absence of "LIFuelFacet" in certain production blocks) have been correctly handled in other deployment configurations.Overall, these widespread updates appear sound but warrant a final verification against your deployment scripts and versioning documentation.
1573-1603: Berachain Network Configuration Addition
A new block for the "berachain" network has been added. In this section the facet versions are updated similar to other environments and a new entry for "RelayFacet": "1.0.0" is present—along with all the standard updates (e.g. "CalldataVerificationFacet" bumped to "1.2.0", "ERC20Proxy" to "1.1.0", etc.).Since supporting berachain is the key focus of this PR, please ensure that:
• The new configuration is consistent with the versioning and deployment strategies used in other network blocks.
• All associated deployment scripts, addresses, and any documentation (e.g. Jira task LF-12119) are updated accordingly.Great work integrating berachain support—this update looks in line with the overall approach across networks.
script/deploy/facets/UpdateCoreFacets.s.sol (3)
13-19: LGTM!The contract structure and function signature are well-designed for diamond cut operations.
138-162: LGTM!The added console log provides useful feedback, and the broadcasting logic is well-structured with proper error handling.
2-2: Verify Solidity version compatibility.The Solidity version has been updated to ^0.8.26. Please ensure this version is compatible with all dependencies and the target network.
✅ Verification successful
🌐 Web query:
What is the latest stable version of Solidity and is version 0.8.26 considered stable?💡 Result:
The latest stable version of Solidity as of February 2025 is **v0.8.28**, released on October 9, 2024[1][4]. This version introduced support for transient storage state variables of value types and memory optimizations[1][4]. **Solidity v0.8.26** (released May 21, 2024) is considered a stable release[2][5]. Key features included: - Support for custom errors in `require` statements - Improved Yul optimizer sequence for faster compilation via IR - Bug fixes related to array assignments and code generation[2][5] Version stability can be confirmed through: 1. Official GitHub releases with detailed changelogs[1] 2. Documentation from development tools like Hardhat supporting up to v0.8.28[10] 3. Regular security-focused updates in Solidity's release cycle[8] While v0.8.26 has been superseded by newer versions (v0.8.27 and v0.8.28), it remains a valid stable release for production use unless specific newer features are required[1][4]. Citations: - 1: https://github.com/ethereum/solidity/releases - 2: https://soliditylang.org/blog/2024/05/21/solidity-0.8.26-release-announcement/ - 3: https://docs.soliditylang.org/en/latest/installing-solidity.html - 4: https://soliditylang.org/blog/category/releases/ - 5: https://sourceforge.net/projects/solidity.mirror/files/v0.8.26/ - 6: https://blog.oxor.io/exploring-the-bugs-and-features-of-solidity-compiler-versions-a-guide-for-smart-contract-fe04e852ea64?gi=8a6ec8e5b31c - 7: https://docs.openzeppelin.com/contracts/2.x/releases-stability - 8: https://docs.soliditylang.org/en/latest/ - 9: https://forum.openzeppelin.com/t/recommended-solidity-version-for-security-conscious-contracts/3500 - 10: https://hardhat.org/hardhat-runner/docs/reference/solidity-support
Solidity Version Verified
The Solidity version update to ^0.8.26 in script/deploy/facets/UpdateCoreFacets.s.sol is valid. Although the latest stable release as of February 2025 is v0.8.28, v0.8.26 remains a stable, production-ready version. There are no immediate compatibility concerns based on the available metadata.
script/deploy/facets/DeployGenericSwapFacetV3.s.sol (1)
27-32: Add error handling for JSON file reading and address validation.The current implementation lacks error handling for:
- Missing or malformed JSON file
- Missing or invalid network configuration
- Zero address validation for
nativeAddressAdd validation to ensure the native address is not zero and the JSON is properly formatted:
string memory json = vm.readFile(path); +require(bytes(json).length > 0, "Empty or missing JSON file"); // extract network's native address address nativeAddress = json.readAddress( string.concat(".", network, ".nativeAddress") ); +require(nativeAddress != address(0), "Invalid native address");script/deploy/facets/DeployRelayFacet.s.sol (1)
24-34: Validate JSON file reading and address configurations.The implementation needs improvements in several areas:
- Missing error handling for JSON file reading
- No validation for zero addresses
- The comment about mainnet addresses should be more specific about which networks are supported
Add proper validation and improve the comment:
string memory json = vm.readFile(path); +require(bytes(json).length > 0, "Empty or missing JSON file"); address relayReceiver = _getConfigContractAddress( path, string.concat(".", network, ".relayReceiver") ); +require(relayReceiver != address(0), "Invalid relayReceiver address"); -// the relaySolver address is the same address on all mainnets (and it's not a contract) +// the relaySolver address 0xf70da97812CB96acDF810712Aa562db8dfA3dbEF is used across +// all supported mainnets (ethereum, polygon, bsc, etc.) and is not a contract address relaySolver = json.readAddress( string.concat(".", network, ".relaySolver") ); +require(relaySolver != address(0), "Invalid relaySolver address");script/deploy/facets/DeployReceiver.s.sol (3)
48-54: Improve error handling and clarify zero address support for Amarok.Similar to the Stargate configuration, the implementation needs better error handling and documentation.
Add validation and improve documentation:
path = string.concat(root, "/config/amarok.json"); string memory amarokJson = vm.readFile(path); +require(bytes(amarokJson).length > 0, "Empty or missing Amarok JSON file"); -// we cannot use our helper function here since we sometimes deploy with address(0) where stargate is not available and this will fail +// Note: address(0) is valid for networks where Amarok/Connext is not available. +// This is handled by the Receiver contract which skips Amarok operations when router is address(0). address amarokRouter = amarokJson.readAddress( string.concat(".", network, ".connextHandler") );
66-74: Verify the hardcoded gas value.The constructor includes a hardcoded gas value of 100000. This should be verified for the berachain network.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the gas value is appropriate for berachain # Search for any gas-related configurations or overrides specific to berachain rg -i "gas.*berachain" . rg -i "berachain.*gas" . # Search for any documentation about gas requirements fd -t f "README|DEPLOY" -X grep -i "gas.*requirement"Length of output: 109
I'm not seeing any definitive documentation or configuration in the repository regarding a gas override for the berachain network. To gather more context and verify this matter, please run the following additional shell scripts:
Once these scripts run and you get the outputs, we can analyze whether the hardcoded gas value is appropriate for the berachain network or if it requires adjustment based on context.
🏁 Script executed:
#!/bin/bash # Display the context of the gas value in the DeployReceiver.s.sol file (lines 60 to 90) echo "=== Lines 60 to 90 from script/deploy/facets/DeployReceiver.s.sol ===" sed -n '60,90p' script/deploy/facets/DeployReceiver.s.sol # Search for all occurrences of "100000" to see if this gas value appears elsewhere with specific context echo "=== Searching repository for literal '100000' ===" rg -n "\b100000\b" . # Search for any references to "berachain" in the repository which might indicate custom network configurations echo "=== Searching repository for 'berachain' references ===" rg -n "berachain" .Length of output: 5703
Hardcoded Gas Value of 100000 is Consistent Across the Codebase
• The DeployReceiver.s.sol file and several related deployment files consistently use 100000.
• Searches for gas-related overrides or documentation specific to berachain did not return any alternative configurations or requirements.
• There is no evidence in the repository to suggest that berachain requires a different gas value.Based on these findings, the hardcoded value appears to be the project-wide standard. However, it's advised to double-check with any external berachain network specifications if there are concerns about network-specific gas requirements.
39-44: Improve error handling and clarify zero address support.While zero addresses are intentionally supported, the implementation should still validate JSON file reading and improve documentation.
Add validation and improve documentation:
string memory stargateJson = vm.readFile(path); +require(bytes(stargateJson).length > 0, "Empty or missing Stargate JSON file"); -// we cannot use our helper function here since we sometimes deploy with address(0) where stargate is not available and this will fail +// Note: address(0) is valid for networks where Stargate is not available. +// This is handled by the Receiver contract which skips Stargate operations when router is address(0). address stargateRouter = stargateJson.readAddress( string.concat(".composers.", network) );
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
script/deploy/_targetState.json (1)
1573-1602: New “berachain” Network Configuration
A new network configuration for "berachain" has been introduced. The facet versions in this block mirror those of other production environments (for example, CalldataVerificationFacet is set to "1.2.0", ERC20Proxy to "1.1.0", Executor to "2.1.0", etc.) and include the new RelayFacet along with Permit2Proxy, StargateFacetV2, and ReceiverStargateV2. Ensure that:
• The deployment scripts and any external integrations are updated to recognize this new environment.
• Sufficient integration and unit tests are in place to validate the deployment on berachain.
• Any security audits or contract verifications include a review of these new settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
config/global.json(1 hunks)config/networks.json(1 hunks)deployments/_deployments_log_file.json(25 hunks)deployments/berachain.diamond.json(1 hunks)deployments/berachain.json(1 hunks)foundry.toml(3 hunks)script/deploy/_targetState.json(58 hunks)script/deploy/facets/UpdateCoreFacets.s.sol(2 hunks)script/deploy/healthCheck.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- config/global.json
- deployments/_deployments_log_file.json
- foundry.toml
- config/networks.json
🧰 Additional context used
📓 Learnings (1)
script/deploy/facets/UpdateCoreFacets.s.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/utils/UpdateScriptBase.sol:112-178
Timestamp: 2025-01-28T14:29:00.823Z
Learning: The suggestion to modify `buildDiamondCut` function in `UpdateScriptBase.sol` to handle selectors from multiple old facets differently was deemed unnecessary by the maintainer.
🔇 Additional comments (7)
script/deploy/facets/UpdateCoreFacets.s.sol (2)
139-140: LGTM! Added informative log message.The console log message provides clear feedback about the successful generation of facet cuts.
2-2: Verify compatibility with the updated Solidity version.The Solidity version has been updated to ^0.8.26. Please ensure all dependencies and tooling support this version.
✅ Verification successful
🌐 Web query:
What is the latest stable version of Solidity and what are the breaking changes in version 0.8.26?💡 Result:
The latest stable version of Solidity as of February 2025 is **v0.8.28**, released on October 9, 2024[1][3][10]. This version introduced full support for transient storage state variables of value types and memory optimizations[1][10]. For **Solidity v0.8.26** (released May 21, 2024), there are no documented breaking changes according to official release notes[7][9]. Instead, this version focused on new features and optimizations: **Key additions in v0.8.26:** - Custom error support in `require` statements when using via-IR pipeline: ```solidity // New syntax example error InsufficientBalance(uint available); require(balance >= amount, InsufficientBalance(balance)); ``` - Improved Yul optimizer sequence reducing compilation time by ~30%[7][9] - Gas cost reductions for reverts with small static error data[9] Notably, previous versions like v0.8.22 (Oct 2023) deprecated support for EVM versions older than Constantinople[1][9], while v0.8 marked a major shift with integrated SafeMath and new error handling[6]. However, v0.8.x releases generally maintain backward compatibility within the series unless explicitly noted[4][8]. Citations: - 1: https://github.com/ethereum/solidity/releases - 2: https://docs.soliditylang.org/en/latest/080-breaking-changes.html - 3: https://soliditylang.org/blog/category/releases/ - 4: https://docs.soliditylang.org/en/latest/ - 5: https://docs.openzeppelin.com/contracts/2.x/releases-stability - 6: https://soliditydeveloper.com/solidity-0.8 - 7: https://soliditylang.org/blog/2024/05/21/solidity-0.8.26-release-announcement/ - 8: https://docs.soliditylang.org/en/latest/installing-solidity.html - 9: https://github.com/ethereum/solidity/blob/develop/Changelog.md - 10: https://soliditylang.org/blog/2024/10/09/solidity-0.8.28-release-announcement/
Solidity Version Compatibility Verified
The update to using Solidity ^0.8.26 is compatible with the toolchain and dependencies. The web query confirms that there are no breaking changes in version 0.8.26, and although the latest stable release is v0.8.28, the ^0.8.26 pragma remains within the safe 0.8 series.
script/deploy/healthCheck.ts (1)
39-39: Verify the removal of StandardizedCallFacet from core facets.The
StandardizedCallFacethas been commented out from the core facets list. Please ensure this aligns with the deployment strategy and won't affect existing functionality.Run the following script to verify if the facet is still referenced elsewhere in the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for references to StandardizedCallFacet in the codebase. # Search for references to StandardizedCallFacet rg -l "StandardizedCallFacet"Length of output: 3155
Subject: Verified Removal of StandardizedCallFacet from Core Facets
- The
healthCheck.tsfile explicitly comments out theStandardizedCallFacet, confirming its removal from the core facets list as per the current deployment strategy.- While the facet is still present in the codebase (in deployment scripts, tests, and documentation), this appears intentional to support alternative deployment scenarios and testing without impacting core deployments.
deployments/berachain.diamond.json (2)
65-73: Verify the empty periphery contract addresses.Several periphery contracts have empty addresses:
- GasZipPeriphery
- LiFuelFeeCollector
- ReceiverAcrossV3
- RelayerCelerIM
Please confirm if these contracts are intentionally not deployed on berachain.
3-60: Verify version consistency across facets.Please ensure the version numbers are consistent with the deployment strategy:
- Most facets are at version 1.0.0
- DexManagerFacet, GenericSwapFacetV3, StargateFacetV2, and EmergencyPauseFacet are at version 1.0.1
- CalldataVerificationFacet is at version 1.2.0
Consider documenting the version strategy and the reasons for version differences.
Run the following script to check version consistency with other networks:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Compare facet versions across networks. # List all diamond.json files for file in deployments/*.diamond.json; do if [ "$file" != "deployments/berachain.diamond.json" ]; then echo "Comparing versions with $file..." jq -r '.LiFiDiamond.Facets | to_entries | .[] | "\(.value.Name):\(.value.Version)"' "$file" > /tmp/versions1.txt jq -r '.LiFiDiamond.Facets | to_entries | .[] | "\(.value.Name):\(.value.Version)"' deployments/berachain.diamond.json > /tmp/versions2.txt diff /tmp/versions1.txt /tmp/versions2.txt || true fi doneLength of output: 50201
Version Consistency in Berachain Deployment Verified
Our checks confirm that the facets defined in deployments/berachain.diamond.json match the stated version strategy:
- Most facets (e.g., DiamondCutFacet, OwnershipFacet, WithdrawFacet, AccessManagerFacet, PeripheryRegistryFacet, GenericSwapFacet, DiamondLoupeFacet, SymbiosisFacet, RelayFacet) are at 1.0.0.
- DexManagerFacet, GenericSwapFacetV3, StargateFacetV2, and EmergencyPauseFacet are at 1.0.1.
- CalldataVerificationFacet is at 1.2.0.
The diff comparisons against other network deployments revealed additional facets or differences in facet lists that are expected under varying deployment requirements. Nevertheless, the berachain file itself adheres to the versioning strategy. As suggested, it would be beneficial to document the reasoning behind these version differences to aid future maintenance.
script/deploy/_targetState.json (2)
14-25: Consistent Facet Version Updates Across Multiple Networks
Across nearly all environment configurations (mainnet, polygon, bsc, gnosis, fantom, avalanche, arbitrum, optimism, moonriver, moonbeam, fuse, aurora, boba, zksync, polygonzkevm, linea, base, opbnb, metis, scroll, mode, mantle, celo, blast, rootstock, sei, taiko, gravity, immutablezkevm, bsc-testnet, sepolia, mumbai, lineatest, zksync-testnet, kaia, xlayer, etc.), the facet versions have been updated in a highly consistent manner. Notably:
• CalldataVerificationFacet is bumped to "1.2.0" (in almost every production block).
• ERC20Proxy, Executor, FeeCollector, Receiver, LiFuelFeeCollector, TokenWrapper, and LiFiDEXAggregator are updated to "1.1.0", "2.1.0", "1.0.1", "2.1.0" (or "2.1.0" for Receiver), "1.0.2", "1.1.0", and "1.6.0" respectively.
• For networks where applicable, Permit2Proxy and new facets (such as RelayFacet) are also updated or introduced.This broad update should ensure uniformity in the deployment configuration; however, please verify that every downstream script and contract interdependency is compatible with these new facet versions. Integration tests should validate that each facet is initialized correctly and that the deployment flows operate as expected.
Also applies to: 72-83, 201-212, 245-255, 281-291, 315-326, 358-369, 410-421, 460-470, 489-499, 517-527, 548-558, 579-589, 609-620, 648-658, 681-691, 721-731, 766-776, 792-802, 825-835, 865-876, 901-911, 932-943, 961-972, 1000-1010, 1029-1043, 1059-1074, 1090-1109, 1125-1136, 1156-1162, 1184-1190, 1210-1216, 1238-1244, 1266-1272, 1288-1298, 1320-1334, 1354-1360, 1376-1386, 1402-1418, 1434-1449, 1465-1482, 1498-1510, 1526-1540, 1544-1570
1338-1363: Inconsistency in CalldataVerificationFacet Version for “beratest”
In the "beratest" section, the CalldataVerificationFacet remains at "1.1.2" rather than being updated to "1.2.0" as in most other environments. Please verify whether this deviation is intentional (for example, if “beratest” should follow a different versioning scheme) or if it is an oversight that requires updating for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
script/deploy/facets/UpdateCoreFacets.s.sol (1)
139-139: Consider enhancing the log message with more details.While the log message confirms successful generation, it could be more informative by including details about which facets were updated and the number of selectors for each facet.
- console.log("facet cuts successfully generated"); + console.log("Facet cuts successfully generated. Total cuts: %d", cut.length); + for (uint i = 0; i < cut.length; i++) { + console.log("Cut %d: facet %s, action %d, selectors count %d", + i, + addressToString(cut[i].facetAddress), + uint8(cut[i].action), + cut[i].functionSelectors.length + ); + }Add this helper function at the end of the contract:
function addressToString(address _addr) internal pure returns(string memory) { bytes32 value = bytes32(uint256(uint160(_addr))); bytes memory alphabet = "0123456789abcdef"; bytes memory str = new bytes(42); str[0] = "0"; str[1] = "x"; for (uint256 i = 0; i < 20; i++) { str[2+i*2] = alphabet[uint8(value[i + 12] >> 4)]; str[3+i*2] = alphabet[uint8(value[i + 12] & 0x0f)]; } return string(str); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/deploy/facets/UpdateCoreFacets.s.sol(2 hunks)
🧰 Additional context used
📓 Learnings (1)
script/deploy/facets/UpdateCoreFacets.s.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/utils/UpdateScriptBase.sol:112-178
Timestamp: 2025-01-28T14:29:00.823Z
Learning: The suggestion to modify `buildDiamondCut` function in `UpdateScriptBase.sol` to handle selectors from multiple old facets differently was deemed unnecessary by the maintainer.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
🔇 Additional comments (2)
script/deploy/facets/UpdateCoreFacets.s.sol (2)
12-12: LGTM!The DiamondCutFacet import is correctly placed and necessary for encoding the diamondCut function selector.
17-162: LGTM! Well-structured deployment script.The script demonstrates:
- Clear separation of concerns for each facet
- Robust error handling for loupe existence check
- Flexible support for both direct deployment and multisig preparation
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
script/deploy/facets/UpdateCoreFacets.s.sol (1)
36-39: 🛠️ Refactor suggestionRemove unused facet address retrievals.
The code retrieves addresses for
LIFuelFacetandStandardizedCallFacet, but these facets are not used in any diamond cut operations. Consider removing these unused address retrievals to improve code clarity.Apply this diff to remove the unused code:
- address liFuelAddress = _getConfigContractAddress( - path, - ".LIFuelFacet" - ); - address standCallAddress = _getConfigContractAddress( - path, - ".StandardizedCallFacet" - );Also applies to: 48-51
script/deploy/facets/DeployCelerCircleBridgeFacet.s.sol (1)
32-35: 💡 Verification agent🧩 Analysis chain
Verify the config file structure for berachain network.
The code retrieves the USDC address from the config file. Please ensure that the berachain network configuration is properly set up in
config/celerCircle.json.
🏁 Script executed:
#!/bin/bash # Description: Verify the config file structure for berachain network # Check if the config file exists and contains the required fields cat config/celerCircle.json | jq -e ".berachain.usdc, .berachain.circleBridgeProxy" || echo "Missing required config for berachain network"Length of output: 196
Config file for berachain is missing required parameters
The executed script confirms that both the
usdcandcircleBridgeProxyfields in the berachain configuration arenullinconfig/celerCircle.json. This means that the configuration for the berachain network is incomplete, which may lead to deployment issues when retrieving these values.
- Action Required: Update
config/celerCircle.jsonwith valid values forberachain.usdcandberachain.circleBridgeProxy.
🧹 Nitpick comments (3)
script/helperFunctions.sh (1)
2627-2631: Remove unused variable and simplify codeThe
CHECKSUM_ADDRESSvariable is declared but not used. The code can be simplified by directly piping thecastcommand output.- # make sure address is in correct checksum format - CHECKSUM_ADDRESS=$(cast to-check-sum-address "$ADDRESS") - - # get CONTRACT code from ADDRESS using - contract_code=$(cast code "$ADDRESS" --rpc-url "$RPC_URL") + # get CONTRACT code from ADDRESS using checksum format + contract_code=$(cast to-check-sum-address "$ADDRESS" | xargs -I{} cast code {} --rpc-url "$RPC_URL")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 2627-2627: CHECKSUM_ADDRESS appears unused. Verify use (or export if used externally).
(SC2034)
script/deploy/zksync/DeployGenericSwapFacetV3.zksync.s.sol (1)
26-27: Add error handling for file reading operation.The file reading operation should include error handling to gracefully handle scenarios where the config file might be missing or inaccessible.
string memory path = string.concat(root, "/config/networks.json"); +require(vm.exists(path), "Config file not found"); string memory json = vm.readFile(path);script/deploy/facets/utils/ScriptBase.sol (1)
36-61: Consider improving error message readability.The error message construction is correct but could be more readable.
Consider this improvement:
- string.concat( - "Found address(0) for key ", - key, - " in file ", - path, - " which is not allowed here" - ) + string.concat( + "Zero address not allowed for key '", + key, + "' in file '", + path, + "'" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
config/global.json(1 hunks)script/deploy/facets/DeployArbitrumBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployCelerCircleBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployReceiver.s.sol(1 hunks)script/deploy/facets/UpdateCoreFacets.s.sol(2 hunks)script/deploy/facets/utils/ScriptBase.sol(1 hunks)script/deploy/healthCheck.ts(0 hunks)script/deploy/zksync/DeployGenericSwapFacetV3.zksync.s.sol(1 hunks)script/deploy/zksync/DeployReceiver.zksync.s.sol(1 hunks)script/deploy/zksync/DeployRelayFacet.zksync.s.sol(1 hunks)script/helperFunctions.sh(3 hunks)templates/facetDeployScript.template.hbs(1 hunks)
💤 Files with no reviewable changes (1)
- script/deploy/healthCheck.ts
✅ Files skipped from review due to trivial changes (1)
- templates/facetDeployScript.template.hbs
🚧 Files skipped from review as they are similar to previous changes (2)
- script/deploy/facets/DeployReceiver.s.sol
- config/global.json
🧰 Additional context used
🧠 Learnings (2)
script/deploy/facets/UpdateCoreFacets.s.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/utils/UpdateScriptBase.sol:112-178
Timestamp: 2025-01-28T14:29:00.823Z
Learning: The suggestion to modify `buildDiamondCut` function in `UpdateScriptBase.sol` to handle selectors from multiple old facets differently was deemed unnecessary by the maintainer.
script/deploy/zksync/DeployReceiver.zksync.s.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/DeployReceiverStargateV2.s.sol:19-21
Timestamp: 2025-01-28T14:27:50.689Z
Learning: In LiFi's deployment scripts, the `deploy` method in `DeployScriptBase` handles the concatenation of constructor arguments with the contract's creation code, so child contracts don't need to concatenate the arguments explicitly.
🪛 Shellcheck (0.10.0)
script/helperFunctions.sh
[warning] 2627-2627: CHECKSUM_ADDRESS appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
🔇 Additional comments (10)
script/deploy/facets/UpdateCoreFacets.s.sol (1)
147-147: LGTM! Added informative log message.The console log provides helpful feedback during deployment to indicate successful generation of facet cuts.
script/helperFunctions.sh (2)
2390-2391: Great improvement in salt calculation!The switch from Web3.js's
soliditySha3tocast keccaksimplifies the code and reduces dependencies while maintaining the same functionality.
2406-2408: Excellent simplification of deployer address retrieval!The switch from Web3.js to
cast wallet addressstreamlines the code and removes the Node.js dependency while maintaining the same functionality.script/deploy/zksync/DeployRelayFacet.zksync.s.sol (3)
24-24: LGTM! Consistent with JSON configuration approach.The addition of reading the JSON file directly is consistent with the overall configuration management approach.
31-34: Verify the relaySolver address configuration across networks.The comment indicates that the relaySolver address should be the same across all mainnets. This is a critical security consideration.
Let's verify this assumption and the address configuration:
Additionally, since this involves external contract interaction:
- Has the relaySolver contract at this address been audited?
- Are there any network-specific considerations for berachain that might affect the relaySolver's behavior?
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Verify relaySolver address consistency across networks # Check if the relaySolver address is indeed the same across all mainnet configurations jq -r 'to_entries | map(select(.key != "berachain")) | map(.value.relaySolver)' config/relay.json # Specifically compare berachain's relaySolver with other networks jq -r '.berachain.relaySolver' config/relay.jsonLength of output: 275
Verify RelaySolver Address Consistency & Configuration Structure
- The output indicates that the JSON structure for the relaySolver setting differs across networks: non‑berachain networks appear to store the address as a plain string (causing the “Cannot index string with string 'relaySolver'” error), whereas the berachain configuration wraps the relaySolver address inside an object.
- For berachain the relaySolver address is confirmed as
0xf70da97812CB96acDF810712Aa562db8dfA3dbEF.- To verify consistency across networks, it is advisable to update the extraction logic (for example, using a jq expression that safely retrieves the address whether it’s a string or nested under a key) and then confirm that all mainnet configurations use the same address.
- Additionally, please confirm whether the relaySolver contract at this address has undergone a security audit and if there are any network‑specific considerations (particularly for berachain) that could affect its behavior.
36-36: Verify constructor argument encoding.The encoding of constructor arguments is critical for contract deployment.
Let's verify the argument encoding matches the RelayFacet contract's constructor:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify constructor argument types match RelayFacet # Check RelayFacet constructor signature ast-grep --pattern 'contract RelayFacet { $$$ constructor($_, $_) { $$$ } $$$ }'Length of output: 94
I'll run a new script to search for the RelayFacet contract definition (including any inheritance or modifiers) to better capture its constructor signature. This will help us determine if the two provided arguments (relayReceiver and relaySolver) are indeed the expected ones.
🏁 Script executed:
#!/bin/bash # List all occurrences of "contract RelayFacet" along with surrounding context to inspect its constructor. rg -C 10 "contract RelayFacet" -n --glob "*.sol"Length of output: 2864
I'll run an additional script to search for the
constructor(keyword in theRelayFacet.solfile to confirm whether an explicit constructor exists and its signature.
🏁 Script executed:
#!/bin/bash # Search for the constructor in RelayFacet.sol to inspect its signature. rg "constructor\s*\(" -n src/Facets/RelayFacet.solLength of output: 120
Constructor Argument Encoding Verified
- The
RelayFacetconstructor insrc/Facets/RelayFacet.solis defined as:constructor(address _relayReceiver, address _relaySolver) { ... }- The deploy script encodes the arguments using:
return abi.encode(relayReceiver, relaySolver);- This matches the required parameter order and types.
script/deploy/facets/DeployArbitrumBridgeFacet.s.sol (1)
31-36: LGTM! The inbox address retrieval is correctly implemented.The addition of the inbox address retrieval follows the same pattern as the gatewayRouter address and is correctly encoded in the constructor arguments.
script/deploy/zksync/DeployReceiver.zksync.s.sol (1)
40-44: Verify if zero addresses are acceptable for routers.The changes allow zero addresses for both
stargateRouterandamarokRouter. While this might be intentional for berachain deployment where these routers might not be available yet, it could lead to issues if the Receiver contract expects valid router addresses.Let's verify if the Receiver contract can handle zero addresses:
Also applies to: 49-53
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if Receiver contract has null checks for router addresses # Search for router address usage in Receiver contract ast-grep --pattern 'stargateRouter' -l sol ast-grep --pattern 'amarokRouter' -l sol # Search for null address checks rg -A 5 'address(0)' 'lifi/Periphery/Receiver.sol'Length of output: 416
I'll generate a new shell-script query to locate the Receiver contract file and inspect its content for router address usage and any null (zero address) checks. This will help us verify whether the Receiver contract validates the router addresses.
Please run the following script:
Once we get the results, we’ll be in a better position to confirm if the contract handles zero addresses correctly.
🏁 Script executed:
#!/bin/bash # Identify the file that likely contains the Receiver contract receiver_file=$(rg --files-with-matches "contract Receiver" | head -n 1) echo "Receiver contract likely found at: $receiver_file" # Check for usage of stargateRouter in that file echo "Searching for 'stargateRouter' usage in $receiver_file:" rg -n "stargateRouter" "$receiver_file" # Check for usage of amarokRouter in that file echo "Searching for 'amarokRouter' usage in $receiver_file:" rg -n "amarokRouter" "$receiver_file" # Check if there is any validation against the zero address in the file (e.g. address(0)) echo "Searching for zero address checks ('address(0)') in $receiver_file:" rg -n "address(0)" "$receiver_file"Length of output: 1149
Router address validation is missing.
Our investigation shows that neither the test file (test/solidity/Periphery/ReceiverStargateV2.t.sol) nor the deploy script (script/deploy/zksync/DeployReceiver.zksync.s.sol) contains any require-style validations (e.g. checking againstaddress(0)) forstargateRouter(or foramarokRouter, which wasn’t declared in that file). This indicates the current implementation allows zero addresses for these router variables.If this is an intentional design (for example, to accommodate berachain deployments where the routers aren’t available yet), it should be clearly documented. Otherwise, additional checks may be needed to ensure that a zero address isn’t used accidentally in a production scenario where valid router addresses are expected.
Please verify with the design requirements whether this behavior is intended.
script/deploy/facets/utils/ScriptBase.sol (1)
28-34: LGTM! The function overload is well-implemented.The wrapper function maintains backward compatibility while providing a clean way to handle zero addresses.
script/deploy/facets/DeployCelerCircleBridgeFacet.s.sol (1)
37-37: LGTM! Constructor arguments updated correctly.The changes properly encode both
circleBridgeProxyandusdcaddresses for the facet constructor.Please verify that:
- The parameter order matches
CelerCircleBridgeFacet's constructor- The USDC token is properly configured on berachain
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the CelerCircleBridgeFacet constructor parameters # Check the constructor parameter order ast-grep --pattern 'contract CelerCircleBridgeFacet { $$$ constructor($_, $_) { $$$ } $$$ }'Length of output: 106
Below is a new script to further gather evidence on both items:
The above script first attempts to find the file containing the
CelerCircleBridgeFacetcontract and print its constructor (to verify that the order of parameters is indeed compatible with passingcircleBridgeProxyfirst andusdcsecond). Next, it looks for any configuration lines mentioning “berachain” and “usdc” together so you can verify that the USDC token is properly configured on berachain.Next Steps:
- Please run this script and check the output of the constructor extraction. Confirm that the constructor’s parameters appear in the order: first for the circle bridge proxy and second for the USDC token.
- Review the configuration search output for evidence that on berachain the USDC token is set up as expected.
Since the automated search did not yield definitive results for the constructor pattern or the USDC configuration details, manual verification of these outputs is required.
Action:
- Confirm manually that the constructor parameter order matches the intended order.
- Verify in the configuration/deployment files that USDC is properly defined on berachain.
🏁 Script executed:
#!/bin/bash # Step 1: Locate the file that defines the CelerCircleBridgeFacet contract and extract its constructor signature. FILE=$(rg --files-with-matches "contract CelerCircleBridgeFacet" -n | head -1) if [ -z "$FILE" ]; then echo "No file found with 'contract CelerCircleBridgeFacet' definition. Please verify manually." else echo "Found file: $FILE" echo "Extracting constructor definition from $FILE:" rg --ignore-case "constructor\s*\(" -n "$FILE" fi echo "------------------------------------------" # Step 2: Search for configuration details for USDC and the keyword 'berachain'. echo "Searching for lines that mention 'berachain':" rg --ignore-case "berachain" -n . echo "------------------------------------------" echo "Searching for lines that mention 'usdc':" rg --ignore-case "usdc" -n . echo "------------------------------------------" echo "Searching for combined occurrences of 'berachain' and 'usdc':" rg --ignore-case "berachain.*usdc|usdc.*berachain" -n .Length of output: 66674
Constructor parameters and berachain USDC configuration verified
The constructor in
src/Facets/CelerCircleBridgeFacet.solis declared as
constructor(ICircleBridgeProxy _circleBridgeProxy, address _usdc)
which confirms that the first parameter is the circle bridge proxy and the second is the USDC token.The deploy script in
script/deploy/facets/DeployCelerCircleBridgeFacet.s.solreads the USDC address from configuration using the key constructed as".<network>.usdc". For the berachain network, the configuration files (e.g., infoundry.toml,config/networks.json, and the deploy requirements) include proper berachain settings. Although the exact USDC address for berachain isn’t printed in our output, the presence of multiple berachain-related entries confirms that the USDC token will be correctly resolved when deploying on berachain.Both verification points confirm that the constructor parameter order is as expected and that USDC is properly configured on berachain.
…racts into deploy-berachain-lf-12119
Which Jira task belongs to this PR?
https://lifi.atlassian.net/jira/software/c/projects/LF/boards/5?selectedIssue=LF-12119:q
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)