LF-11929 - Moved core facets to global.json and made adjustments #980
Conversation
WalkthroughThis pull request updates several configuration and deployment scripts to shift from using a hardcoded list of core facets to retrieving them dynamically from a centralized JSON file. A new key, "coreFacets," is added in the global configuration, and related scripts are modified to use a new helper function ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 (2)
script/deploy/deployCoreFacets.sh (1)
34-34: Use read -a for safer array handling.The current array assignment could be unsafe if the output contains spaces or special characters.
Consider using this safer approach:
- local FACETS_ARRAY=($(getCoreFacetsArray)) + local FACETS_ARRAY=() + while IFS= read -r line; do + FACETS_ARRAY+=("$line") + done < <(getCoreFacetsArray)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 34-34: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/helperFunctions.sh (1)
1981-2009: Dynamic Retrieval of Core Facets via Global JSON
The new function getCoreFacetsArray correctly checks that GLOBAL_FILE_PATH is set and that the global configuration file exists before using jq to extract the coreFacets array. It also verifies that the extracted array is not empty. For enhanced robustness—and in line with ShellCheck’s recommendation (SC2207)—consider using readarray (or mapfile) instead of a command substitution, which is prone to word splitting issues. Additionally, using 'return 1' rather than 'exit 1' may be preferable if you want the calling script to handle errors gracefully rather than terminating the entire process.Suggested refactor for the array assignment:
- ARRAY=($(jq -r '.coreFacets[]' "$FILE")) + readarray -t ARRAY < <(jq -r '.coreFacets[]' "$FILE")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 1999-1999: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config/global.json(1 hunks)script/config.example.sh(1 hunks)script/deploy/deployAllContracts.sh(1 hunks)script/deploy/deployCoreFacets.sh(2 hunks)script/deploy/healthCheck.ts(1 hunks)script/helperFunctions.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
script/config.example.sh
[warning] 71-71: GLOBAL_FILE_PATH appears unused. Verify use (or export if used externally).
(SC2034)
script/deploy/deployCoreFacets.sh
[warning] 34-34: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/helperFunctions.sh
[warning] 1999-1999: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 2092-2092: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 2452-2452: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/deploy/deployAllContracts.sh
[warning] 86-86: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (4)
script/deploy/healthCheck.ts (1)
25-25: LGTM! Import of coreFacets from global.json.The change aligns with the PR objective of centralizing core facets configuration.
config/global.json (1)
52-63: Verify completeness of core facets list.The list appears comprehensive but please verify that all required core facets are included, especially considering:
- The removed
CORE_FACETSvariable from config.example.sh includedLIFuelFacetwhich is not present in this list.- The relationship with
autoWhitelistPeripheryContractsabove.script/config.example.sh (1)
70-71: LGTM! Well-defined path to global configuration.The path is correctly defined relative to the script's location using
dirname "$0".🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 71-71: GLOBAL_FILE_PATH appears unused. Verify use (or export if used externally).
(SC2034)
script/deploy/deployAllContracts.sh (1)
86-86: Core Facets Regex Construction
The new code uses getCoreFacetsArray to dynamically generate the regex for excluded facets. However, piping its output immediately through xargs and tr in one line can mask errors from getCoreFacetsArray and makes debugging slightly harder. Splitting the assignment into separate steps would improve readability and error handling.Suggested refactor:
- local EXCLUDED_FACETS_REGEXP="^($(getCoreFacetsArray | xargs | tr ' ' '|'))$" + local core_facets + core_facets=$(getCoreFacetsArray | xargs) + local EXCLUDED_FACETS_REGEXP="^($(echo "$core_facets" | tr ' ' '|'))$"✅ Verification successful
Core Facets Regex Construction Refactoring — Verified Improvement
The current implementation works correctly, but the review comment’s suggestion to split the assignment is a good-to-have improvement. Separating the steps:
• Improves readability
• Makes it easier to check and handle potential errors from getCoreFacetsArrayAdopting the suggested refactor is recommended for better debugging and clarity while preserving the functionality.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 86-86: Declare and assign separately to avoid masking return values.
(SC2155)
Test Coverage ReportLine Coverage: 78.66% (2238 / 2845 lines) |
mirooon
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
script/deploy/deployCoreFacets.sh (1)
34-35: Enhanced Core Facets Retrieval with Error HandlingThe new code block retrieves the list of core facets dynamically via
getCoreFacetsArrayand immediately checks the exit status. This improves maintainability by centralizing the logic in one function.
However, as noted by Shellcheck (SC2207), splitting command output this way may lead to unexpected word splitting if any facet name contains whitespace. Consider usingreadarray(ormapfile) to robustly handle line-delimited outputs. For example:- FACETS_ARRAY=($(getCoreFacetsArray)) + readarray -t FACETS_ARRAY < <(getCoreFacetsArray)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 34-34: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/deploy/deployAllContracts.sh (1)
86-92: Robust Regex Construction for Excluding Core FacetsThis segment correctly retrieves the core facets via
getCoreFacetsArrayand processes them to build a regex used for excluding core facets in subsequent deployment steps. The error check (checkFailure) right after retrieving the output is a good addition.However, in line 92, you are performing command substitution directly within the variable declaration. This can mask a potential error from the command substitution (as highlighted by Shellcheck SC2155). I suggest splitting this into two distinct steps—first capturing the transformed facets into an intermediate variable, then constructing the regex. For example:
- local EXCLUDED_FACETS_REGEXP="^($(echo "$CORE_FACETS_OUTPUT" | xargs | tr ' ' '|'))$" + local facets_regex + facets_regex=$(echo "$CORE_FACETS_OUTPUT" | xargs | tr ' ' '|') + local EXCLUDED_FACETS_REGEXP="^(${facets_regex})$"This refactor not only improves clarity but also prevents inadvertently masking any error codes from the transformation command.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 92-92: Declare and assign separately to avoid masking return values.
(SC2155)
script/helperFunctions.sh (2)
1981-2005: FunctiongetCoreFacetsArray: Centralizing core facets retrieval
This new function correctly enforces that theGLOBAL_FILE_PATHvariable is set and points to an existing file before attempting to read the core facets using jq. The error handling is clear and the function returns a non-zero status when issues occur.One minor suggestion: when reading the JSON output into an array (line 1998), using a construct like
mapfile -t ARRAY < <(jq -r '.coreFacets[]' "$GLOBAL_FILE_PATH")could be more robust against word-splitting issues—especially if any facet names might contain spaces. This is suggested by the static analysis hint (SC2207), though if you’re confident in the format of the JSON output this may be optional.
[static_analysis]🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 1998-1998: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
2088-2122: Enhancement ingetIncludedAndSortedFacetContractsArray: Integrating core facets sorting
The changes that invokegetCoreFacetsArrayto obtain the core facets and then segregate facet contracts into “core” and “non-core” groups are a well‑thought step toward centralizing facet management. The subsequent sorting and merging of arrays is straightforward and improves maintainability by ensuring that any changes to core facets only need to be made in one place.A similar suggestion applies here: when assigning the output of
getCoreFacetsArrayto theCORE_FACETS_ARRAYvariable (line 2089), you might consider usingmapfile -t CORE_FACETS_ARRAY < <(getCoreFacetsArray)to avoid potential word-splitting issues.
[static_analysis]🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 2089-2089: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 2100-2100: Quote the right-hand side of == in [[ ]] to prevent glob matching.
(SC2053)
[warning] 2114-2114: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 2115-2115: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/global.json(1 hunks)script/deploy/deployAllContracts.sh(1 hunks)script/deploy/deployCoreFacets.sh(2 hunks)script/helperFunctions.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/global.json
🧰 Additional context used
🪛 Shellcheck (0.10.0)
script/deploy/deployCoreFacets.sh
[warning] 34-34: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/helperFunctions.sh
[warning] 1998-1998: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 2089-2089: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 2450-2450: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/deploy/deployAllContracts.sh
[warning] 92-92: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
script/deploy/deployAllContracts.sh (1)
89-89: Refactor Suggestion: Separate Declaration to Address Shellcheck Warning (SC2155)Declaring and assigning
EXCLUDED_FACETS_REGEXPin a single line can mask the return value of the embedded command. To adhere to best practices and resolve the Shellcheck warning, consider splitting the declaration and assignment into separate steps. For example:- local EXCLUDED_FACETS_REGEXP="^($(echo "$CORE_FACETS_OUTPUT" | xargs | tr ' ' '|'))$" + local EXCLUDED_FACETS_REGEXP + EXCLUDED_FACETS_REGEXP="^($(echo "$CORE_FACETS_OUTPUT" | xargs | tr ' ' '|'))$"This refactor enhances clarity around error propagation and maintains the intended behavior.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 89-89: Declare and assign separately to avoid masking return values.
(SC2155)
script/helperFunctions.sh (3)
1981-2011: New Function:getCoreFacetsArray()ImplementationThe new function is clear in intent: it validates that the global configuration file is set and exists, then reads the
coreFacetsarray from it. The error checks are in place and the function prints the array entries correctly.Suggestion:
Some static analysis tools (e.g. ShellCheck SC2207) recommend usingmapfileorreadarrayrather than command substitution for building arrays in order to avoid word splitting issues. In the current implementation, if any core facet contains whitespace it might split unexpectedly. Consider updating the array assignment like this:- ARRAY=($(jq -r '.coreFacets[]' "$GLOBAL_FILE_PATH")) + readarray -t ARRAY < <(jq -r '.coreFacets[]' "$GLOBAL_FILE_PATH")This change improves robustness while reading values from JSON.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 1998-1998: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
2095-2097: Capturing the Core Facets Array in Sorted Array FunctionWithin the
getIncludedAndSortedFacetContractsArray()function the core facets are retrieved using:CORE_FACETS_ARRAY=($(getCoreFacetsArray)) checkFailure $? "retrieve core facets array from global.json"Suggestion:
Again, to stay consistent with best practices and avoid potential splitting issues, it is recommended to usereadarray -tinstead of a plain command substitution. For example:- CORE_FACETS_ARRAY=($(getCoreFacetsArray)) + readarray -t CORE_FACETS_ARRAY < <(getCoreFacetsArray)This refactor will make the code more resistant to unexpected word splitting especially if facet names ever contain spaces.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 2095-2095: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
2456-2458: Usage indoesDiamondHaveCoreFacetsRegistered()Here the function is used again to load a list of core facets:
# get list of all core facet contracts from global.json FACETS_NAMES=($(getCoreFacetsArray)) checkFailure $? "retrieve core facets array from global.json"Suggestion:
For consistency and reliability in array assignments, it is advisable to update this similarly usingreadarray:- FACETS_NAMES=($(getCoreFacetsArray)) + readarray -t FACETS_NAMES < <(getCoreFacetsArray)This small change (suggested by static analysis hints) helps prevent issues related to word splitting and ensures that each facet name is captured as a distinct element.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 2456-2456: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
script/deploy/deployAllContracts.sh(1 hunks)script/helperFunctions.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
script/helperFunctions.sh
[warning] 1998-1998: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 2095-2095: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 2456-2456: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/deploy/deployAllContracts.sh
[warning] 89-89: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: generate-tag
🔇 Additional comments (1)
script/deploy/deployAllContracts.sh (1)
86-87: Effective Retrieval and Error Handling for Core FacetsThe dynamic retrieval of core facets using the
getCoreFacetsArrayfunction is a great move toward centralizing configuration. The immediate error check withcheckFailure $? "retrieve core facets array from global.json"ensures that any failure in loading the global configuration is caught early, which improves robustness.
Which Jira task belongs to this PR?
LF-11929
Why did I implement it this way?
Instead of maintaining duplicate arrays of core facets in both the health check and bash script we decided to keep them in a single source of truth within
global.json.Additionally, the GLOBAL_FILE_PATH variable should be defined in config.sh for scripts, with the value:
GLOBAL_FILE_PATH="$(dirname "$0")/../config/global.json"Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)