LF-11894 zkevm update scripts #953
Conversation
WalkthroughThis update introduces new Solidity contracts and utilities to streamline the deployment and updating of diamond facets with a focus on ZkSync. A new contract is added to deploy the RelayFacet, while base contracts manage environment-based configurations and diamond operations. Several shell scripts have been modified to use an enhanced zkEVM network check via the new Changes
Possibly related PRs
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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
🧹 Nitpick comments (11)
script/deploy/zksync/utils/UpdateScriptBase.sol (4)
51-62: Implement stubs or placeholders forgetExcludesandgetCallData.Both functions are declared as
virtualbut have empty bodies. Assure these placeholders get properly implemented or documented so future contributors know how to override them.
112-178: Consider refactoring for clarity in selector handling.The logic for adding, replacing, or removing selectors is well structured but somewhat lengthy. Grouping repeated checks or extracting subroutines might improve readability and maintainability.
180-191: Evaluate potential unification withbuildDiamondCut.
buildInitialCutoverlaps semantically with parts ofbuildDiamondCut. Consolidating these could reduce code duplication, depending on the diamond cut logic needed.
239-266: Fix inconsistent comment or rename local variable.The comment says “get refund wallet address” while the code reads
.deployerWallet. Adjust the comment or variable name to avoid confusion and ensure clarity for maintainers.- // get refund wallet address from global config file + // get deployer wallet address from global config filescript/deploy/zksync/UpdateRelayFacet.zksync.s.sol (2)
6-12: Restrict deployment if necessary.The
run()function ispublic. If this script is intended only for owners or operators, consider limiting its visibility or adding a permission check to avoid unintended usage.
7-12: Consider contract naming clarity.While
DeployScriptworks, you might opt for a name likeUpdateRelayFacetScriptto better reflect that this contract updates the RelayFacet. This helps future maintainers find relevant scripts quickly.script/deploy/zksync/utils/ScriptBase.sol (2)
28-45: Enforce stronger checks for the configuration path and key.
_getConfigContractAddressreverts if the address is not a contract but does not verify the JSON file’s existence or the presence of the key. Consider adding error handling for missing or malformed JSON config.
41-43: Use the custom errorNotAContractto enhance clarity.You’ve defined
NotAContract(string key)but revert with a string-built message. Using the custom error consistently makes error handling more uniform and helps highlight the specific condition.- revert( - string.concat(key, " in file ", path, " is not a contract") - ); + revert NotAContract( + string.concat(key, " in file ", path) + );script/tasks/diamondUpdateFacet.sh (3)
117-121: Refactor duplicated path construction logic.The script path construction logic is duplicated from lines 88-93. Consider extracting this into a helper function to improve maintainability and reduce duplication.
+ # Add at the beginning of the file + getScriptPath() { + local NETWORK="$1" + local SCRIPT="$2" + if isZkEvmNetwork "$NETWORK"; then + echo "$DEPLOY_SCRIPT_DIRECTORY"zksync/"$SCRIPT".zksync.s.sol + else + echo "$DEPLOY_SCRIPT_DIRECTORY""$SCRIPT".s.sol + fi + } - if isZkEvmNetwork "$NETWORK"; then - UPDATE_SCRIPT=$(echo "$DEPLOY_SCRIPT_DIRECTORY"zksync/"$SCRIPT".zksync.s.sol) - else - UPDATE_SCRIPT=$(echo "$DEPLOY_SCRIPT_DIRECTORY""$SCRIPT".s.sol) - fi + UPDATE_SCRIPT=$(getScriptPath "$NETWORK" "$SCRIPT")
125-131: Refactor duplicated forge script execution logic.The forge script execution commands are duplicated for different network types. Consider extracting the common parameters into variables and creating a helper function.
+ executeForgeScript() { + local NETWORK="$1" + local UPDATE_SCRIPT="$2" + local PRIVATE_KEY="$3" + local COMMON_ARGS="NO_BROADCAST=true NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND PRIVATE_KEY=$PRIVATE_KEY" + + if [[ $NETWORK == "zksync" ]] || isZkEvmNetwork "$NETWORK"; then + FOUNDRY_PROFILE=zksync $COMMON_ARGS ./foundry-zksync/forge script "$UPDATE_SCRIPT" -f $NETWORK -vvvv --json --silent --skip-simulation --legacy --slow --zksync + else + $COMMON_ARGS forge script "$UPDATE_SCRIPT" -f $NETWORK -vvvv --json --silent --skip-simulation --legacy + fi + } - if [[ $NETWORK == "zksync" ]]; then - RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync NO_BROADCAST=true NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND PRIVATE_KEY=$PRIVATE_KEY ./foundry-zksync/forge script "$UPDATE_SCRIPT" -f $NETWORK -vvvv --json --silent --skip-simulation --legacy --slow --zksync) - else - if isZkEvmNetwork "$NETWORK"; then - RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync NO_BROADCAST=true NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND PRIVATE_KEY=$PRIVATE_KEY ./foundry-zksync/forge script "$UPDATE_SCRIPT" -f $NETWORK -vvvv --json --silent --skip-simulation --legacy --slow --zksync) - else - RAW_RETURN_DATA=$(NO_BROADCAST=true NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND PRIVATE_KEY=$PRIVATE_KEY forge script "$UPDATE_SCRIPT" -f $NETWORK -vvvv --json --silent --skip-simulation --legacy) - fi - fi + RAW_RETURN_DATA=$(executeForgeScript "$NETWORK" "$UPDATE_SCRIPT" "$PRIVATE_KEY")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 125-125: This assignment is only seen by the forked process.
(SC2097)
[warning] 125-125: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 128-128: This assignment is only seen by the forked process.
(SC2097)
[warning] 128-128: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 130-130: This assignment is only seen by the forked process.
(SC2097)
[warning] 130-130: This expansion will not see the mentioned assignment.
(SC2098)
162-166: Reuse the forge script execution helper function.The forge script execution logic is duplicated again for the staging environment. The previously suggested helper function can be extended to handle both broadcast and no-broadcast scenarios.
+ executeForgeScript() { + local NETWORK="$1" + local UPDATE_SCRIPT="$2" + local PRIVATE_KEY="$3" + local BROADCAST="$4" + local COMMON_ARGS="NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND PRIVATE_KEY=$PRIVATE_KEY" + + if [[ "$BROADCAST" == "true" ]]; then + COMMON_ARGS="$COMMON_ARGS NO_BROADCAST=false" + else + COMMON_ARGS="$COMMON_ARGS NO_BROADCAST=true" + fi + + if [[ $NETWORK == "zksync" ]] || isZkEvmNetwork "$NETWORK"; then + FOUNDRY_PROFILE=zksync $COMMON_ARGS ./foundry-zksync/forge script "$UPDATE_SCRIPT" -f $NETWORK -vvvv --json --silent --skip-simulation --legacy --slow --zksync ${BROADCAST:+--broadcast} + else + $COMMON_ARGS forge script "$UPDATE_SCRIPT" -f $NETWORK -vvvv --json --silent --skip-simulation --legacy ${BROADCAST:+--broadcast} + fi + } - if isZkEvmNetwork "$NETWORK"; then - RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync ./foundry-zksync/forge script "$SCRIPT_PATH" -f $NETWORK --json --broadcast --skip-simulation --slow --zksync --private-key $(getPrivateKey "$NETWORK" "$ENVIRONMENT")) - else - RAW_RETURN_DATA=$(NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND NO_BROADCAST=false PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") forge script "$SCRIPT_PATH" -f $NETWORK -vvvv --json --silent --broadcast --skip-simulation --legacy) - fi + RAW_RETURN_DATA=$(executeForgeScript "$NETWORK" "$SCRIPT_PATH" "$(getPrivateKey "$NETWORK" "$ENVIRONMENT")" "true")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 163-163: Quote this to prevent word splitting.
(SC2046)
[warning] 165-165: This assignment is only seen by the forked process.
(SC2097)
[warning] 165-165: This expansion will not see the mentioned assignment.
(SC2098)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
script/deploy/zksync/UpdateRelayFacet.zksync.s.sol(1 hunks)script/deploy/zksync/utils/ScriptBase.sol(1 hunks)script/deploy/zksync/utils/UpdateScriptBase.sol(1 hunks)script/helperFunctions.sh(1 hunks)script/tasks/diamondUpdateFacet.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
script/tasks/diamondUpdateFacet.sh
[warning] 125-125: This assignment is only seen by the forked process.
(SC2097)
[warning] 125-125: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 128-128: This assignment is only seen by the forked process.
(SC2097)
[warning] 128-128: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 130-130: This assignment is only seen by the forked process.
(SC2097)
[warning] 130-130: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 163-163: Quote this to prevent word splitting.
(SC2046)
[warning] 165-165: This assignment is only seen by the forked process.
(SC2097)
[warning] 165-165: This expansion will not see the mentioned assignment.
(SC2098)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (6)
script/deploy/zksync/utils/UpdateScriptBase.sol (3)
31-49: Verify environment variable usage.Currently, environment variables are read but not validated. Consider defaulting to safe fallback values or reverting with clear error messages if environment variables are missing or invalid.
63-74: Double-check the no-broadcast flow for completeness.When
noBroadcastis set, the code returns early without performing a diamond cut on-chain. Confirm this path is tested, ensuring it covers all selectors and handles partial cuts consistently.
213-238: Verify presence of global config and addresses for refunds.Ensure that the
rootandpathvariables are updated to reference the correctglobal.jsonfile. Consider adding error handling ifrefundWalletis missing, invalid, or points to a non-contract address, depending on your security requirements.script/deploy/zksync/utils/ScriptBase.sol (1)
20-26: Validate environment-based values.Currently, the constructor retrieves environment variables but does not confirm they’re set correctly (e.g., non-zero private key). Validate these inputs to safeguard against misconfiguration.
script/tasks/diamondUpdateFacet.sh (1)
88-93: LGTM! Clean path handling for ZkSync networks.The conditional logic correctly determines the script path based on the network type, ensuring proper handling of ZkSync-specific paths and extensions.
script/helperFunctions.sh (1)
2800-2812: LGTM! Clean implementation of ZkEVM network detection.The function follows shell scripting best practices:
- Uses a case statement for efficient network matching
- Follows the convention of returning 0 for true and 1 for false
- Has a single, clear responsibility
Test Coverage ReportLine Coverage: 78.66% (2238 / 2845 lines) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
script/tasks/diamondUpdateFacet.sh (1)
100-100: Remove unused variable declaration.The
CONTRACT_NAMEvariable is declared but never used in non-zkEVM networks.- CONTRACT_NAME="" # Not needed for non-zkEVM networks🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 100-100: CONTRACT_NAME appears unused. Verify use (or export if used externally).
(SC2034)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
foundry.toml(1 hunks)script/deploy/deploySingleContract.sh(3 hunks)script/scriptMaster.sh(1 hunks)script/tasks/diamondUpdateFacet.sh(2 hunks)
🧰 Additional context used
📓 Learnings (1)
script/tasks/diamondUpdateFacet.sh (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/deploySingleContract.sh:231-231
Timestamp: 2024-11-26T01:18:52.125Z
Learning: In the `deploySingleContract.sh` script, environment variable assignments preceding the `forge script` command (e.g., `DEPLOYSALT=$DEPLOYSALT ... forge script ...`) are acceptable and function correctly, even when variable expansions like `$DIAMOND_TYPE` and `$NETWORK` are used later in the same command.
🪛 Shellcheck (0.10.0)
script/tasks/diamondUpdateFacet.sh
[warning] 100-100: CONTRACT_NAME appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 134-134: This assignment is only seen by the forked process.
(SC2097)
[warning] 134-134: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 136-136: This assignment is only seen by the forked process.
(SC2097)
[warning] 136-136: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 150-150: Quote this to prevent word splitting.
(SC2046)
[warning] 152-152: This assignment is only seen by the forked process.
(SC2097)
[warning] 152-152: This expansion will not see the mentioned assignment.
(SC2098)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: generate-tag
- GitHub Check: analyze
🔇 Additional comments (4)
foundry.toml (1)
24-25: LGTM! Configuration for ZkSync profile looks good.The cache and output paths are properly configured for ZkSync builds, and the default profile already has the necessary read permissions for the
zkoutdirectory.script/tasks/diamondUpdateFacet.sh (1)
88-101: LGTM! ZkSync path handling is well-structured.The code correctly:
- Checks for ZkSync networks using
isZkEvmNetwork- Sets up appropriate paths and extensions
- Installs required foundry-zksync binaries
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 100-100: CONTRACT_NAME appears unused. Verify use (or export if used externally).
(SC2034)
script/deploy/deploySingleContract.sh (1)
96-97: LGTM! Network check is properly updated.The code correctly uses the new
isZkEvmNetworkfunction for ZkSync network detection.script/scriptMaster.sh (1)
134-140: LGTM! ZkSync handling is properly updated.The code correctly:
- Uses the new
isZkEvmNetworkfunction for network detection- Sets up appropriate script directory
- Installs required foundry-zksync binaries
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
script/tasks/diamondUpdateFacet.sh (2)
127-134: 🛠️ Refactor suggestionFix environment variable handling and private key expansion.
- Environment variables should be exported before the command.
- Private key should be quoted to prevent word splitting.
- Use consistent path variables.
Apply this diff:
PRIVATE_KEY=$(getPrivateKey $NETWORK $ENVIRONMENT) echoDebug "Calculating facet cuts for $SCRIPT..." if isZkEvmNetwork "$NETWORK"; then - RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync NO_BROADCAST=true NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND PRIVATE_KEY=$PRIVATE_KEY ./foundry-zksync/forge script "$UPDATE_SCRIPT" -f $NETWORK -vvvv --json --skip-simulation --slow --zksync --suppress-warnings assemblycreate) + export FOUNDRY_PROFILE=zksync + export NO_BROADCAST=true + export NETWORK=$NETWORK + export FILE_SUFFIX=$FILE_SUFFIX + export USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND + export PRIVATE_KEY="$PRIVATE_KEY" + RAW_RETURN_DATA=$(./foundry-zksync/forge script "$UPDATE_SCRIPT" -f "$NETWORK" -vvvv --json --skip-simulation --slow --zksync --suppress-warnings assemblycreate) + unset FOUNDRY_PROFILE NO_BROADCAST NETWORK FILE_SUFFIX USE_DEF_DIAMOND PRIVATE_KEY else - RAW_RETURN_DATA=$(NO_BROADCAST=true NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND PRIVATE_KEY=$PRIVATE_KEY forge script "$UPDATE_SCRIPT" -f $NETWORK -vvvv --json --skip-simulation --legacy) + export NO_BROADCAST=true + export NETWORK=$NETWORK + export FILE_SUFFIX=$FILE_SUFFIX + export USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND + export PRIVATE_KEY="$PRIVATE_KEY" + RAW_RETURN_DATA=$(forge script "$SCRIPT_PATH" -f "$NETWORK" -vvvv --json --skip-simulation --legacy) + unset NO_BROADCAST NETWORK FILE_SUFFIX USE_DEF_DIAMOND PRIVATE_KEY fi🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 131-131: This assignment is only seen by the forked process.
(SC2097)
[warning] 131-131: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 133-133: This assignment is only seen by the forked process.
(SC2097)
[warning] 133-133: This expansion will not see the mentioned assignment.
(SC2098)
145-150: 🛠️ Refactor suggestionFix staging deployment environment variables and private key handling.
Similar issues as in production deployment need to be addressed:
- Quote private key expansion
- Export environment variables properly
- Use consistent path variables
Apply this diff:
# STAGING: just deploy normally without further checks if isZkEvmNetwork "$NETWORK"; then - RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync ./foundry-zksync/forge script "$UPDATE_SCRIPT" -f $NETWORK --json --broadcast --skip-simulation --slow --zksync --private-key $(getPrivateKey "$NETWORK" "$ENVIRONMENT") --suppress-warnings assemblycreate) + export FOUNDRY_PROFILE=zksync + RAW_RETURN_DATA=$(./foundry-zksync/forge script "$ZK_SCRIPT_PATH" -f "$NETWORK" --json --broadcast --skip-simulation --slow --zksync --private-key "$(getPrivateKey "$NETWORK" "$ENVIRONMENT")" --suppress-warnings assemblycreate) + unset FOUNDRY_PROFILE else - RAW_RETURN_DATA=$(NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND NO_BROADCAST=false PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") forge script "$SCRIPT_PATH" -f $NETWORK -vvvv --json --broadcast --skip-simulation --legacy) + export NETWORK=$NETWORK + export FILE_SUFFIX=$FILE_SUFFIX + export USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND + export NO_BROADCAST=false + export PRIVATE_KEY="$(getPrivateKey "$NETWORK" "$ENVIRONMENT")" + RAW_RETURN_DATA=$(forge script "$SCRIPT_PATH" -f "$NETWORK" -vvvv --json --broadcast --skip-simulation --legacy) + unset NETWORK FILE_SUFFIX USE_DEF_DIAMOND NO_BROADCAST PRIVATE_KEY fi🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 147-147: Quote this to prevent word splitting.
(SC2046)
[warning] 149-149: This assignment is only seen by the forked process.
(SC2097)
[warning] 149-149: This expansion will not see the mentioned assignment.
(SC2098)
🧹 Nitpick comments (3)
.gitignore (1)
1-8: Clean up duplicate .DS_Store entries.The file has multiple entries for
.DS_Storefiles. Let's consolidate them.Apply this diff to clean up the duplicates:
cache/ zkcache/ artifacts/ artifacts-zk/ out/ zkout/ broadcast/ foundry-zksync/*.tar.gz -.DS_Store*script/deploy/zksync/utils/UpdateScriptBase.sol (2)
113-179: Optimize nested loops in buildDiamondCut.The function contains a nested loop for finding selectors to remove, which could be optimized.
Consider using a mapping for O(1) lookup:
function buildDiamondCut( bytes4[] memory newSelectors, address newFacet ) internal { // ... existing code ... // Get selectors to remove bytes4[] memory oldSelectors = loupe.facetFunctionSelectors(oldFacet); + mapping(bytes4 => bool) memory newSelectorMap; + for (uint256 i; i < newSelectors.length; i++) { + newSelectorMap[newSelectors[i]] = true; + } for (uint256 i; i < oldSelectors.length; i++) { - bool found = false; - for (uint256 j; j < newSelectors.length; j++) { - if (oldSelectors[i] == newSelectors[j]) { - found = true; - break; - } - } - if (!found) { + if (!newSelectorMap[oldSelectors[i]]) { selectorsToRemove.push(oldSelectors[i]); } } // ... rest of the code ... }
194-212: Consider using a library for hex conversion.The manual hex conversion functions could be replaced with a well-tested library.
Consider using OpenZeppelin's
Stringslibrary or similar for hex conversion:+import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; + contract UpdateScriptBase is ScriptBase, ScriptExt { using stdJson for string; + using Strings for uint256; - function toHexDigit(uint8 d) internal pure returns (bytes1) { - // ... existing implementation ... - } - function fromCode(bytes4 code) public pure returns (string memory) { - bytes memory result = new bytes(10); - result[0] = bytes1("0"); - result[1] = bytes1("x"); - for (uint256 i = 0; i < 4; ++i) { - result[2 * i + 2] = toHexDigit(uint8(code[i]) / 16); - result[2 * i + 3] = toHexDigit(uint8(code[i]) % 16); - } - return string(result); + return string.concat("0x", Strings.toHexString(uint32(code), 4)); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks).gitmodules(1 hunks)foundry.toml(2 hunks)lib/forge-zksync-std(1 hunks)script/deploy/zksync/UpdateRelayFacet.zksync.s.sol(1 hunks)script/deploy/zksync/utils/UpdateScriptBase.sol(1 hunks)script/deploy/zksync/utils/contract-selectors.sh(1 hunks)script/helperFunctions.sh(3 hunks)script/tasks/diamondUpdateFacet.sh(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/forge-zksync-std
🚧 Files skipped from review as they are similar to previous changes (2)
- foundry.toml
- script/helperFunctions.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
script/tasks/diamondUpdateFacet.sh
[warning] 97-97: CONTRACT_NAME appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 131-131: This assignment is only seen by the forked process.
(SC2097)
[warning] 131-131: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 133-133: This assignment is only seen by the forked process.
(SC2097)
[warning] 133-133: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 147-147: Quote this to prevent word splitting.
(SC2046)
[warning] 149-149: This assignment is only seen by the forked process.
(SC2097)
[warning] 149-149: This expansion will not see the mentioned assignment.
(SC2098)
script/deploy/zksync/utils/contract-selectors.sh
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: analyze
- GitHub Check: generate-tag
🔇 Additional comments (4)
.gitmodules (1)
28-30: Verify the third-party fork's authenticity and maintenance status.The submodule points to a fork by Moonsong-Labs rather than the official ZkSync repository. This requires verification to ensure security and maintenance standards.
Let's check the repository's status and the specified commit:
Consider:
- Is this the most appropriate fork to use?
- Should we switch to an official ZkSync repository if available?
- Should we pin to a specific commit or tag for stability?
script/deploy/zksync/UpdateRelayFacet.zksync.s.sol (1)
1-14: LGTM! Verify ZkSync compatibility.The implementation is clean and focused. The
vmExt.zkVm(true)call correctly enables ZkSync mode before updating the RelayFacet.Run this script to verify ZkSync compatibility:
✅ Verification successful
✅ ZkSync compatibility verified
The deployment setup is properly configured for ZkSync with:
- Correct forge-zksync-std integration
- Proper ZkSync configuration in foundry.toml
- Appropriate VM mode activation in the deployment script
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all required ZkSync dependencies and configurations are in place # Check if forge-zksync-std is imported in other files rg -l "forge-zksync-std" # Check for other ZkSync-specific configurations if [ -f "foundry.toml" ]; then echo "Checking foundry.toml for ZkSync configs..." rg "zksync|zk" foundry.toml fiLength of output: 767
Script:
#!/bin/bash # Check the base script implementation cat script/deploy/zksync/utils/UpdateScriptBase.solLength of output: 6814
script/deploy/zksync/utils/contract-selectors.sh (1)
1-11:⚠️ Potential issueFix word splitting and verify path assumptions.
The script has two issues that need attention:
- The command substitution in the read command should be quoted to prevent word splitting
- The script assumes a specific output directory structure that should be verified
Fix the word splitting issue:
-read -a EXCLUDE <<< $(sed 's/0x//g' <<< "$2") +read -a EXCLUDE <<< "$(sed 's/0x//g' <<< "$2")"Verify the path assumptions:
✅ Verification successful
Fix word splitting in the command substitution
The path assumption for
./out/zksyncis correct as it refers to the standard Forge/Foundry build output directory. While this directory doesn't exist in the repository, it's expected as these are build artifacts generated during compilation.Fix needed:
- Quote the command substitution to prevent word splitting:
-read -a EXCLUDE <<< $(sed 's/0x//g' <<< "$2") +read -a EXCLUDE <<< "$(sed 's/0x//g' <<< "$2")"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the required directory structure exists # Check if the zksync output directory exists if [ ! -d "./out/zksync" ]; then echo "Error: ./out/zksync directory not found" exit 1 fi # Check if any contract JSON files exist in the directory fd -e json . ./out/zksyncLength of output: 127
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
script/deploy/zksync/utils/UpdateScriptBase.sol (1)
97-111: Improve FFI security in getSelectors.The function uses FFI to execute a shell script, which could be a security risk if not properly validated.
Run this script to verify the FFI call security:
✅ Verification successful
FFI implementation in getSelectors is properly secured
The script implementation follows security best practices:
- Proper input sanitization and quoting
- Constrained file access to build artifacts
- Uses trusted tools (jq, cast) for processing
- Type-safe inputs through Solidity's type system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the contract-selectors.sh script exists and has proper permissions SCRIPT_PATH="script/deploy/zksync/utils/contract-selectors.sh" # Check if script exists if [ ! -f "$SCRIPT_PATH" ]; then echo "Error: $SCRIPT_PATH not found" exit 1 fi # Check script permissions ls -l "$SCRIPT_PATH" # Check script contents for potential security issues rg "(rm|mv|cp|chmod|chown|sudo|eval)" "$SCRIPT_PATH"Length of output: 371
Script:
#!/bin/bash # Examine the complete contents of the script cat script/deploy/zksync/utils/contract-selectors.sh # Check for potential command substitution and injection points rg -n "[\`\$\(\)]|[^\\]\"" script/deploy/zksync/utils/contract-selectors.shLength of output: 637
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
script/tasks/diamondUpdateFacet.sh (1)
147-151: 🛠️ Refactor suggestion
⚠️ Potential issueStaging Branch: Quote Private Key in Command Substitution
In the staging branch (lines 147–151), when executing the deploy script for ZkSync networks, the private key is inserted via command substitution without quotes. This can lead to word splitting issues if the private key contains special characters. Please wrap the substitution in quotes.Suggested Diff:
- RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync ./foundry-zksync/forge script "$UPDATE_SCRIPT" -f $NETWORK --json --broadcast --skip-simulation --slow --zksync --private-key $(getPrivateKey "$NETWORK" "$ENVIRONMENT") --suppress-warnings assemblycreate) + RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync ./foundry-zksync/forge script "$UPDATE_SCRIPT" -f $NETWORK --json --broadcast --skip-simulation --slow --zksync --private-key "$(getPrivateKey "$NETWORK" "$ENVIRONMENT")" --suppress-warnings assemblycreate)A similar quoting consideration may also be applied to the else branch in this block if not already handled.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 147-147: Quote this to prevent word splitting.
(SC2046)
[warning] 149-149: This assignment is only seen by the forked process.
(SC2097)
[warning] 149-149: This expansion will not see the mentioned assignment.
(SC2098)
🧹 Nitpick comments (1)
script/tasks/diamondUpdateFacet.sh (1)
117-136: Production Branch: Secure Deployment Command and Quoting
Within the production branch block, the logic first differentiates between ZkSync and non-ZkSync networks by settingUPDATE_SCRIPTappropriately. A few improvements are recommended:
- Private Key Quoting: In the assignment
the function arguments should be quoted to avoid potential word splitting—especially if a private key contains spaces or special characters.PRIVATE_KEY=$(getPrivateKey $NETWORK $ENVIRONMENT)- Note on Inline Environment Exports: Although inline variable assignments (e.g. for
NO_BROADCAST,NETWORK, etc.) are common, be aware that shellcheck warns these assignments are scoped only to the forked process. In this context, that behavior is acceptable, but a brief verification that it is intended would be useful.Suggested Diff:
- PRIVATE_KEY=$(getPrivateKey $NETWORK $ENVIRONMENT) + PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 131-131: This assignment is only seen by the forked process.
(SC2097)
[warning] 131-131: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 133-133: This assignment is only seen by the forked process.
(SC2097)
[warning] 133-133: This expansion will not see the mentioned assignment.
(SC2098)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
foundry.toml(1 hunks)script/deploy/deploySingleContract.sh(4 hunks)script/helperFunctions.sh(3 hunks)script/scriptMaster.sh(2 hunks)script/tasks/diamondUpdateFacet.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- foundry.toml
- script/deploy/deploySingleContract.sh
- script/helperFunctions.sh
- script/scriptMaster.sh
🧰 Additional context used
🧠 Learnings (1)
script/tasks/diamondUpdateFacet.sh (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/tasks/diamondUpdateFacet.sh
[warning] 97-97: CONTRACT_NAME appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 131-131: This assignment is only seen by the forked process.
(SC2097)
[warning] 131-131: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 133-133: This assignment is only seen by the forked process.
(SC2097)
[warning] 133-133: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 147-147: Quote this to prevent word splitting.
(SC2046)
[warning] 149-149: This assignment is only seen by the forked process.
(SC2097)
[warning] 149-149: This expansion will not see the mentioned assignment.
(SC2098)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
script/helperFunctions.sh (1)
2848-2860: New Helper Function:isZkEvmNetworkThe implementation correctly checks if the provided network name is either
"zksync"or"abstract", returning 0 when matched and 1 otherwise. This centralizes the logic for detecting zkEVM networks and helps reduce duplication across scripts.Suggestions:
- Case Sensitivity: Consider normalizing the input (for example, using
tr '[:upper:]' '[:lower:]') so that network names are compared in a case-insensitive fashion. This would help avoid issues if the network name is provided with different letter cases.- Documentation: It may be helpful to add a short inline comment explaining the expected use of this function and its return values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
script/deploy/deploySingleContract.sh(4 hunks)script/helperFunctions.sh(3 hunks)script/scriptMaster.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- script/scriptMaster.sh
- script/deploy/deploySingleContract.sh
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: generate-tag
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
script/deploy/zksync/utils/UpdateScriptBase.sol (2)
105-108: Avoid repeated string concatenations in a loop.Looping string concatenations in Solidity can be costly. Consider building the entire string once or using intermediate buffers to reduce overhead.
-for (uint256 i; i < _exclude.length; i++) { - exclude = string.concat(exclude, fromCode(_exclude[i]), " "); -} +{ + bytes memory buffer; + for (uint256 i = 0; i < _exclude.length; i++) { + buffer = abi.encodePacked(buffer, fromCode(_exclude[i]), " "); + } + exclude = string(buffer); +}
1-1: Add inline documentation or natspec comments for clarity.Consider adding brief explanations for key functions (e.g.,
update,buildDiamondCut) to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
script/deploy/zksync/UpdateRelayFacet.zksync.s.sol(1 hunks)script/deploy/zksync/utils/UpdateScriptBase.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/deploy/zksync/UpdateRelayFacet.zksync.s.sol
🧰 Additional context used
🧠 Learnings (1)
script/deploy/zksync/utils/UpdateScriptBase.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 (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: generate-tag
🔇 Additional comments (2)
script/deploy/zksync/utils/UpdateScriptBase.sol (2)
32-50: Enhance error handling in the constructor.This is the same recommendation as before to validate the JSON file contents and the diamond address to prevent silent failures.
117-147: SingleoldFacetusage may skip some selectors if multiple old facets exist.Only the last replaced facet is examined for removed selectors. Prior suggestions to handle multiple old facets were declined, so this is noted merely for clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
script/helperFunctions.sh (1)
2848-2860: Refactor: Separate variable declaration from assignment to preserve return codes.In the new
isZkEvmNetworkfunction, the linelocal IS_ZK_EVM=$(jq -r --arg network "$NETWORK" '.[$network].isZkEVM // false' "$NETWORKS_JSON_FILE_PATH")combines declaration and assignment. This can mask errors (as highlighted by SC2155). Splitting the declaration and assignment ensures that any nonzero exit codes from
jqaren’t inadvertently hidden.Suggested Diff:
- local IS_ZK_EVM=$(jq -r --arg network "$NETWORK" '.[$network].isZkEVM // false' "$NETWORKS_JSON_FILE_PATH") + local IS_ZK_EVM + IS_ZK_EVM=$(jq -r --arg network "$NETWORK" '.[$network].isZkEVM // false' "$NETWORKS_JSON_FILE_PATH")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 2853-2853: Declare and assign separately to avoid masking return values.
(SC2155)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/networks.json(43 hunks)foundry.toml(2 hunks)script/helperFunctions.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- foundry.toml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
script/helperFunctions.sh
[warning] 2853-2853: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
🔇 Additional comments (1)
config/networks.json (1)
19-19: Consistent Addition of "isZkEVM" Flag in Network ConfigurationsThe configuration file has been updated to include the new boolean key
"isZkEVM"in each network definition. The flag is set as follows:
- For networks like mainnet, apechain, arbitrum, and most others,
"isZkEVM": falsecorrectly indicates that these networks do not support zkEVM.- For the abstract network (line 39) and zksync (line 839), the flag is set to
true, which appears to reflect their intended support for zkEVM.This uniform addition greatly improves clarity and simplifies the logic in deployment and update scripts (e.g., via the new
isZkEvmNetworkhelper) by allowing explicit checks of zkEVM support. Please verify that these boolean values truly match the capabilities of each network per your latest specifications.Also applies to: 39-39, 59-59, 79-79, 99-99, 119-119, 139-139, 159-159, 179-179, 199-199, 219-219, 239-239, 259-259, 279-279, 299-299, 319-319, 339-339, 359-359, 379-379, 399-399, 419-419, 439-439, 459-459, 479-479, 499-499, 519-519, 539-539, 559-559, 579-579, 599-599, 619-619, 639-639, 659-659, 679-679, 699-699, 719-719, 739-739, 759-759, 779-779, 799-799, 819-819, 839-839, 859-859
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
script/helperFunctions.sh (1)
2848-2872: NewisZkEvmNetworkfunction implementationThe new function correctly checks that the provided network exists in the JSON configuration and verifies the presence and boolean value of the
isZkEVMproperty. The logic is clear and returns 0 when the network supports zkEVM and 1 otherwise.Minor Suggestion (ShellCheck SC2155):
On line 2865, the inline declaration and assignment of the variableIS_ZK_EVMmay mask the return value of thejqcommand. For improved error detection and clarity, consider declaring the variable separately and then assigning it, for example:- local IS_ZK_EVM=$(jq -r --arg network "$NETWORK" '.[$network].isZkEVM' "$NETWORKS_JSON_FILE_PATH") + local IS_ZK_EVM + IS_ZK_EVM=$(jq -r --arg network "$NETWORK" '.[$network].isZkEVM' "$NETWORKS_JSON_FILE_PATH")This change adheres to best practices and helps ensure that any jq errors aren’t unintentionally suppressed.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 2865-2865: Declare and assign separately to avoid masking return values.
(SC2155)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/helperFunctions.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
script/helperFunctions.sh
[warning] 2865-2865: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
script/tasks/diamondUpdateFacet.sh (1)
107-109:⚠️ Potential issueAlign File Existence Check with zkEVM Path Logic and Correct Variable Name
At this block, the script buildsFULL_SCRIPT_PATHunconditionally from$DEPLOY_SCRIPT_DIRECTORYand$SCRIPT.s.sol, which ignores the zkEVM-specific path already computed in lines 88–98. Moreover, the error message references an undefined variable ($CONTRACT) instead of the extracted contract name ($CONTRACT_NAME).
To fix, consider a diff such as:- local FULL_SCRIPT_PATH=""$DEPLOY_SCRIPT_DIRECTORY""$SCRIPT"".s.sol"" - if ! checkIfFileExists "$FULL_SCRIPT_PATH" >/dev/null; then - error "could not find update script for $CONTRACT in this path: $FULL_SCRIPT_PATH". Aborting update. + local FULL_SCRIPT_PATH="" + if isZkEvmNetwork "$NETWORK"; then + FULL_SCRIPT_PATH="$SCRIPT_PATH" + else + FULL_SCRIPT_PATH="$DEPLOY_SCRIPT_DIRECTORY$SCRIPT.s.sol" + fi + if ! checkIfFileExists "$FULL_SCRIPT_PATH" >/dev/null; then + error "could not find update script for $CONTRACT_NAME in this path: $FULL_SCRIPT_PATH". Aborting update.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 107-107: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 107-107: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 107-107: Word is of the form "A"B"C" (B indicated). Did you mean "ABC" or "A"B"C"?
(SC2140)
🧹 Nitpick comments (1)
script/tasks/diamondUpdateFacet.sh (1)
88-98: Consistent Script Path Setup for zkEVM Networks
The new conditional block correctly sets a zkEVM-specific script path (usingSCRIPT_PATH) and extracts the contract name intoCONTRACT_NAME. However, please ensure that all downstream references (especially the file existence check) consistently use this variable so that the intended zkEVM path isn’t inadvertently overridden.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 97-97: CONTRACT_NAME appears unused. Verify use (or export if used externally).
(SC2034)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/tasks/diamondUpdateFacet.sh(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
script/tasks/diamondUpdateFacet.sh (2)
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/deploySingleContract.sh:231-231
Timestamp: 2025-03-12T15:36:55.565Z
Learning: In the `deploySingleContract.sh` script, environment variable assignments preceding the `forge script` command (e.g., `DEPLOYSALT=$DEPLOYSALT ... forge script ...`) are acceptable and function correctly, even when variable expansions like `$DIAMOND_TYPE` and `$NETWORK` are used later in the same command.
🪛 Shellcheck (0.10.0)
script/tasks/diamondUpdateFacet.sh
[warning] 97-97: CONTRACT_NAME appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 126-126: This assignment is only seen by the forked process.
(SC2097)
[warning] 126-126: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 128-128: This assignment is only seen by the forked process.
(SC2097)
[warning] 128-128: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 142-142: Quote this to prevent word splitting.
(SC2046)
[warning] 144-144: This assignment is only seen by the forked process.
(SC2097)
[warning] 144-144: This expansion will not see the mentioned assignment.
(SC2098)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
🔇 Additional comments (1)
script/tasks/diamondUpdateFacet.sh (1)
122-122: Quote Private Key Expansion to Prevent Word Splitting
When invokinggetPrivateKey, the command substitution’s output isn’t wrapped in quotes. If the private key contains spaces or special characters, it could lead to word splitting and potential command injection issues. Please update these usages so that the result is properly quoted. For example:- PRIVATE_KEY=$(getPrivateKey $NETWORK $ENVIRONMENT) + PRIVATE_KEY="$(getPrivateKey "$NETWORK" "$ENVIRONMENT")"And for inline usage in command arguments:
- --private-key $(getPrivateKey "$NETWORK" "$ENVIRONMENT") + --private-key "$(getPrivateKey "$NETWORK" "$ENVIRONMENT")"This applies to both the production branch (line 122) and the staging branch (lines 142 and 144).
Also applies to: 142-142, 144-144
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
script/helperFunctions.sh (1)
2865-2889: ** Improve variable declaration to avoid masking return values.**
The newly addedisZkEvmNetworkfunction is logically sound and clearly documents the process of verifying whether the provided network is a zkEVM network. One minor improvement is suggested: on line 2882, instead of combining declaration and assignment withlocal IS_ZK_EVM=$(jq -r --arg network "$NETWORK" '.[$network].isZkEVM' "$NETWORKS_JSON_FILE_PATH")split the declaration and assignment. This avoids inadvertently masking the return value from the command, which is flagged by shellcheck SC2155. For example, you can refactor it as follows:
- local IS_ZK_EVM=$(jq -r --arg network "$NETWORK" '.[$network].isZkEVM' "$NETWORKS_JSON_FILE_PATH") + local IS_ZK_EVM + IS_ZK_EVM=$(jq -r --arg network "$NETWORK" '.[$network].isZkEVM' "$NETWORKS_JSON_FILE_PATH")This small change adheres to best practices and enhances code robustness. Overall, the function correctly checks for the existence of the network and its
isZkEVMproperty and returns an appropriate exit status.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 2882-2882: Declare and assign separately to avoid masking return values.
(SC2155)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
script/helperFunctions.sh(3 hunks)script/scriptMaster.sh(2 hunks)test/solidity/utils/TestBase.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/scriptMaster.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
script/helperFunctions.sh
[warning] 2882-2882: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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 (1)
test/solidity/utils/TestBase.sol (1)
200-201:✅ Verification successful
Diamond owner address update verified.
The USER_DIAMOND_OWNER constant has been updated to a new address. This address is used to create the diamond (line 308) and deploy the fee collector (line 311) in the test environment, so this change aligns with the PR's objective to update ZkEVM scripts.
🏁 Script executed:
#!/bin/bash # Description: Search for references to the previous diamond owner address in the codebase to ensure all references have been updated. # Search for any remaining references to the old diamond owner address echo "Searching for any references to the old diamond owner address..." rg --no-ignore "0x5042255A3F3FD7727e419CeA387cAFDfad3C3aF8" --type-add 'solidity:*.{sol,js,ts}' # Check if the new address is used consistently in the updated scripts echo "Checking usage of new diamond owner address in the codebase..." rg --no-ignore "0x8b60A21A0e2852cCC944971486868beBB9f32287" --type-add 'solidity:*.{sol,js,ts}'Length of output: 550
Diamond Owner Address Update Verified
The new diamond owner address is used as expected intest/solidity/utils/TestBase.sol, and our search confirms no lingering references to the old address exist in the codebase. All changes align with the PR's objective to update the ZkEVM scripts.
- Confirmed new address (
0x8b60A21A0e2852cCC944971486868beBB9f32287) appears only where expected.- No references to the old address were found.
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)