Integrate CREATE3Factory into contracts repo (and deploy all flow)#1113
Integrate CREATE3Factory into contracts repo (and deploy all flow)#1113
Conversation
## Walkthrough
A new deployment step for the CREATE3Factory smart contract was integrated into the deployment workflow. This involved introducing a dedicated Bash function and a Forge Solidity script for deploying the CREATE3Factory contract, updating the deployment sequence in the main deployment script to call this new step early in the process. The new function ensures the contract is deployed, its address is stored, and the deployment is validated before proceeding with the rest of the contracts. No changes were made to exported or public entities outside of the new deployment logic.
## Changes
| Files/Paths | Change Summary |
|-----------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------|
| script/deploy/deployAllContracts.sh | Updated deployment sequence to call the new CREATE3Factory deployment function early; reordered script sourcing. |
| script/deploy/deployAndStoreCREATE3Factory.sh | Added new Bash function to deploy CREATE3Factory, handle errors, and update address in networks.json. |
| script/deploy/facets/DeployCREATE3Factory.s.sol | Added new Forge deployment script contract for CREATE3Factory, with public run() function. |
## Suggested reviewers
- ezynda3Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /// @notice Enables deploying contracts using CREATE3. Each deployer (msg.sender) has | ||
| /// its own namespace for deployed addresses. | ||
| /// @custom:version 1.0.0 | ||
| contract CREATE3Factory is ICREATE3Factory { |
Check failure
Code scanning / Olympix Integrated Security
Contracts that can receive ether but cannot send it may lock value permanently. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/locked-ether
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
script/deploy/deployAllContracts.sh (1)
43-45: Verify if this deployment strategy is intentional.The script now exclusively deploys the CREATE3Factory while commenting out all other deployment steps. Is this intended to be a temporary state for testing, or is this the first step in transitioning to a new deployment model using CREATE3?
Consider documenting the new deployment strategy that uses CREATE3Factory, explaining how it replaces or modifies the previous workflow. This will help future developers understand the transition and implementation approach.
src/Helpers/CREATE3Factory.sol (1)
13-22: Add a withdrawal mechanism to handle locked ETH.The contract can receive ETH through the payable
deployfunction, but has no way to retrieve it if deployment fails or if ETH is sent directly to the contract. This could lead to permanently locked funds.Consider adding a withdrawal function accessible only by an owner or admin:
contract CREATE3Factory is ICREATE3Factory { + address public owner; + + constructor() { + owner = msg.sender; + } + /// @inheritdoc ICREATE3Factory function deploy( bytes32 salt, bytes memory creationCode ) external payable override returns (address deployed) { // hash salt with the deployer address to give each deployer its own namespace salt = keccak256(abi.encodePacked(msg.sender, salt)); return CREATE3.deploy(salt, creationCode, msg.value); } + + /// @notice Allows the owner to withdraw any ETH stuck in the contract + /// @param to The address to send the ETH to + /// @param amount The amount of ETH to withdraw + function withdrawETH(address to, uint256 amount) external { + require(msg.sender == owner, "Only owner"); + require(to != address(0), "Invalid recipient"); + (bool success, ) = to.call{value: amount}(""); + require(success, "Transfer failed"); + }🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[failure] 13-13:
Contracts that can receive ether but cannot send it may lock value permanently. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/locked-etherscript/deploy/deployAndStoreCREATE3Factory.sh (5)
1-2: Use env-based shebang for portability.
Replace the hardcoded#!/bin/bashwith:#!/usr/bin/env bashso that the script picks up the user’s environment
bashand works consistently across different systems.
20-21: Verify.envexists before sourcing.
Blindly sourcing.envcan silently fail or introduce unexpected variables. Safeguard with:if [ ! -f .env ]; then error ".env file not found" return 1 fi source .env
40-43: Unify error reporting viaerrorhelper.
Instead of:if [[ $RETURN_CODE -ne 0 ]]; then echo "❌ Error: deployment was not successful" return 1 fiuse:
if [[ $RETURN_CODE -ne 0 ]]; then error "Deployment failed on network '$NETWORK' ($ENVIRONMENT)" return 1 fiThis ensures consistent, colorized error output.
55-58: EnsureNETWORKS_JSON_FILE_PATHis set.
Before running:jq -e --arg net "$NETWORK" '.[$net]' "$NETWORKS_JSON_FILE_PATH"verify that
NETWORKS_JSON_FILE_PATHwas loaded fromconfig.sh, e.g.:if [ -z "$NETWORKS_JSON_FILE_PATH" ]; then error "NETWORKS_JSON_FILE_PATH is not defined" return 1 fiThis yields clearer errors if the config is misconfigured.
61-67: Harden temp-file creation when updating JSON.
You’re usingmktempandjqatomically, which is great. Catchmktemperrors explicitly:local tmpfile tmpfile=$(mktemp) || { error "mktemp failed"; return 1; } if ! jq --arg net "$NETWORK" --arg addr "$FACTORY_ADDRESS" \ '(.[$net].create3Factory) = $addr' "$NETWORKS_JSON_FILE_PATH" > "$tmpfile"; then error "Failed to update networks.json" rm -f "$tmpfile" return 1 fiThis ensures any temp-file issues are surfaced immediately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
script/deploy/deployAllContracts.sh(2 hunks)script/deploy/deployAndStoreCREATE3Factory.sh(1 hunks)script/deploy/facets/DeployCREATE3Factory.s.sol(1 hunks)src/Helpers/CREATE3Factory.sol(1 hunks)src/Interfaces/ICREATE3Factory.sol(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
script/deploy/deployAllContracts.sh (2)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
deployAndStoreCREATE3Factory(5-75)script/helperFunctions.sh (1)
checkFailure(2226-2236)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
script/helperFunctions.sh (3)
getFileSuffix(2064-2074)echoDebug(2239-2247)error(2248-2250)
🪛 Shellcheck (0.10.0)
script/deploy/deployAndStoreCREATE3Factory.sh
[warning] 23-23: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 GitHub Check: Olympix Integrated Security
src/Helpers/CREATE3Factory.sol
[failure] 13-13:
Contracts that can receive ether but cannot send it may lock value permanently. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/locked-ether
🪛 GitHub Actions: Types Bindings
src/Helpers/CREATE3Factory.sol
[error] 6-6: Compiler error: Source "src/interfaces/ICREATE3Factory.sol" not found. Import statement at line 6 cannot be resolved.
🪛 GitHub Actions: Enforce Min Test Coverage
src/Helpers/CREATE3Factory.sol
[error] 6-6: Compiler error: Source file 'src/interfaces/ICREATE3Factory.sol' not found. Import statement at line 6 cannot be resolved.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (10)
src/Interfaces/ICREATE3Factory.sol (1)
1-28: Well-designed interface for deterministic contract deployment.The interface is clearly documented with comprehensive NatSpec comments and provides a clean abstraction for the CREATE3 deployment pattern. The salt mechanism ensures unique namespaces per deployer, preventing address collisions.
script/deploy/facets/DeployCREATE3Factory.s.sol (2)
8-17: LGTM! Script follows standard Forge deployment pattern.The deployment script correctly uses the Script contract, retrieves a private key from the environment, and handles the broadcast session appropriately.
6-6:Details
✅ Verification successful
Verify the import path remapping configuration.
The import uses the "lifi/" prefix which suggests a remapping in Forge's configuration. Ensure this remapping is correctly set up in your foundry.toml or other configuration files to avoid import errors.
🏁 Script executed:
#!/bin/bash # Check for remapping configuration in foundry.toml if [ -f foundry.toml ]; then echo "Checking remappings in foundry.toml..." grep -i "remappings\|lifi" foundry.toml else echo "foundry.toml not found in the repository root" fiLength of output: 154
🏁 Script executed:
#!/bin/bash # Check for remappings.txt if [ -f remappings.txt ]; then echo "Found remappings.txt:" cat remappings.txt else echo "remappings.txt not found" fi # Show any remappings in foundry.toml echo "Remappings in foundry.toml (if any):" grep -n "remappings" -C2 foundry.toml || echo "No remappings entries found" # Search for CREATE3Factory.sol in the repo echo "Looking for CREATE3Factory.sol files:" grep -R "CREATE3Factory.sol" -n .Length of output: 1496
Import path remapping is correctly configured
– Your remappings.txt includes
lifi/=src/
soimport { CREATE3Factory } from "lifi/Helpers/CREATE3Factory.sol";resolves tosrc/Helpers/CREATE3Factory.sol.
No changes needed.script/deploy/deployAllContracts.sh (2)
9-9: LGTM! New deployment script is correctly sourced.The script properly includes the new CREATE3Factory deployment script.
48-134: Future implementation plan needed for commented-out deployment steps.All standard deployment steps (core facets, diamond, periphery, etc.) have been commented out. Is there a plan to reimplement these using the CREATE3Factory in a future PR? Consider adding a TODO comment describing the implementation plan and related tickets.
src/Helpers/CREATE3Factory.sol (1)
24-32: LGTM! Function correctly implements the interface.The
getDeployedfunction properly hashes the salt with the deployer address and calls the CREATE3 utility to predict the deployed address.script/deploy/deployAndStoreCREATE3Factory.sh (4)
26-31: Debug logging is clear.
UsingechoDebugunder aDEBUGflag provides controlled verbosity and context for troubleshooting.
49-52: Validate extracted factory address.
The emptiness check correctly catches missing addresses:if [[ -z "$FACTORY_ADDRESS" ]]; then error "Failed to obtain deployed contract address for CREATE3Factory on $NETWORK ($ENVIRONMENT)" return 1 fiThis prevents storing invalid data downstream.
69-75: Final logging and exit look solid.
The use of an atomicmv, confirmation log, andreturn 0properly signals success.
23-23: 🛠️ Refactor suggestionAvoid masking return values when declaring locals.
ShellCheck SC2155 flags the combined declaration and assignment:local FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT")Split it to preserve
$?correctly:local FILE_SUFFIX FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT")⛔ Skipped due to learnings
Learnt from: 0xDEnYO PR: lifinance/contracts#715 File: script/tasks/diamondEMERGENCYPause.sh:25-25 Timestamp: 2024-10-08T04:55:05.706Z Learning: When reviewing shell scripts, do not report issues about masking return values by declaring and assigning variables in the same line (e.g., `local FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT")`), as this practice is acceptable to the team.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 23-23: Declare and assign separately to avoid masking return values.
(SC2155)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
script/deploy/deployAndStoreCREATE3Factory.sh (3)
11-12: Make sourcing robust to working-directory changes.Using relative paths for
sourcewill break if the CWD isn’t the repo root. Anchor imports to the script’s directory.SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" source "$SCRIPT_DIR/../config.sh" source "$SCRIPT_DIR/../helperFunctions.sh"This change ensures
config.shandhelperFunctions.share always found.
15-17: Validate input parameters before use.Currently, missing arguments will lead to empty variables and obscure errors down the line. Add a guard to verify both
NETWORKandENVIRONMENTare provided.if [[ $# -lt 2 ]]; then error "Usage: deployAndStoreCREATE3Factory <NETWORK> <ENVIRONMENT>" return 1 fiInsert this at the top of the function (after the
local NETWORK ENVIRONMENTlines).
46-52: Check for empty address before printing success.Echoing success prior to validating the address can mislead users. Move the success message below the empty‐check.
- FACTORY_ADDRESS=$(echo "$RETURN_DATA" | jq -r '.factory.value') - echo "✅ Successfully deployed to address $FACTORY_ADDRESS" - if [[ -z "$FACTORY_ADDRESS" ]]; then - error "Failed to obtain deployed contract address for CREATE3Factory on $NETWORK ($ENVIRONMENT)" - return 1 - fi + FACTORY_ADDRESS=$(echo "$RETURN_DATA" | jq -r '.factory.value') + if [[ -z "$FACTORY_ADDRESS" ]]; then + error "Failed to obtain deployed contract address for CREATE3Factory on $NETWORK ($ENVIRONMENT)" + return 1 + fi + echo "✅ Successfully deployed to address $FACTORY_ADDRESS"
🧹 Nitpick comments (5)
script/deploy/deployAndStoreCREATE3Factory.sh (5)
1-2: Add strict mode flags for safer script execution.Include
set -euo pipefailimmediately after the shebang to ensure the script exits on any error, treats unset variables as errors, and fails on pipeline errors. This guards against silent failures and unexpected behavior.1#!/bin/bash +2 set -euo pipefailPlace this directly below
#!/bin/bash.
22-22: Avoid masking return values when declaring and assigning variables.ShellCheck SC2155 flags combining
localwith assignment. Split declaration and assignment to preserve exit codes.- local FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT") + local FILE_SUFFIX + FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 22-22: Declare and assign separately to avoid masking return values.
(SC2155)
33-38: Scope deployment variables locally and quote expansions.Bundle all temp variables under
local, and quote parameters and paths to handle spaces or special characters reliably.- PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") - RAW_RETURN_DATA=$(PRIVATE_KEY=$PRIVATE_KEY forge script script/deploy/facets/DeployCREATE3Factory.s.sol -f $NETWORK -vvvv --verify --json --legacy --broadcast --skip-simulation --gas-limit 2000000) - RETURN_CODE=$? - CLEAN_RETURN_DATA=$(echo $RAW_RETURN_DATA | sed 's/^.*{\"logs/{\"logs/') - RETURN_DATA=$(echo $CLEAN_RETURN_DATA | jq -r '.returns' 2>/dev/null) + local PRIVATE_KEY RAW_RETURN_DATA RETURN_CODE CLEAN_RETURN_DATA RETURN_DATA + PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") + RAW_RETURN_DATA=$(PRIVATE_KEY="$PRIVATE_KEY" \ + forge script "script/deploy/facets/DeployCREATE3Factory.s.sol" \ + -f "$NETWORK" -vvvv \ + --verify --json --legacy --broadcast --skip-simulation \ + --gas-limit 2000000) + RETURN_CODE=$? + CLEAN_RETURN_DATA=$(echo "$RAW_RETURN_DATA" | sed 's/^.*\({\"logs\)/\1/') + RETURN_DATA=$(echo "$CLEAN_RETURN_DATA" | jq -r '.returns' 2>/dev/null)This change localizes your variables and hardens the invocation.
39-42: Use consistent error logging for deployment failures.Replace the plain
echowith theerrorhelper to standardize formatting and color.- if [[ $RETURN_CODE -ne 0 ]]; then - echo "❌ Error: deployment was not successful" - return 1 - fi + if [[ $RETURN_CODE -ne 0 ]]; then + error "❌ Deployment failed for CREATE3Factory on $NETWORK ($ENVIRONMENT)" + return 1 + fi
54-70: Leveragejq -i(when available) or handle tempfile atomically.You update
networks.jsonvia a tmpfile+mv, which is safe. As an alternative, considerjq -ion Linux for in-place edits, or add a backup step beforemvto aid rollbacks:cp "$NETWORKS_JSON_FILE_PATH" "${NETWORKS_JSON_FILE_PATH}.bak" jq --arg net "$NETWORK" --arg addr "$FACTORY_ADDRESS" \ '(.[$net].create3Factory) = $addr' "$NETWORKS_JSON_FILE_PATH" > "$tmpfile" \ && mv "$tmpfile" "$NETWORKS_JSON_FILE_PATH" \ || { error "Failed to update networks.json"; rm -f "$tmpfile"; return 1; }This ensures you have a backup and a single atomic move.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
script/deploy/deployAllContracts.sh(2 hunks)script/deploy/deployAndStoreCREATE3Factory.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- script/deploy/deployAllContracts.sh
🧰 Additional context used
🧬 Code Graph Analysis (1)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
script/helperFunctions.sh (3)
getFileSuffix(2064-2074)echoDebug(2239-2247)error(2248-2250)
🪛 Shellcheck (0.10.0)
script/deploy/deployAndStoreCREATE3Factory.sh
[warning] 22-22: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
52-56: Prevent misleading logs on missing address
The script currently echoes a success message before verifyingFACTORY_ADDRESS, which can lead to false positives if parsing fails. Reorder the check:- echo "✅ Successfully deployed to address $FACTORY_ADDRESS" - if [[ -z "$FACTORY_ADDRESS" ]]; then - error "Failed to obtain deployed contract address for CREATE3Factory on $NETWORK ($ENVIRONMENT)" - return 1 - fi + if [[ -z "$FACTORY_ADDRESS" ]]; then + error "Failed to obtain deployed contract address for CREATE3Factory on $NETWORK ($ENVIRONMENT)" + return 1 + fi + echo "✅ Successfully deployed to address $FACTORY_ADDRESS"
🧹 Nitpick comments (3)
script/deploy/deployAndStoreCREATE3Factory.sh (3)
28-28: Avoid masking return values (SC2155)
Usinglocal FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT")suppresses the exit code of the function. Split declaration and assignment to preserve error detection:-local FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT") +local FILE_SUFFIX +FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
39-39: Avoid masking return values (SC2155)
Similarly, separate declaration from assignment forPRIVATE_KEYto avoid masking its exit status:-local PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") +local PRIVATE_KEY +PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 39-39: Declare and assign separately to avoid masking return values.
(SC2155)
38-44: Improve robustness and consistency of the deployment command
- Quote variable expansions (e.g.,
"$NETWORK") to guard against spaces or special characters.- Use consistent space-based indentation instead of mixed tabs.
Proposed diff:
- RAW_RETURN_DATA=$(PRIVATE_KEY="$PRIVATE_KEY" forge script script/deploy/facets/DeployCREATE3Factory.s.sol -f $NETWORK -vvvv --verify --json --legacy --broadcast --skip-simulation --gas-limit 2000000) + local RAW_RETURN_DATA + RAW_RETURN_DATA=$(PRIVATE_KEY="$PRIVATE_KEY" \ + forge script script/deploy/facets/DeployCREATE3Factory.s.sol \ + -f "$NETWORK" \ + -vvvv --verify --json --legacy --broadcast --skip-simulation \ + --gas-limit 2000000)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 39-39: 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 (2)
script/deploy/deployAndStoreCREATE3Factory.sh(1 hunks)src/Helpers/CREATE3Factory.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Helpers/CREATE3Factory.sol
🧰 Additional context used
🧬 Code Graph Analysis (1)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
script/helperFunctions.sh (3)
error(2248-2250)getFileSuffix(2064-2074)echoDebug(2239-2247)
🪛 Shellcheck (0.10.0)
script/deploy/deployAndStoreCREATE3Factory.sh
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 39-39: 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: enforce-min-test-coverage
🔇 Additional comments (2)
script/deploy/deployAndStoreCREATE3Factory.sh (2)
7-10: Well-handled parameter validation
The function correctly checks for required arguments and returns a non-zero status on incorrect usage.
21-23: Correctly reading function arguments
Capturing$1and$2intoNETWORKandENVIRONMENTis clear and concise.
Test Coverage ReportLine Coverage: 79.49% (2113 / 2658 lines) |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
53-59: Check address before printing success
The success echo currently comes before validatingFACTORY_ADDRESS. Move the check above theecho "✅ Successfully deployed..."to avoid misleading logs when address extraction fails.
🧹 Nitpick comments (4)
script/deploy/deployAndStoreCREATE3Factory.sh (4)
1-5: Add strict error handling for safer Bash execution
It’s best practice to enable strict mode to fail fast on errors, unset variables, and pipeline failures.
Apply this diff at the top of the file:#!/bin/bash +set -euo pipefail
28-29: Avoid SC2155: separate declaration and assignment
Split thelocaldeclaration and assignment to preserve the return value ofgetFileSuffix:- local FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT") + local FILE_SUFFIX + FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
39-39: Avoid SC2155: mask return code by declare+assign
Separate thelocaland assignment forPRIVATE_KEYso that the script exit code isn’t masked:- local PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") + local PRIVATE_KEY + PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 39-39: Declare and assign separately to avoid masking return values.
(SC2155)
40-41: Quote variables, declare locals, and unify indentation
- Use spaces instead of a tab at the start of line 40.
- Quote
$NETWORKand$PRIVATE_KEY.- Declare
RAW_RETURN_DATAandRETURN_CODEwithlocal.- RAW_RETURN_DATA=$(PRIVATE_KEY="$PRIVATE_KEY" forge script script/deploy/facets/DeployCREATE3Factory.s.sol -f $NETWORK -vvvv --verify --json --legacy --broadcast --skip-simulation --gas-limit 2000000) - RETURN_CODE=$? + local RAW_RETURN_DATA + local RETURN_CODE + RAW_RETURN_DATA=$(PRIVATE_KEY="$PRIVATE_KEY" forge script script/deploy/facets/DeployCREATE3Factory.s.sol \ + -f "$NETWORK" -vvvv --verify --json --legacy --broadcast --skip-simulation --gas-limit 2000000) + RETURN_CODE=$?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
script/deploy/deployAndStoreCREATE3Factory.sh(1 hunks)script/deploy/facets/DeployCREATE3Factory.s.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/deploy/facets/DeployCREATE3Factory.s.sol
🧰 Additional context used
🧬 Code Graph Analysis (1)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
script/helperFunctions.sh (3)
error(2248-2250)getFileSuffix(2064-2074)echoDebug(2239-2247)
🪛 Shellcheck (0.10.0)
script/deploy/deployAndStoreCREATE3Factory.sh
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 39-39: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (7)
script/deploy/deployAndStoreCREATE3Factory.sh (7)
12-19: No changes needed for blank-line logging or sourcing order beyond the above. The current conventions for log separation and CWD assumptions are acceptable in this repo.
21-23: Reading function arguments correctly
Usinglocal NETWORK="$1"andlocal ENVIRONMENT="$2"inside the function is appropriate for scoping.
31-36: Debug logging is clear and useful
UsingechoDebugto print internal state aids troubleshooting when$DEBUGis enabled.
46-49: Deployment exit check is solid
VerifyingRETURN_CODEimmediately ensures we don’t proceed on failures. The error message is clear.
61-65: Network existence check is correct
Usingjq -eto assert the network key exists and early-return on failure aligns with robust parameter handling.
67-76: Atomic update ofnetworks.jsonis well-implemented
Leveragingmktempandjqfor an atomic write minimizes risk of corruption.
77-82: Final logging and return are clear
The closing info message and explicitreturn 0make the function’s outcome obvious to callers.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
script/deploy/deployAndStoreCREATE3Factory.sh (7)
1-2: Enable strict mode for robust error handling.Consider adding
set -euo pipefailafter the shebang to:
- Exit immediately on any command failure (
-e).- Treat unset variables as errors (
-u).- Prevent subtle bugs in pipelines (
-o pipefail).#!/bin/bash + set -euo pipefail
26-27: Ensure.envfile exists to avoid silent failures.If
.envis missing,source .envwill error out. Consider adding a guard:if [[ ! -f .env ]]; then error ".env file not found" return 1 fi source .env
29-30: Split declaration and assignment for accurate return codes (SC2155).Declaring and assigning in one line masks the return code of
getFileSuffix. Separate them:- local FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT") + local FILE_SUFFIX + FILE_SUFFIX=$(getFileSuffix "$ENVIRONMENT")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
32-37: Optional: Combine blank lines and unify logging style.You currently emit multiple blank lines before debug output. You could compress this to a single
echo -e "\n"and standardize oninfovsdebugfunctions for clarity.
54-56: Quote JSON input and use structured logging.Wrap inputs in quotes to prevent word splitting, and consider using an
infohelper:- FACTORY_ADDRESS=$(echo $RETURN_DATA | jq -r '.deployed.value') + FACTORY_ADDRESS=$(echo "$RETURN_DATA" | jq -r '.deployed.value') - echo "✅ Successfully deployed to address $FACTORY_ADDRESS" + echo "[info] ✅ Successfully deployed CREATE3Factory at address $FACTORY_ADDRESS"
63-66: Handle missing or unreadablenetworks.json.Ensure
NETWORKS_JSON_FILE_PATHis set and readable beforejq:if [[ -z "$NETWORKS_JSON_FILE_PATH" || ! -r "$NETWORKS_JSON_FILE_PATH" ]]; then error "networks.json not found or unreadable at '$NETWORKS_JSON_FILE_PATH'" return 1 fi
80-83: Consistent final logging and exit.Consider using
infofor the closing message and compressing blank lines:- echo "[info] <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< CREATE3Factory deployed (please check for warnings)" - echo "" - return 0 + echo "[info] <<< CREATE3Factory deployment complete >>>" + echo -e "\n" + return 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/deploy/deployAndStoreCREATE3Factory.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
script/helperFunctions.sh (3)
error(2248-2250)getFileSuffix(2064-2074)echoDebug(2239-2247)
🪛 Shellcheck (0.10.0)
script/deploy/deployAndStoreCREATE3Factory.sh
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 40-40: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (4)
script/deploy/deployAndStoreCREATE3Factory.sh (4)
11-14: Good: Validate function parameters.The check ensures the required
<NETWORK>and<ENVIRONMENT>arguments are provided before proceeding.
21-24: Uselocalto scope parameters.Declaring
NETWORKandENVIRONMENTaslocalvariables limits their scope to the function, preventing global pollution.
57-60: Good: Guard against missing or null address.The check prevents storing an empty or invalid address in
networks.json.
69-75: Declaretmpfileaslocaland handlemktempfailures.- tmpfile=$(mktemp) + local tmpfile + tmpfile=$(mktemp) || { error "mktemp failed"; return 1; }Scoping temp files and checking for creation errors improves robustness.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
script/deploy/deployAndStoreCREATE3Factory.sh (2)
29-37: ReviewFILE_SUFFIXusage
You computeFILE_SUFFIXand log it in debug, but you never apply it when reading/writingnetworks.json. If multi‑environment files (e.g.,networks.staging.json) are intended, incorporateFILE_SUFFIXintoNETWORKS_JSON_FILE_PATH. Otherwise, consider removing the unused variable.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
41-46: Improve quoting and logging of deployment output
- Quote
$NETWORKin theforgecommand to guard against special chars (-f "$NETWORK").- Log the raw return data under debug before cleaning.
- Fix the
echoquoting forCLEAN_RETURN_DATA.Proposed diff:
- RAW_RETURN_DATA=$(PRIVATE_KEY="$PRIVATE_KEY" forge script script/deploy/facets/DeployCREATE3Factory.s.sol -f $NETWORK -vvvv --verify --json --legacy --broadcast --skip-simulation --gas-limit 2000000) + RAW_RETURN_DATA=$(PRIVATE_KEY="$PRIVATE_KEY" forge script script/deploy/facets/DeployCREATE3Factory.s.sol \ + -f "$NETWORK" -vvvv --verify --json --legacy --broadcast --skip-simulation --gas-limit 2000000) RETURN_CODE=$? + echoDebug "RAW_RETURN_DATA: $RAW_RETURN_DATA" unset PRIVATE_KEY - CLEAN_RETURN_DATA=$(echo "$RAW_RETURN_DATA" | sed 's/^.*{\"logs/{\"logs/') - echo "CLEAN_RETURN_DATA: "$CLEAN_RETURN_DATA"" + CLEAN_RETURN_DATA=$(echo "$RAW_RETURN_DATA" | sed 's/^.*{"logs/{"logs/') + echoDebug "CLEAN_RETURN_DATA: $CLEAN_RETURN_DATA" RETURN_DATA=$(echo "$CLEAN_RETURN_DATA" | jq -r '.returns' 2>/dev/null)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 45-45: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/deploy/deployAndStoreCREATE3Factory.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#1113
File: script/deploy/deployAndStoreCREATE3Factory.sh:40-46
Timestamp: 2025-04-23T02:46:41.926Z
Learning: For lifinance/contracts repository, do not flag or suggest splitting bash variable declarations from assignments (e.g., `local VAR=$(command)` vs `local VAR; VAR=$(command)`). The team is aware of the potential masking of return codes (SC2155) and has made a conscious decision to use the combined syntax.
🧬 Code Graph Analysis (1)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
script/helperFunctions.sh (3)
error(2248-2250)getFileSuffix(2064-2074)echoDebug(2239-2247)
🪛 Shellcheck (0.10.0)
script/deploy/deployAndStoreCREATE3Factory.sh
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 40-40: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 45-45: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (9)
script/deploy/deployAndStoreCREATE3Factory.sh (9)
7-8: Helper functions sourced before use – good move
Sourcingscript/helperFunctions.sh(which defineserror(),echoDebug(), etc.) before you callerrorin the parameter check ensures the helper is available and avoids undefined‑command failures.
11-14: Parameter validation guard is properly placed
Checking"$#"before reading"$1"/"$2"protects against missing arguments. The use ofreturn 1inside the function is also correct for signaling failure in a sourced script.
21-24: Local variable declarations for arguments
Usinglocal NETWORK="$1"andlocal ENVIRONMENT="$2"scopes these names to the function—great for preventing pollution of the outer shell environment.
29-30: SC2155 static analysis is intentionally disabled here
Per team conventions, you’re combining declaration and assignment forFILE_SUFFIX. We’ll respect the existing learning and not split these.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
40-40: SC2155 at PRIVATE_KEY assignment is acceptable
Given the repository’s decision to combine declaration and assignment, no change is needed here.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 40-40: Declare and assign separately to avoid masking return values.
(SC2155)
60-66: Deployed address extraction and validation flow is solid
You correctly parse.deployed.valueand guard against empty ornulladdresses before proceeding.
75-83: Atomic update of JSON via temporary file is best practice
Usingmktemp, pipingjqoutput into it, and thenmv-ing over the original ensures you won’t corruptnetworks.jsonon failures.
83-85: Confirmation log after storing address
The success message clearly communicates that the new address was persisted. Good for tracing deployment outcomes.
68-72:Details
❓ Verification inconclusive
Verify
NETWORKS_JSON_FILE_PATHis defined in config
You reference$NETWORKS_JSON_FILE_PATHto locatenetworks.json. Ensure this variable is set byscript/config.sh. You can run:Expect to see a line defining or exporting that variable.
🏁 Script executed:
#!/bin/bash # Check that NETWORKS_JSON_FILE_PATH is exported in config.sh grep -R "NETWORKS_JSON_FILE_PATH" script/config.shLength of output: 100
Let's locate where
config.shactually lives and verify whetherNETWORKS_JSON_FILE_PATHis defined anywhere in the repo:
🏁 Script executed:
#!/bin/bash # 1) Find all config.sh files in the repo echo "== CONFIG.SH Locations ==" fd --hidden config.sh # 2) Search for any definition or export of NETWORKS_JSON_FILE_PATH echo "== NETWORKS_JSON_FILE_PATH Definitions ==" grep -R "NETWORKS_JSON_FILE_PATH" .Length of output: 6983
Gathering where and how
NETWORKS_JSON_FILE_PATHis actually set and whetherconfig.shis sourced:
🏁 Script executed:
#!/bin/bash echo "== config.sh files ==" fd --hidden config.sh echo "== ‘source’ usage in scripts ==" grep -R "source" -n script echo "== deployAndStoreCREATE3Factory.sh top (1–50) ==" sed -n '1,50p' script/deploy/deployAndStoreCREATE3Factory.sh echo "== config.example.sh (1–200) ==" sed -n '1,200p' script/config.example.shLength of output: 14557
Ensure
script/config.shDefinesNETWORKS_JSON_FILE_PATH
I couldn’t find ascript/config.shin the repo—even though all deploy scripts source it at runtime. Please confirm that you’ve createdscript/config.sh(for example, by copyingscript/config.example.sh) and that it includes a line such as:NETWORKS_JSON_FILE_PATH="$(dirname "$0")/../config/networks.json"Then verify with:
grep -R "NETWORKS_JSON_FILE_PATH" script/config.sh
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
script/deploy/deployAndStoreCREATE3Factory.sh (2)
1-2: Consider enabling strict mode for more robust error handling.Adding
set -euo pipefail(or at leastset -eandset -u) after the shebang can help catch unhandled errors, undefined variables, and pipeline failures early in this script.
40-46: Improve indentation consistency and quoting in the deployment block.
- Lines 41–42 and 44–46 use a tab for indentation; convert these to two spaces to match the rest of the function.
- Quote the
$NETWORKvariable in the Forge CLI invocation to guard against unexpected whitespace.- Consider scoping
RAW_RETURN_DATA,RETURN_CODE,CLEAN_RETURN_DATA, andRETURN_DATAaslocalto avoid leaking them into the global namespace.Example diff:
- local PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") - RAW_RETURN_DATA=$(PRIVATE_KEY="$PRIVATE_KEY" forge script script/deploy/facets/DeployCREATE3Factory.s.sol -f $NETWORK -vvvv --verify --json --legacy --broadcast --skip-simulation --gas-limit 2000000) - RETURN_CODE=$? - unset PRIVATE_KEY - CLEAN_RETURN_DATA=$(echo "$RAW_RETURN_DATA" | sed 's/^.*{\"logs/{\"logs/') - echo "CLEAN_RETURN_DATA: "$CLEAN_RETURN_DATA"" - RETURN_DATA=$(echo "$CLEAN_RETURN_DATA" | jq -r '.returns' 2>/dev/null) + local PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") + local RAW_RETURN_DATA=$(PRIVATE_KEY="$PRIVATE_KEY" forge script script/deploy/facets/DeployCREATE3Factory.s.sol \ + -f "$NETWORK" -vvvv --verify --json --legacy --broadcast --skip-simulation --gas-limit 2000000) + local RETURN_CODE=$? + unset PRIVATE_KEY + local CLEAN_RETURN_DATA=$(echo "$RAW_RETURN_DATA" | sed 's/^.*{"logs/{"logs/') + echo "CLEAN_RETURN_DATA: $CLEAN_RETURN_DATA" + local RETURN_DATA=$(echo "$CLEAN_RETURN_DATA" | jq -r '.returns' 2>/dev/null)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 40-40: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 45-45: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/deploy/deployAndStoreCREATE3Factory.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#1113
File: script/deploy/deployAndStoreCREATE3Factory.sh:40-46
Timestamp: 2025-04-23T02:46:41.926Z
Learning: For lifinance/contracts repository, do not flag or suggest splitting bash variable declarations from assignments (e.g., `local VAR=$(command)` vs `local VAR; VAR=$(command)`). The team is aware of the potential masking of return codes (SC2155) and has made a conscious decision to use the combined syntax.
🧬 Code Graph Analysis (1)
script/deploy/deployAndStoreCREATE3Factory.sh (1)
script/helperFunctions.sh (3)
error(2248-2250)getFileSuffix(2064-2074)echoDebug(2239-2247)
🪛 Shellcheck (0.10.0)
script/deploy/deployAndStoreCREATE3Factory.sh
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 40-40: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 45-45: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (9)
script/deploy/deployAndStoreCREATE3Factory.sh (9)
11-14: Parameter validation looks good.The script correctly checks for at least two arguments and uses the
errorhelper to report usage issues. No further changes needed here.
29-30: LocalFILE_SUFFIXassignment is correct.The function-suffix logic is encapsulated and
FILE_SUFFIXis properly scoped. Nice use ofgetFileSuffix.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
32-36: Informative debug logging.Using
echoDebugto output internal state is helpful whenDEBUG=true. This improves observability without cluttering normal runs.
49-52: Return-code check is correctly ordered.Verifying
$RETURN_CODEbefore inspectingRETURN_DATAprevents misleading “no data found” errors when the Forge script actually failed.
55-58: Return-data validation is solid.Checking for empty or
nullreturn data ensures the deployment script returned expected values before proceeding.
61-67: Deployed address extraction and validation are correct.You extract
FACTORY_ADDRESSfrom the JSON and guard against missing ornullvalues. Clear user feedback with the✅and error messages improves maintainability.
69-73: Networks.json existence check is well-implemented.Using
jq -eto verify the network key before updating avoids silent failures or corrupting the configuration file.
76-82: Safe JSON update with a temporary file.The workflow of writing to a
mktempfile and moving it back ensures atomic updates fornetworks.json. Good job handling errors and cleanup.
84-86: Final storage and user feedback.Moving the temp file into place and echoing a success message completes the flow nicely. The quotes around variables prevent word-splitting issues.
4 participants
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/LF-13286
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)