LF-12061 Run healthcheck on new network #985
Conversation
WalkthroughThis PR introduces a new GitHub Actions workflow that triggers on pull requests modifying Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
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
🧹 Nitpick comments (3)
.github/workflows/healthCheckForNewNetworkDeployment.yml (3)
3-10: Verify Trigger File Path Consistency.The workflow triggers on changes to
'networks.json'. However, the PR summary mentions updates toconfig/networks.json. Please double-check that the intended file path is being monitored. If the networks file resides under a subdirectory (e.g.,config/), update the trigger paths accordingly.
26-46: Validate New Networks Detection Logic & Clean Formatting.The script under the "Detect Newly Added Networks" step effectively:
- Fetches the main branch with
git fetch origin main --depth=1- Retrieves keys from both the previous (
git show origin/main:networks.json | jq 'keys') and current (jq 'keys' networks.json) versions usingjq- Uses a neat subtraction (
$new - $old) to determine new network entries.Please verify that the file paths used here (i.e.
"networks.json") align with the actual location of the networks configuration, especially in light of the PR description. Additionally, remove the trailing whitespace on line 33 to conform with YAML formatting best practices.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 33-33: trailing spaces
(trailing-spaces)
47-55: Iterate Health Checks Over Detected Networks with Robustness.The "Run Health Checks on New Networks" step correctly checks that health checks are only run when new networks are detected (i.e. when
env.CONTINUEis not set to'true') and iterates over them. For improved robustness:
- Consider quoting the loop variable (e.g.,
"$network") to safely handle any potential whitespace in network names.- Optionally, add a
set -eat the beginning of the script block to ensure the script exits immediately on any unexpected error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/healthCheckForNewNetworkDeployment.yml(1 hunks)config/networks.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/networks.json
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/healthCheckForNewNetworkDeployment.yml
[error] 33-33: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (2)
.github/workflows/healthCheckForNewNetworkDeployment.yml (2)
1-2: Workflow Name is Clear and Concise.The workflow name "Health Check for New Network Deployment" clearly communicates its purpose and scope.
11-25: Job Setup Steps are Standard and Correct.The steps for checking out the repository, installing Bun using the
oven-sh/setup-bun@v1action, and runningbun installfor dependencies are implemented as expected.
Test Coverage ReportLine Coverage: 78.66% (2238 / 2845 lines) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/healthCheckForNewNetworkDeployment.yml (3)
18-22: Bun Installation Step – Consider Version Pinning
Usingoven-sh/setup-bun@v1withbun-version: latestworks, but consider pinning to a specific version to avoid unexpected breakage from future Bun releases.
33-33: Remove Trailing Whitespaces
There are trailing spaces detected on this line (as flagged by YAMLlint). Please remove them to keep the workflow file clean and compliant with linting rules.Proposed diff:
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 33-33: trailing spaces
(trailing-spaces)
47-54: Executing Health Checks for New Networks is Robust
The conditional (if: env.CONTINUE != 'true') correctly ensures that health checks run only when new networks are detected. The loop iterates over theadded_networksvariable and exits on failure, which provides a safeguard against unhealthy network configurations.Additional Refactor Suggestion: Consider adding a validation step for the
added_networksvariable (e.g., checking if the JSON is valid) before entering the loop, though this may not be necessary if the file is always well-formed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/healthCheckForNewNetworkDeployment.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/healthCheckForNewNetworkDeployment.yml
[error] 33-33: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
🔇 Additional comments (7)
.github/workflows/healthCheckForNewNetworkDeployment.yml (7)
1-10: Trigger Configuration is Clear and Focused
The workflow is configured to trigger only on changes to theconfig/networks.jsonfile for both pushes and pull requests. This focused trigger minimizes unnecessary runs and is aligned with the PR objectives.
11-17: Job Setup and Checkout Step are Correct
The jobcheck-new-network-healthis properly defined to run onubuntu-latest, and the checkout step (usingactions/checkout@v4) is implemented correctly.
23-25: Dependency Installation is Straightforward
Runningbun installto install dependencies is clear and concise.
26-28: Detect Newly Added Networks: Initial Setup is Clear
The step’s heading and the overall intent—to compare the currentnetworks.jsonwith that fromorigin/main—are clear and well-commented.
29-32: Fetching and Extracting Network Keys is Well-Implemented
The script fetches the previous version ofnetworks.jsonand extracts keys usingjq, which is an effective approach for detecting changes. Ensure that the main branch name is consistent with your repository’s default branch.
34-36: Logging for Debugging is Informative
Echoing both the old and new network keys helps with debugging and verifying that the correct data is being compared.
37-45: Calculation and Environment Variable Assignment for New Networks is Effective
Usingjqto compute the difference between the new and old keys and then conditionally setting environment variables (CONTINUEoradded_networks) is a clever solution. This logic ensures that subsequent steps only execute when needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/healthCheckForNewNetworkDeployment.yml (2)
23-25: Dependency Installation: Remove Trailing WhitespaceThere is a trailing space observed on line 25. Removing such extraneous whitespace can help maintain cleanliness and consistency in the YAML file.
- run: bun install + run: bun install🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 25-25: trailing spaces
(trailing-spaces)
58-66: Health Check Execution: Effective with a Minor Improvement SuggestionThe loop that runs health checks on the new networks correctly iterates over the identified networks and exits with an error if any health check fails. For improved robustness, it’s advisable to quote the variable when echoing
added_networksin the loop, especially to safeguard against any unexpected spacing issues:- for network in $(echo $added_networks | jq -r '.[]'); do + for network in $(echo "$added_networks" | jq -r '.[]'); do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/healthCheckForNewNetworkDeployment.yml(1 hunks)config/networks.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/networks.json
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/healthCheckForNewNetworkDeployment.yml
[error] 25-25: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
🔇 Additional comments (6)
.github/workflows/healthCheckForNewNetworkDeployment.yml (6)
1-2: Workflow Naming: Clear and DescriptiveThe workflow name "Health Check for New Network Deployment" succinctly conveys its intent. This aligns well with the PR objectives.
3-10: Trigger Configuration: Specific and FocusedThe workflow is configured to trigger on changes to the
config/networks.jsonfile for both push and pull request events. This targeted trigger ensures that the job only runs when relevant changes occur.
11-17: Job Setup: Proper Checkout StepThe job configuration uses
actions/checkout@v4to check out the repository, which is standard and ensures that the latest code is available for subsequent steps.
18-22: Bun Installation: Up-to-Date EnvironmentThe step using
oven-sh/setup-bun@v1withbun-version: latestconfirms that the environment will have the latest Bun runtime installed. This is appropriate given the project’s dependency needs.
26-28: Louper CLI Installation: Clear and DirectThe installation of the Louper CLI via the command
bun add -g @mark3labs/louper-cliis clear and straightforward, ensuring the necessary tool is available for the subsequent steps.
30-57: Detect Newly Added Networks: Robust and Thoughtful LogicThe script block that detects newly added networks:
- Efficiently compares the current
config/networks.jsonwith the version from the main branch.- Uses
git fetchwith a fallback to handle scenarios where the previous commit might not be available.- Appropriately utilizes
jqto extract and compare keys, setting theskip_checkflag or storing the added networks via environment variables.This approach is robust and aligns well with the intended automation. Consider enhancing error logging (e.g., if
jqencounters issues) for even clearer diagnostics in edge cases, though this is a minor recommendation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/healthCheckForNewNetworkDeployment.yml (1)
23-31: Installing Dependencies and Louper CLI
The dependency installation (line 23–25) usingbun installis simple and effective. In the “Install Louper CLI” step (lines 26–31), note that the CLI installation is performed twice (once normally, then re-run after trusting postinstall scripts). This may be intentional for ensuring the CLI is trusted and fully setup; however, please verify if the duplicate invocation is necessary.deployments/arbitrumtest.json (1)
1-60: Deployment Addresses for “arbitrumtest”
This JSON file defines the facets and their deployed addresses for the new "arbitrumtest" network and appears to be a duplicate of an existing configuration (e.g. “arbitrum”). The formatting and structure are clear.Static Analysis Note:
Static analysis flagged lines 6 and 44 as containing a Generic API Key. These entries are Ethereum addresses (public on-chain) that represent deployed contract addresses. They are not secret API keys. It is recommended to add a comment or documentation note clarifying that these values are public addresses used for network deployments so that false positives can be safely ignored.🧰 Tools
🪛 Gitleaks (8.21.2)
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
44-44: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
script/deploy/_targetState.json (1)
1-1674: Comprehensive Target State Configuration
This very long JSON file holds the deployment version configuration for numerous networks. Each network (e.g. mainnet, polygon, bsc, arbitrum, arbitrumtest, etc.) lists detailed facet versions for theLiFiDiamondcontract (and variants such asLiFiDiamondImmutable). The structure is consistent with a nested configuration (network → environment → contract), which is good for managing deployments.Suggestions:
- Maintainability: Given the size of the file (over 1600 lines), consider modularizing the data by grouping common configurations or maintaining separate files by network. This may simplify future updates and version management.
- Consistency Check: Please verify that version numbers for similar facets are intended to be repeated across all networks. In some cases, you might want to centralize default versions for consistency and easier updates.
- Formatting: Although the file is correct, ensure that a final newline is added (if not already present) to follow standard conventions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1615-1615: The key sonic 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/healthCheckForNewNetworkDeployment.yml(1 hunks)deployments/arbitrumtest.json(1 hunks)script/deploy/_targetState.json(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
deployments/arbitrumtest.json
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
44-44: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
🔇 Additional comments (7)
.github/workflows/healthCheckForNewNetworkDeployment.yml (7)
1-10: Workflow Setup and Trigger Configuration
The header, name, and trigger paths (lines 1–10) are clear and correctly set to fire on changes toconfig/networks.json. Please double-check that this file alone is the intended change trigger.
11-22: Job and Dependency Installation
The job definition (lines 11–14) and the steps to checkout the repository and install Bun (lines 15–22) are implemented correctly.
32-39: Setting PATH and Environment Variables
The steps to set the PATH (lines 32–39) are well implemented. Appending the new path to$GITHUB_ENVand exporting it in the current shell is a good practice to ensure subsequent steps can use the CLI.
40-47: Verifying Louper CLI Installation
The verification step (lines 40–47) properly checks the availability of thelouper-clicommand and exits on failure. This is a solid safeguard in the workflow.
48-75: Detection of Newly Added Networks
The script block (lines 48–75) successfully fetches the previous commit’snetworks.json(with a fallback), extracts keys usingjq, and computes the difference. Ensure that the fallback behavior (when no previous commit is found) meets your requirements (i.e. treating all networks as new).
76-95: Validation of Required Deployment Files
In the “Validate Network Deployment Files” step (lines 76–95), the loop correctly iterates over the derived list of added networks. This block properly verifies the existence of the corresponding entry in_targetState.jsonand the deployment JSON file.
96-104: Running Health Checks
The health check step (lines 96–104) iterates over the new networks and executes the health check script using Bun. The use of an exit failure on error (with|| exit 1) is correct.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
script/deploy/healthCheck.ts (1)
180-209: Reduce or restructure debug logging where feasible.The debug logs (e.g.,
consola.warn('facetsResult:', facetsResult)in lines 191–192) may unnecessarily clutter CI logs, especially over multiple retries. Consider lowering their verbosity or providing a flag to toggle debug logs, ensuring that essential information is still captured for troubleshooting..github/workflows/healthCheckForNewNetworkDeployment.yml (2)
30-30: Remove trailing spaces.Trailing spaces break some YAML linting rules. Please remove the extra spaces at the end of this line.
- npm install -g @mark3labs/louper-cli + npm install -g @mark3labs/louper-cli🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 30-30: trailing spaces
(trailing-spaces)
42-42: Remove trailing spaces.Similar linting issue here; remove any trailing spaces for a clean YAML configuration.
- echo "✅ Louper CLI is installed and ready to use." + echo "✅ Louper CLI is installed and ready to use."🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 42-42: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/healthCheckForNewNetworkDeployment.yml(1 hunks).github/workflows/networkSecretsChecker.yml(1 hunks)script/deploy/healthCheck.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/networkSecretsChecker.yml
🧰 Additional context used
🪛 GitHub Actions: Health Check for New Network Deployment
script/deploy/healthCheck.ts
[warning] 568-568: Can't find variable: diamondAddress
[error] 588-588: Max retries reached: Diamond details still not available.
[error] Facet DiamondCutFacet not registered in Diamond or possibly unverified
[error] Facet DiamondLoupeFacet not registered in Diamond or possibly unverified
[error] Facet OwnershipFacet not registered in Diamond or possibly unverified
[error] Facet WithdrawFacet not registered in Diamond or possibly unverified
[error] Facet DexManagerFacet not registered in Diamond or possibly unverified
[error] Facet PeripheryRegistryFacet not registered in Diamond or possibly unverified
[error] Facet AccessManagerFacet not registered in Diamond or possibly unverified
[error] Facet GenericSwapFacet not registered in Diamond or possibly unverified
[error] Facet GenericSwapFacetV3 not registered in Diamond or possibly unverified
[error] Facet CalldataVerificationFacet not registered in Diamond or possibly unverified
[error] Facet StandardizedCallFacet not registered in Diamond or possibly unverified
[error] Facet LIFuelFacet not registered in Diamond or possibly unverified
[error] Facet EmergencyPauseFacet not registered in Diamond or possibly unverified
[error] Facet AcrossFacet not registered in Diamond or possibly unverified
[error] Facet AllBridgeFacet not registered in Diamond or possibly unverified
[error] Facet AmarokFacet not registered in Diamond or possibly unverified
[error] Facet CBridgeFacet not registered in Diamond or possibly unverified
[error] Facet CelerCircleBridgeFacet not registered in Diamond or possibly unverified
[error] Facet GasZipFacet not registered in Diamond or possibly unverified
[error] Facet HopFacet not registered in Diamond or possibly unverified
[error] Facet MayanFacet not registered in Diamond or possibly unverified
[error] Facet SquidFacet not registered in Diamond or possibly unverified
[error] Facet StargateFacet not registered in Diamond or possibly unverified
[error] Facet SymbiosisFacet not registered in Diamond or possibly unverified
[error] ERC20Proxy owner is incorrect.
[error] FeeCollector owner is incorrect.
🪛 YAMLlint (1.35.1)
.github/workflows/healthCheckForNewNetworkDeployment.yml
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: generate-tag
🔇 Additional comments (2)
script/deploy/healthCheck.ts (2)
555-556: Confirm whether failing on no DEX configuration is intended.The script immediately logs an error and exits if no DEX is configured. Verify that this condition should indeed be treated as a hard failure. If a network legitimately operates without DEX configurations, failing may be too strict.
649-651: Exiting with correct return codes is appropriate.Gracefully exiting with a non-zero code on errors and zero otherwise follows standard CI best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
script/deploy/healthCheck.ts (3)
181-207: Enhance error handling and logging.The error handling and logging could be improved to help diagnose issues in CI:
- Remove or guard the raw facetsResult logging in local mode.
- Add more context to the error message when parsing fails.
- consola.warn('facetsResult:', facetsResult) + // Only log in debug mode + if (process.env.DEBUG) { + consola.debug('facetsResult:', facetsResult) + } parsedOutput = JSON.parse(facetsResult.stdout) } } else { throw new Error( - 'Facets not found or in an unexpected format in the louper output.' + 'Facets not found or in an unexpected format in the louper output. ' + + 'Expected an object with a "facets" array property.' )🧰 Tools
🪛 GitHub Actions: Health Check for New Network Deployment
[warning] 204-204: Unable to parse louper output - skipping facet registration check
564-621: Improve the robustness of diamond details fetching.The implementation could be enhanced in several ways:
- Make the delay configurable
- Add retry logic for transient failures
- Guard verbose logging
const fetchDiamondDetailsWithDelay = ( diamondAddress: string, - network: string + network: string, + options: { + delayMs?: number; + maxRetries?: number; + debug?: boolean; + } = {} ): Promise<any> => { + const { + delayMs = 15000, + maxRetries = 3, + debug = false + } = options; + let attempts = 0; + + const tryFetch = (): Promise<any> => { + attempts++; return new Promise((resolve, reject) => { const args = ['inspect', 'diamond', '-a', diamondAddress, '-n', network, '--json'] - consola.info(`Executing: ${louperCmd} ${args.join(' ')}`) + debug && consola.info(`Executing: ${louperCmd} ${args.join(' ')}`) const proc = spawn(louperCmd, args) let stdoutData = '' proc.stdout.on('data', (data: Buffer) => { stdoutData += data.toString() }) proc.stderr.on('data', (data: Buffer) => { - consola.error(`stderr: ${data.toString()}`) + debug && consola.error(`stderr: ${data.toString()}`) }) proc.on('error', (err) => { - reject(err) + if (attempts < maxRetries) { + consola.warn(`Attempt ${attempts}/${maxRetries} failed, retrying...`) + setTimeout(() => tryFetch().then(resolve).catch(reject), 1000) + } else { + reject(new Error(`Failed after ${maxRetries} attempts: ${err.message}`)) + } }) proc.on('close', (code) => { - consola.info(`Process exited with code ${code}. Waiting 15 seconds...`) + debug && consola.info(`Process exited with code ${code}. Waiting ${delayMs}ms...`) setTimeout(() => { try { - consola.info('Output captured from Louper CLI:', stdoutData) + debug && consola.debug('Output captured from Louper CLI:', stdoutData) const parsedOutput = JSON.parse(stdoutData) resolve(parsedOutput) } catch (e) { reject(new Error(`Error parsing Louper CLI output: ${e}`)) } - }, 15000) + }, delayMs) }) }) + } + return tryFetch() }
677-685: Enhance error reporting in finish function.The function could provide more detailed error summaries to help diagnose deployment issues.
const finish = () => { if (errors.length) { - consola.error(`${errors.length} Errors found in deployment`) + consola.error('\nDeployment Check Summary:') + consola.error(`Found ${errors.length} error${errors.length === 1 ? '' : 's'}:`) + errors.forEach((error, index) => { + consola.error(`${index + 1}. ${error}`) + }) process.exit(1) } else { consola.success('Deployment checks passed') process.exit(0) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/deploy/healthCheck.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Health Check for New Network Deployment
script/deploy/healthCheck.ts
[warning] 204-204: Unable to parse louper output - skipping facet registration check
[error] Facet DiamondCutFacet not registered in Diamond or possibly unverified
[error] Facet DiamondLoupeFacet not registered in Diamond or possibly unverified
[error] Facet OwnershipFacet not registered in Diamond or possibly unverified
[error] Facet WithdrawFacet not registered in Diamond or possibly unverified
[error] Facet DexManagerFacet not registered in Diamond or possibly unverified
[error] Facet PeripheryRegistryFacet not registered in Diamond or possibly unverified
[error] Facet AccessManagerFacet not registered in Diamond or possibly unverified
[error] Facet PeripheryRegistryFacet not registered in Diamond or possibly unverified
[error] Facet GenericSwapFacet not registered in Diamond or possibly unverified
[error] Facet GenericSwapFacetV3 not registered in Diamond or possibly unverified
[error] Facet CalldataVerificationFacet not registered in Diamond or possibly unverified
[error] Facet StandardizedCallFacet not registered in Diamond or possibly unverified
[error] Facet LIFuelFacet not registered in Diamond or possibly unverified
[error] Facet EmergencyPauseFacet not registered in Diamond or possibly unverified
[error] Facet AcrossFacet not registered in Diamond or possibly unverified
[error] Facet AllBridgeFacet not registered in Diamond or possibly unverified
[error] Facet AmarokFacet not registered in Diamond or possibly unverified
[error] Facet CBridgeFacet not registered in Diamond or possibly unverified
[error] Facet CelerCircleBridgeFacet not registered in Diamond or possibly unverified
[error] Facet GasZipFacet not registered in Diamond or possibly unverified
[error] Facet HopFacet not registered in Diamond or possibly unverified
[error] Facet MayanFacet not registered in Diamond or possibly unverified
[error] Facet SquidFacet not registered in Diamond or possibly unverified
[error] Facet StargateFacet not registered in Diamond or possibly unverified
[error] Facet SymbiosisFacet not registered in Diamond or possibly unverified
[error] ERC20Proxy owner is 0x11F11121DF7256C40339393b0FB045321022ce44, expected 0x11F1022cA6AdEF6400e5677528a80d49a069C00c
[error] FeeCollector owner is 0xCB1e9fA11Edd27098A6B2Ff23cB6b79183ccf8Ee, expected 0x08647cc950813966142A416D40C382e2c5DB73bB
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/deploy/healthCheck.ts(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
script/deploy/healthCheck.ts
[error] 51-51: Shouldn't redeclare 'networks'. Consider to delete it or rename it.
'networks' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (3)
script/deploy/healthCheck.ts (3)
162-216: LGTM! Improved facet inspection with direct blockchain calls.The switch from Louper CLI to direct blockchain calls using
castis a good improvement. The code properly handles:
- RPC URL validation
- JSON parsing and transformation
- Facet address comparison and validation
565-567: LGTM! Added proper error handling for no-DEXs case.The addition of
finish()call when no DEXs are configured ensures proper script termination.
625-633: LGTM! Improved error handling with explicit exit codes.The
finish()function now uses explicit exit codes:
- Exit code 1 for errors
- Exit code 0 for success
This makes it easier for CI/CD pipelines to detect failures.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/healthCheckForNewNetworkDeployment.yml (1)
94-101:⚠️ Potential issueUse the dynamically added network instead of hardcoding "arbitrum".
Hardcoding "arbitrum" defeats the purpose of detecting newly added networks. Update to use the loop variable so the correct network is tested.
Apply this diff to fix the network parameter:
- bun run script/deploy/healthCheck.ts --network arbitrum || exit 1 + bun run script/deploy/healthCheck.ts --network "$network" || exit 1
🧹 Nitpick comments (2)
.github/workflows/healthCheckForNewNetworkDeployment.yml (2)
26-42: Enhance Louper CLI installation step.The installation process is thorough but can be improved:
- Remove trailing spaces.
- Simplify PATH modification.
- Add version check to verification.
Apply this diff to enhance the installation step:
- name: Install Louper CLI (Using npm Instead of Bun) run: | echo "🚀 Installing Louper CLI..." - npm install -g @mark3labs/louper-cli - + npm install -g @mark3labs/louper-cli echo "🔧 Setting PATH..." - echo "export PATH=$(npm root -g)/.bin:\$PATH" >> $GITHUB_ENV - export PATH=$(npm root -g)/.bin:$PATH + echo "PATH=$(npm root -g)/.bin:$PATH" >> $GITHUB_ENV echo "🛠️ Verifying installation..." if ! command -v louper-cli &> /dev/null; then echo "❌ Louper CLI not found after installation!" exit 1 fi + echo "📦 Louper CLI version: $(louper-cli --version)" echo "✅ Louper CLI is installed and ready to use." -🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
43-44: Avoid hardcoding the diamond address.The Louper CLI test uses a hardcoded diamond address. Consider making this configurable or using a test address.
Apply this diff to make the address configurable:
- run: louper-cli inspect diamond -a 0x1231DEB6f5749EF6cE6943a275A1D3E7486F4EaE -n arbitrum --json + run: | + DIAMOND_ADDRESS="${{ vars.TEST_DIAMOND_ADDRESS }}" + if [ -z "$DIAMOND_ADDRESS" ]; then + DIAMOND_ADDRESS="0x1231DEB6f5749EF6cE6943a275A1D3E7486F4EaE" + fi + louper-cli inspect diamond -a "$DIAMOND_ADDRESS" -n arbitrum --json
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/healthCheckForNewNetworkDeployment.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/healthCheckForNewNetworkDeployment.yml
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
🔇 Additional comments (1)
.github/workflows/healthCheckForNewNetworkDeployment.yml (1)
11-25: LGTM! Well-structured job setup.The job configuration follows best practices by using specific action versions and proper dependency installation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/healthCheckForNewNetworkDeployment.yml (2)
24-26: Add error handling to the Foundry installation.The curl command for installing Foundry lacks error handling and verification.
Apply this diff to make the installation more resilient:
- name: Install Foundry run: | + set -e # Exit on any error + echo "Downloading Foundry..." curl -L https://foundry.paradigm.xyz | bash + if [ $? -ne 0 ]; then + echo "Failed to download Foundry" + exit 1 + fi echo "$HOME/.foundry/bin" >> $GITHUB_PATH # Make Foundry available globally
40-66: Enhance error handling in network detection.The network detection step could fail silently if jq commands fail.
Apply this diff to improve error handling:
- name: Detect Newly Added Networks id: detect-changes run: | + set -e # Exit on any error echo "Comparing config/networks.json with the previous commit..." - git fetch origin main --depth=1 || echo "No previous commit found." + if ! git fetch origin main --depth=1; then + echo "Failed to fetch main branch. Please ensure the repository is properly configured." + exit 1 + fi if git show origin/main:config/networks.json > /dev/null 2>&1; then - OLD_NETWORKS=$(git show origin/main:config/networks.json | jq 'keys') + if ! OLD_NETWORKS=$(git show origin/main:config/networks.json | jq 'keys' 2>/dev/null); then + echo "Failed to parse old networks.json" + exit 1 + fi else echo "No previous networks.json found, assuming all networks are new." OLD_NETWORKS="[]" fi - NEW_NETWORKS=$(jq 'keys' config/networks.json) + if ! NEW_NETWORKS=$(jq 'keys' config/networks.json 2>/dev/null); then + echo "Failed to parse current networks.json" + exit 1 + fi echo "Old Networks: $OLD_NETWORKS" echo "New Networks: $NEW_NETWORKS" - ADDED_NETWORKS=$(jq -n --argjson old "$OLD_NETWORKS" --argjson new "$NEW_NETWORKS" '$new - $old') + if ! ADDED_NETWORKS=$(jq -n --argjson old "$OLD_NETWORKS" --argjson new "$NEW_NETWORKS" '$new - $old' 2>/dev/null); then + echo "Failed to compute network differences" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/healthCheckForNewNetworkDeployment.yml(1 hunks)
🔇 Additional comments (3)
.github/workflows/healthCheckForNewNetworkDeployment.yml (3)
3-9: Uncomment the path triggers to optimize workflow execution.The workflow should only run when
networks.jsonis modified. Currently, it runs on every push, which is inefficient.
68-87: Add error handling for jq commands.The validation step lacks error handling for jq commands, which could fail silently.
94-94: Use the dynamically added network instead of hardcoding "arbitrum".Hardcoding "arbitrum" defeats the purpose of detecting newly added networks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
script/deploy/healthCheck.ts (1)
51-51:⚠️ Potential issueRemove duplicate import of
networks.The
networksobject is already imported throughviemScriptHelpersat line 21. The direct import at line 51 is redundant.-import networks from '../../config/networks.json'🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Shouldn't redeclare 'networks'. Consider to delete it or rename it.
'networks' is defined here:
(lint/suspicious/noRedeclare)
.github/workflows/healthCheckForNewNetworkDeployment.yml (1)
3-6: 🛠️ Refactor suggestionOptimize workflow triggers.
The workflow should only run when
networks.jsonis modified to avoid unnecessary executions.on: push: + paths: + - 'config/networks.json' pull_request: + paths: + - 'config/networks.json'
🧹 Nitpick comments (4)
script/deploy/healthCheck.ts (2)
162-218: Improve error handling in facet registration check.The facet registration check could be improved with better error handling and logging:
- The error message when RPC URL is missing could be more descriptive
- The error handling for JSON parsing could be more robust
if (networksConfig[network.toLowerCase()].rpcUrl) { const rpcUrl: string = networksConfig[network.toLowerCase()].rpcUrl try { const facetsResult = await $`cast call ${diamondAddress} "facets() returns ((address,bytes4[])[])" --rpc-url ${rpcUrl}` const rawString = facetsResult.stdout const jsonCompatibleString = rawString .replace(/\(/g, '[') .replace(/\)/g, ']') .replace(/0x[0-9a-fA-F]+/g, '"$&"') - const onChainFacets = JSON.parse(jsonCompatibleString) + const onChainFacets = JSON.parse(jsonCompatibleString.trim()) if (Array.isArray(onChainFacets)) { // ... rest of the code } else { + throw new Error('Invalid facets data structure') } } catch (error) { + if (error instanceof SyntaxError) { + consola.error('Failed to parse facets data:', error) + } else { + consola.error('Failed to fetch facets:', error) + } + throw error } } else { - throw new Error('Failed to get rpc from network config file') + throw new Error(`No RPC URL configured for network ${network}`) }
627-635: Improve error reporting in finish function.The
finishfunction should provide more detailed error reporting to help diagnose issues.const finish = () => { if (errors.length) { - consola.error(`${errors.length} Errors found in deployment`) + consola.error('Deployment check failed with the following errors:') + errors.forEach((error, index) => { + consola.error(`${index + 1}. ${error}`) + }) process.exit(1) } else { consola.success('Deployment checks passed') process.exit(0) } }.github/workflows/healthCheckForNewNetworkDeployment.yml (2)
56-75: Enhance validation step error handling.The validation step could benefit from improved error handling and JSON validation.
- name: Validate Network Deployment Files if: env.run_check == 'true' && env.skip_check != 'true' run: | + set -euo pipefail echo "Validating required files for new networks..." for network in $(echo $added_networks | jq -r '.[]'); do echo "🔍 Checking files for network: $network" # Check if network exists in _targetState.json - if ! jq -e 'has("'"$network"'")' script/deploy/_targetState.json > /dev/null; then + if ! jq -e 'has("'"$network"'")' script/deploy/_targetState.json > /dev/null 2>&1; then echo "❌ Error: Network '$network' not found in script/deploy/_targetState.json" exit 1 fi # Check if deployments/{network}.json file exists if [[ ! -f "deployments/$network.json" ]]; then echo "❌ Error: Missing deployment file: deployments/$network.json" exit 1 fi + + # Validate JSON format + if ! jq '.' "deployments/$network.json" > /dev/null 2>&1; then + echo "❌ Error: Invalid JSON format in deployments/$network.json" + exit 1 + fi done
90-99: Add retry mechanism for health checks.The health check execution should include a retry mechanism for transient failures.
- name: Run Health Checks on New Networks if: env.run_check == 'true' && env.skip_check != 'true' run: | + set -euo pipefail echo "Running health check for new networks..." - set -e for network in $(echo $added_networks | jq -r '.[]'); do echo "🔍 Checking network: $network" - bun run script/deploy/healthCheck.ts --network "$network" || exit 1 + max_retries=3 + retry_count=0 + while [ $retry_count -lt $max_retries ]; do + if bun run script/deploy/healthCheck.ts --network "$network"; then + break + else + retry_count=$((retry_count + 1)) + if [ $retry_count -eq $max_retries ]; then + echo "❌ Health check failed after $max_retries attempts" + exit 1 + fi + echo "Retrying health check ($retry_count/$max_retries)..." + sleep 5 + fi + done done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/healthCheckForNewNetworkDeployment.yml(1 hunks)script/deploy/healthCheck.ts(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
script/deploy/healthCheck.ts
[error] 51-51: Shouldn't redeclare 'networks'. Consider to delete it or rename it.
'networks' is defined here:
(lint/suspicious/noRedeclare)
🪛 GitHub Actions: Health Check for New Network Deployment
script/deploy/healthCheck.ts
[error] 1-1: The following facets exist on-chain but are missing in the config: ["0xf7993a8df974ad022647e63402d6315137c58abf"]
[error] 2-2: The following facets exist in the config but are not deployed on-chain: ["0xad50118509eb4c8e3e39a370151b0fd5d5957013","0x1231deb6f5749ef6ce6943a275a1d3e7486f4eae","0x238502adc8ca550723cbe78543c8b757599a21cc","0x5741a7ffe7c39ca175546a54985fa79211290b51","0x2dfadab8266483bed9fd9a292ce56596a2d1378d","0x050e198e36a73a1e32f15c3afc58c4506d82f657","0xb0210de78e28e2633ca200609d9f528c13c26cd9","0xf3552b98bb4234b47674700a99a0308d74b40f51","0x9b11bc9fac17c058cab6286b0c785be6a65492ef","0x73d2d674449b3d8ad549cc3b04f8104c444fd34d","0x7b6d852f58c783ba3b1138c535ff57dda4c826e0","0x0d26d248d4e80377f1d794ab88090e76b0903ede","0x6a8b11bf29c0546991decd6e0db8cc7fda22ba97","0x1c97be47f6da4d2e09b3a11b0a17c513dfd0e896","0xc02ffcdd914dba646704439c6090babad521d04c","0x5215e9fd223bc909083fbdb2860213873046e45d","0x1493e7b8d4dfade0a178dad9335470337a3a219a","0x6140b987d6b51fd75b66c3b07733beb5167c42fc","0x6307119078556fc8ad77781dfc67df20d75fb4f9","0xca6e6b692f568055ada0bf72a06d1ebbc938fb23","0x9a21e33f1a78b17dad32010cedb9fd2f071c17d3"]
[error] 3-3: Facet DiamondCutFacet not registered in Diamond or possibly unverified
[error] 4-4: No dexs configured
[error] 5-5: 4 Errors found in deployment
mirooon
force-pushed
the
lf-12061-run-healthcheck-on-new-network
branch
from
791e4a3 to
43ea772
Compare
mirooon
changed the title
| fi | ||
| done | ||
|
|
||
| - name: Install Bun |
There was a problem hiding this comment.
Would suggest to move the install bits before the code logic....like first set up the environment, then execute code.
There was a problem hiding this comment.
I put deps installation part after validating whether everything is needed, convinced that this could save some resources for this action. Since it's a small action, the impact is minimal I know. The validation code is actually larger than the logic part cuz we only run healthCheck after it, which might make it seem a bit odd. I can change it if needed! Wdyt?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/healthCheckForNewNetworkDeployment.yml (1)
16-16: Remove trailing spaces.Fix the YAMLlint warning by removing trailing spaces.
- fetch-depth: 0 + fetch-depth: 0🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 16-16: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/healthCheckForNewNetworkDeployment.yml(1 hunks).github/workflows/networkSecretsChecker.yml(1 hunks)config/networks.json(1 hunks)script/deploy/healthCheck.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/networkSecretsChecker.yml
- config/networks.json
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/healthCheckForNewNetworkDeployment.yml
[error] 16-16: trailing spaces
(trailing-spaces)
⏰ 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 (7)
script/deploy/healthCheck.ts (3)
146-202: LGTM! Robust implementation of facet registration check.The new implementation using
castcommand is well-structured with:
- Proper RPC URL validation
- Robust output parsing
- Comprehensive comparison between on-chain and configured facets
- Clear error messaging
611-619: LGTM! Clear error handling with explicit exit codes.The updated
finishfunction now properly signals success/failure through process exit codes:
exit(1)for errorsexit(0)for success
551-552: LGTM! Proper error handling for no-dexs case.The code now correctly handles the case when no dexs are configured by calling
finish()to exit with appropriate status code..github/workflows/healthCheckForNewNetworkDeployment.yml (4)
3-8: LGTM! Well-defined workflow triggers.The workflow is correctly configured to:
- Trigger only when
networks.jsonis modified- Run on relevant PR events
29-55: LGTM! Robust network detection logic.The implementation correctly:
- Compares old and new networks.json
- Handles first-time deployment case
- Sets appropriate environment variables
57-75: LGTM! Thorough validation of network deployment files.The validation step properly checks:
- Network existence in _targetState.json
- Deployment file existence
- Clear error messages on validation failures
91-104: LGTM! Well-structured health check execution.The implementation correctly:
- Runs health checks for each new network
- Stops on first failure with
set -e- Provides clear status messages
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
script/deploy/_targetState.json (1)
1834-1862:⚠️ Potential issueRemove duplicate network configuration.
The "sonic" network appears twice in the configuration (lines 1806-1833 and 1834-1862) with different facet versions, which could lead to confusion and deployment issues.
Apply this diff to remove the duplicate configuration:
- "sonic": { - "production": { - "LiFiDiamond": { - "DiamondCutFacet": "1.0.0", - "DiamondLoupeFacet": "1.0.0", - "OwnershipFacet": "1.0.0", - "DexManagerFacet": "1.0.1", - "AccessManagerFacet": "1.0.0", - "WithdrawFacet": "1.0.0", - "PeripheryRegistryFacet": "1.0.0", - "GenericSwapFacet": "1.0.0", - "GenericSwapFacetV3": "1.0.1", - "LIFuelFacet": "1.0.1", - "CalldataVerificationFacet": "1.1.2", - "StandardizedCallFacet": "1.1.0", - "EmergencyPauseFacet": "1.0.1", - "LiFiDiamond": "1.0.0", - "ERC20Proxy": "1.0.0", - "Executor": "2.0.0", - "FeeCollector": "1.0.0", - "Receiver": "2.0.2", - "LiFuelFeeCollector": "1.0.1", - "TokenWrapper": "1.0.0", - "LiFiDEXAggregator": "1.5.0", - "GasZipPeriphery": "1.0.0", - "GasZipFacet": "2.0.0" - } - } - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deployments/bsca.json(1 hunks)deployments/bscb.json(1 hunks)script/deploy/_targetState.json(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
deployments/bscb.json
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
42-42: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
deployments/bsca.json
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
42-42: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (1)
deployments/bsca.json (1)
1-55: Well-organized contract structure.The contract addresses are logically organized into groups (Diamond, Core, Bridge, Utility facets) with consistent naming conventions, making it easier to maintain and understand the system architecture.
🧰 Tools
🪛 Gitleaks (8.21.2)
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
42-42: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
script/deploy/healthCheck.ts (2)
146-202: Improve error handling for RPC URL validation.The RPC URL check could be more robust. Consider validating the URL format and adding a retry mechanism for transient RPC failures.
- if (networksConfig[network.toLowerCase()].rpcUrl) { - const rpcUrl: string = networksConfig[network.toLowerCase()].rpcUrl + const networkConfig = networksConfig[network.toLowerCase()] + if (!networkConfig?.rpcUrl) { + throw new Error('Failed to get RPC from network config file') + } + const rpcUrl: string = networkConfig.rpcUrl + if (!rpcUrl.startsWith('http')) { + throw new Error('Invalid RPC URL format') + } + try { const facetsResult = await $`cast call ${diamondAddress} "facets() returns ((address,bytes4[])[])" --rpc-url ${rpcUrl}` // ... rest of the code + } catch (error) { + if (error.message.includes('connection refused')) { + throw new Error(`Failed to connect to RPC endpoint: ${rpcUrl}`) + } + throw error + }
204-205: Enhance error logging for better debugging.The current error logging could be more informative. Consider adding error type classification and stack traces for better debugging.
- consola.warn('Unable to parse output - skipping facet registration check') - consola.warn('Error:', error) + consola.warn('Unable to parse output - skipping facet registration check', { + error: error instanceof Error ? error.message : String(error), + type: error.constructor.name, + stack: error instanceof Error ? error.stack : undefined + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/deploy/healthCheck.ts(5 hunks)
⏰ 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)
script/deploy/healthCheck.ts (1)
551-553: LGTM: Edge case handling for no DEXs.The addition of
finish()call when no DEXs are configured is a good improvement for proper script termination.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
script/deploy/healthCheck.ts (3)
203-206: Enhance error logging for facet registration check.The current error handling logs a generic warning. Consider logging specific error details to help diagnose issues.
} catch (error) { - consola.warn('Unable to parse output - skipping facet registration check') - consola.warn('Error:', error) + consola.warn('Failed to check facet registration:', { + error: error instanceof Error ? error.message : String(error), + network, + diamondAddress + }) }🧰 Tools
🪛 GitHub Actions: SC Core Dev Approval Check
[error] 204-204: Unable to parse louper output - skipping facet registration check.
435-456: Enhance error handling for workflow file check.The current implementation might throw on file read errors. Consider adding more specific error handling and validation.
try { const fileContent: string = fs.readFileSync(filePath, 'utf8') + if (!fileContent.trim()) { + throw new Error('Workflow file is empty') + } const networkUpper: string = network.toUpperCase() const pattern = new RegExp( `ETH_NODE_URI_${networkUpper}\\s*:\\s*\\$\\{\\{\\s*secrets\\.ETH_NODE_URI_${networkUpper}\\s*\\}\\}` ) const exists: boolean = pattern.test(fileContent) if (!exists) { logError(`Missing ETH_NODE_URI config for ${network} in ${filePath}`) } } catch (error: any) { - logError(`Error checking workflow file: ${error.message}`) + if (error.code === 'ENOENT') { + logError(`Workflow file not found: ${filePath}`) + } else { + logError(`Error checking workflow file: ${error.message}`) + } }
336-342: Consider using built-in array methods for chunking.The custom
chunkArrayfunction could be replaced with a more concise implementation using array methods.-const chunkArray = <T>(array: T[], chunkSize: number): T[][] => { - const chunks: T[][] = [] - for (let i = 0; i < array.length; i += chunkSize) { - chunks.push(array.slice(i, i + chunkSize)) - } - return chunks -} +const chunkArray = <T>(array: T[], chunkSize: number): T[][] => + Array.from({ length: Math.ceil(array.length / chunkSize) }, (_, i) => + array.slice(i * chunkSize, (i + 1) * chunkSize) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/deploy/healthCheck.ts(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: SC Core Dev Approval Check
script/deploy/healthCheck.ts
[error] 51-51: Duplicate declaration of 'networks'. Consider deleting or renaming it.
[error] 204-204: Unable to parse louper output - skipping facet registration check.
[error] 1-1: The following facets exist on-chain but are missing in the config.
[error] 2-2: The following facets exist in the config but are not deployed on-chain.
[error] 4-4: Facet DiamondCutFacet not registered in Diamond or possibly unverified.
[error] 5-5: No DEXs configured.
[error] 6-6: 4 Errors found in deployment.
⏰ 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 (4)
script/deploy/healthCheck.ts (4)
51-51: Remove duplicate import ofnetworks.The
networksobject is already imported throughviemScriptHelpersat line 21.🧰 Tools
🪛 GitHub Actions: SC Core Dev Approval Check
[error] 51-51: Duplicate declaration of 'networks'. Consider deleting or renaming it.
146-202: LGTM! Improved facet registration check.The code now uses direct on-chain calls via
castinstead of the unreliable Louper CLI, making it more robust for CI/CD environments. The implementation includes proper error handling and detailed logging of discrepancies between on-chain and configured facets.
551-553: LGTM! Fixed process exit handling.The code now correctly calls
finish()when no DEXs are configured, ensuring proper script termination.
611-619: LGTM! Corrected process exit codes.The
finish()function now uses the correct exit codes:1for errors and0for success, following standard Unix conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
script/deploy/healthCheck.ts (1)
141-203: Enhance error handling for RPC URL validation.The RPC URL validation and facet inspection implementation has a few areas for improvement:
- The error message for missing RPC URL could be more descriptive
- The JSON parsing could benefit from additional error handling
Consider applying these improvements:
- if (networksConfig[network.toLowerCase()].rpcUrl) { + const networkConfig = networksConfig[network.toLowerCase()] + if (!networkConfig) { + throw new Error(`Network "${network}" not found in config`) + } + if (networkConfig.rpcUrl) { const rpcUrl: string = networksConfig[network.toLowerCase()].rpcUrl const facetsResult = await $`cast call ${diamondAddress} "facets() returns ((address,bytes4[])[])" --rpc-url ${rpcUrl}` const rawString = facetsResult.stdout const jsonCompatibleString = rawString .replace(/\(/g, '[') .replace(/\)/g, ']') .replace(/0x[0-9a-fA-F]+/g, '"$&"') + try { const onChainFacets = JSON.parse(jsonCompatibleString) + } catch (error) { + throw new Error(`Failed to parse facets result: ${error.message}`) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/deploy/healthCheck.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: generate-tag
🔇 Additional comments (3)
script/deploy/healthCheck.ts (3)
3-3: LGTM: Import ofzxfor shell command execution.The
zxlibrary is appropriately imported for executing shell commands, which aligns with the PR's objective of using thecastcommand for on-chain facet inspection.
547-550: LGTM: Proper error handling for missing DEXs configuration.The code now correctly handles the case when no DEXs are configured by calling
finish()after logging the error.
608-616: LGTM: Fixed process exit codes.The
finish()function now uses the correct exit codes:
exit(1)for errors (non-zero exit code indicates failure)exit(0)for success (zero exit code indicates success)
Which Jira task belongs to this PR?
LF-12061
Why did I implement it this way?
Switched health check facet retrieval from the unreliable Louper CLI to a direct on-chain call because Louper CLI doesn’t work in a non-interactive environment like GitHub Actions. Added process.exit codes: exit(1) on errors (failing the GitHub Action) and exit(0) if no errors. Added new logs: logs an error if facets exist on-chain but are missing from the config and
logs an error if facets exist in the config but are not deployed on-chain.
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)