feat(ci): carve out storage layout CI #127
Conversation
Since the storage layout CI needs the API keys to run, it does not work in PRs originating from forked repos. To avoid those unnecessary errors, this PR splits the workflow into two parts: the first that generates the compiled layouts and the second that fetches the deployed layouts and compares them. Such a split is possible because the second type of workflow runs in a trusted context, and hence, the API key is accessible to it.
Since the storage layout CI needs the API keys to run, it does not work in PRs originating from forked repos. To avoid those unnecessary errors, this PR splits the workflow into two parts: the first that generates the compiled layouts and the second that fetches the deployed layouts and compares them. Such a split is possible because the second type of workflow runs in a trusted context, and hence, the API key is accessible to it.
WalkthroughThis pull request introduces a new GitHub Actions workflow for comparing storage layouts of smart contracts, triggered by the completion of the "Forge CI" workflow. It includes jobs for setting commit statuses, environment setup, and layout comparison. Several existing scripts related to layout comparison have been deleted, and a new script has been added to automate the comparison of deployed and compiled JSON files. Additionally, modifications have been made to existing workflows for clarity and efficiency, including renaming jobs and updating Node.js versions. Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
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: 8
🧹 Outside diff range and nitpick comments (15)
.github/workflows/lint.yml (2)
Line range hint
19-34: Update action versions and improve workflow efficiencySeveral improvements can be made to enhance stability and performance:
- The
actions/setup-nodeaction is outdated (v2). Update to the latest version.- Node.js 22 is currently unstable. Consider using LTS version 20.
- Add npm dependency caching to improve workflow speed.
Apply these changes:
- uses: actions/checkout@v4 - name: Set up Node.js - uses: actions/setup-node@v2 + uses: actions/setup-node@v4 with: - node-version: '22' + node-version: '20' + cache: 'npm' - name: Clear npm cache run: npm cache clean --forceNote: With caching enabled, you might want to remove the "Clear npm cache" step unless it's specifically needed for troubleshooting.
🧰 Tools
🪛 actionlint (1.7.4)
23-23: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
Line range hint
1-34: Consider workflow ordering in the CI pipelineSince this PR introduces new storage layout comparison workflows, consider adding this lint workflow as a required check before running the storage layout comparisons. This ensures code quality before performing more complex operations.
You can achieve this by:
- Adding this workflow as a required status check in branch protection rules
- Making the storage layout workflow depend on this one using
needsin the workflow configuration🧰 Tools
🪛 actionlint (1.7.4)
23-23: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
script/compareLayouts.js (4)
33-33: Document the usage of unsafeAllowCustomTypes optionThe
unsafeAllowCustomTypes: trueoption might have security implications. Please document why this option is necessary and what types of custom storage layouts are expected.
29-30: Add JSON structure validation before comparisonThe script parses JSON but doesn't validate if the files have the expected structure for storage layout comparison. This could lead to unclear errors if the JSON structure is invalid.
- const deployedData = JSON.parse(fs.readFileSync(before, 'utf8')); - const compiledData = JSON.parse(fs.readFileSync(after, 'utf8')); + const deployedData = JSON.parse(fs.readFileSync(before, 'utf8')); + const compiledData = JSON.parse(fs.readFileSync(after, 'utf8')); + + // Validate expected structure + if (!deployedData.storage || !compiledData.storage) { + throw new Error('Invalid storage layout format: missing storage field'); + }
43-46: Enhance error handling with specific error typesThe current error handling catches all errors generically. Consider handling specific error types (e.g., FileNotFoundError, JSONParseError) differently to provide more helpful error messages.
- } catch (error) { - console.error(`❌ Error processing ${before} or ${after}: ${error.message}`); - process.exitCode = 1; + } catch (error) { + if (error instanceof SyntaxError) { + console.error(`❌ Invalid JSON in ${before} or ${after}: ${error.message}`); + } else if (error.code === 'ENOENT') { + console.error(`❌ File not found: ${error.path}`); + } else { + console.error(`❌ Error processing ${before} or ${after}: ${error.message}`); + } + process.exitCode = 1;
16-47: Consider parallel processing for better performanceThe script processes comparisons sequentially. For better performance, especially in CI environments, consider using Promise.all to process comparisons concurrently.
-fileMappings.forEach(({ before, after }) => { +Promise.all(fileMappings.map(async ({ before, after }) => { console.log(`🔍 Comparing ${before} and ${after}...`); try { // ... existing comparison logic ... } catch (error) { // ... error handling ... } -}); +})).then(() => { + if (process.exitCode === 1) { + console.log('❌ One or more comparisons failed'); + } else { + console.log('✅ All comparisons completed successfully'); + } +});.github/workflows/status-comment.yml (2)
9-11: Consider alternative approach for Compare Storage Layout status updatesThe comment correctly notes that nested
workflow_runtriggers aren't supported. However, this means we won't get status updates for the Compare Storage Layout workflow in the same format as other workflows. Consider documenting this limitation in the PR description or implementing a consistent status update mechanism across all workflows.
51-72: Enhance message construction robustnessThe message construction could be more robust with input validation and proper shell quoting.
run: | + # Validate required environment variables + : "${WORKFLOW_NAME:?Required environment variable not set}" + : "${WORKFLOW_URL:?Required environment variable not set}" + : "${SHA:?Required environment variable not set}" + if [ "${{ github.event.action }}" == "requested" ]; then message="🚀 The $WORKFLOW_NAME workflow has started." elif [ "${{ github.event.workflow_run.conclusion }}" == "success" ]; then message="✅ The $WORKFLOW_NAME workflow has completed successfully." elif [ "${{ github.event.workflow_run.conclusion }}" == "failure" ]; then message="❌ The $WORKFLOW_NAME workflow has failed!" elif [ "${{ github.event.workflow_run.conclusion }}" == "cancelled" ]; then message="⏹️ The $WORKFLOW_NAME workflow was cancelled." else message="❓ The $WORKFLOW_NAME workflow has completed with an unknown status." fi - echo "message=$message Check the [workflow run]($WORKFLOW_URL) for details. ($SHA)" >> "${GITHUB_OUTPUT}" + # Properly escape the message for GitHub Actions output + message="${message} Check the [workflow run](${WORKFLOW_URL}) for details. (${SHA})" + message="${message//'%'/'%25'}" + message="${message//$'\n'/'%0A'}" + message="${message//$'\r'/'%0D'}" + echo "message=${message}" >> "${GITHUB_OUTPUT}".github/workflows/forge-ci.yml (4)
Line range hint
22-55: Consider enhancing the caching strategyTwo suggestions to improve the caching mechanism:
- Add
restore-keysfallback to the Foundry toolchain cache restore step for better cache hits- Include a hash of relevant files (e.g., forge config, dependencies) in the build artifacts cache key for more precise caching
uses: actions/cache/restore@v3 with: path: ${{ needs.setup.outputs.installation-dir }} key: ${{ needs.setup.outputs.cache-key }} + restore-keys: | + foundry- uses: actions/cache/save@v3 with: path: | ./lib ./out ./cache ./broadcast - key: build-${{ github.event.pull_request.head.sha || github.event.after || github.sha }} + key: build-${{ hashFiles('foundry.toml', 'lib/**/*.json') }}-${{ github.event.pull_request.head.sha || github.event.after || github.sha }}🧰 Tools
🪛 actionlint (1.7.4)
40-40: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
57-82: Consider reducing test verbosityThe
-vvvflag generates very verbose output which might make it harder to identify important information in CI logs. Consider using-vor-vvunless the extra verbosity is specifically needed.- run: forge test -vvv + run: forge test -vv🧰 Tools
🪛 actionlint (1.7.4)
67-67: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting
(shellcheck)
160-162: Consider making the file list dynamicThe hardcoded list of files to generate layouts for could become outdated as new contracts are added or removed. Consider making this list dynamic by finding all contract files in the
src/coredirectory.- for file in Bootstrap ClientChainGateway RewardVault Vault ExocoreGateway ExoCapsule; do - forge inspect src/core/${file}.sol:${file} storage-layout > ${file}.compiled.json; - done + # Find all .sol files in src/core and generate layouts + for filepath in src/core/*.sol; do + filename=$(basename "$filepath" .sol) + if grep -q "^contract ${filename}" "$filepath"; then + forge inspect "$filepath:${filename}" storage-layout > "${filename}.compiled.json" + fi + done
133-135: Simplify base branch layout file handlingThe current approach of copying files back and forth between /tmp seems unnecessary. Consider simplifying this process.
- run: | - forge inspect src/core/ExocoreGateway.sol:ExocoreGateway storage-layout > ExocoreGateway.base.json - - name: Copy base branch layout file - run: cp ExocoreGateway.base.json /tmp/ + run: | + forge inspect src/core/ExocoreGateway.sol:ExocoreGateway storage-layout > ExocoreGateway.base.json.tmp + mv ExocoreGateway.base.json.tmp ExocoreGateway.base.json - - name: Copy back base branch layout file - run: cp /tmp/ExocoreGateway.base.json ./ExocoreGateway.base.jsonAlso applies to: 163-164
package.json (1)
12-12: Consider adding npm scripts for layout comparison workflow.To streamline the CI process and make it easier to run locally, consider adding dedicated npm scripts for the storage layout comparison functionality.
Add these scripts to the
scriptssection:"scripts": { "test": "mocha", + "compile-layouts": "YOUR_COMPILE_COMMAND", + "compare-layouts": "node script/compareLayouts.js" },.github/workflows/compare-layouts.yml (2)
17-73: Consider extracting status context as environment variable.The status context "Compare Storage Layouts" is used in multiple places. Consider extracting it to an environment variable to maintain consistency and ease updates.
jobs: + env: + STATUS_CONTEXT: "Compare Storage Layouts" set-commit-status: # ... existing code ... -d '{ "state": "pending", - "context": "Compare Storage Layouts", + "context": "${{ env.STATUS_CONTEXT }}",
185-197: Simplify conditional logic for better readability.The current shell logic using
&&and||is hard to read and potentially error-prone. Consider using a more explicit if-then-else structure.- outcome=$( - [[ "${{ steps.compare-layouts.outcome }}" == "success" ]] && - echo "success" || - echo "failure" - ) - description=$( - [[ "${{ steps.compare-layouts.outcome }}" == "success" ]] && - echo "Storage layouts match" || - ([[ "${{ steps.compare-layouts.outcome }}" == "failure" ]] && - echo "Storage layouts do not match" || - echo "Job skipped since ${{ github.event.workflow_run.name }} failed.") - ) + if [[ "${{ steps.compare-layouts.outcome }}" == "success" ]]; then + outcome="success" + description="Storage layouts match" + elif [[ "${{ steps.compare-layouts.outcome }}" == "failure" ]]; then + outcome="failure" + description="Storage layouts do not match" + else + outcome="failure" + description="Job skipped since ${{ github.event.workflow_run.name }} failed." + fi🧰 Tools
🪛 actionlint (1.7.4)
185-185: shellcheck reported issue in this script: SC2015:info:7:61: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.github/workflows/compare-layouts.yml(1 hunks).github/workflows/compare_deployed_storage_layout.py(0 hunks).github/workflows/compare_storage_layout.py(0 hunks).github/workflows/forge-ci.yml(5 hunks).github/workflows/lint.yml(2 hunks).github/workflows/reusable-foundry-setup.yml(2 hunks).github/workflows/status-comment.yml(1 hunks)package.json(1 hunks)script/compareLayouts.js(1 hunks)src/storage/ClientChainGatewayStorage.sol(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/compare_deployed_storage_layout.py
- .github/workflows/compare_storage_layout.py
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/compare-layouts.yml
120-120: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting
(shellcheck)
166-166: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
185-185: shellcheck reported issue in this script: SC2015:info:7:61: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
.github/workflows/forge-ci.yml
40-40: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting
(shellcheck)
67-67: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting
(shellcheck)
82-82: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
94-94: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting
(shellcheck)
119-119: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting
(shellcheck)
166-166: shellcheck reported issue in this script: SC2035:info:1:25: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
.github/workflows/lint.yml
23-23: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/status-comment.yml
[error] 80-80: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (14)
.github/workflows/lint.yml (2)
Line range hint 1-13: LGTM! Well-configured workflow triggers
The workflow triggers are comprehensive, covering all necessary events (PRs, merges, and specific branch/tag pushes).
14-18: LGTM! Clear job naming and appropriate strategy
The job name "lint" clearly describes its purpose, and the fail-fast strategy is appropriate for linting tasks.
.github/workflows/reusable-foundry-setup.yml (3)
13-13: LGTM: Output references correctly updated
The output references have been properly updated to reflect the job rename from 'setup' to 'install', maintaining workflow consistency.
Also applies to: 16-16
Line range hint 19-24: LGTM: Job name change improves clarity
Renaming from 'setup' to 'install' better reflects the job's primary purpose of installing the Foundry toolchain. The outputs remain properly configured.
43-43: Verify the impact of removing OS-specific cache keys
The removal of ${{ runner.os }} from cache keys means all operating systems will share the same cache key. Since Foundry binaries are typically OS-specific, this could potentially cause issues.
Let's verify if this workflow is only used with Ubuntu runners:
If this workflow is used with multiple OS types, consider keeping the OS-specific cache keys to prevent potential issues with binary compatibility.
Also applies to: 47-47
✅ Verification successful
Cache key simplification is safe - workflow only runs on Ubuntu
The verification shows that:
- The reusable workflow is only used by
forge-ci.ymlandcompare-layouts.yml - All workflows in the repository, including the reusable workflow itself and its callers, exclusively use
ubuntu-latestrunners
Since the workflow and its dependencies are strictly Ubuntu-based, removing the OS-specific cache prefix is safe and won't cause any binary compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the workflow is only used with Ubuntu runners
# Expected: Only find references to ubuntu-latest as the runner
# Search for workflow files that reference this reusable workflow
echo "Searching for workflows using reusable-foundry-setup.yml..."
rg -l "uses: .*workflows/reusable-foundry-setup.yml" .github/workflows/
# Search for different runner types in those files
echo "Checking runner types in those workflows..."
rg "runs-on:" .github/workflows/ -A 1
Length of output: 1509
src/storage/ClientChainGatewayStorage.sol (2)
44-44: Ensure storage layout compatibility through new CI workflow
Since this PR introduces a new storage layout comparison workflow, please ensure that:
- This change passes the new storage layout comparison checks
- The reduction in storage gap doesn't create incompatibilities with deployed contracts
Let's verify the existence and configuration of the new workflow:
44-44: Verify the storage gap reduction and its implications
The storage gap has been reduced from 40 to 39 slots. While this might be intentional to accommodate new state variables, reducing the gap without clear documentation could limit future upgradability.
Let's verify the storage layout changes and their documentation:
Consider:
- Adding inline documentation explaining why the gap was reduced
- Updating storage layout documentation if it exists
- Ensuring this change is compatible with any deployed instances of this contract
.github/workflows/status-comment.yml (1)
16-19: LGTM! Well-scoped permissions
The permissions are appropriately minimal, following security best practices by only granting necessary access.
.github/workflows/forge-ci.yml (3)
Line range hint 1-11: LGTM! Well-structured workflow triggers
The workflow triggers are properly configured to run on necessary events while maintaining efficiency by limiting push events to relevant branches and tags.
15-21: LGTM! Good use of reusable workflow and version management
Excellent practice using a reusable workflow and maintaining foundry-version as a single source of truth.
Line range hint 84-107: LGTM! Consistent formatting check implementation
The format job follows the established pattern and correctly implements the formatting check.
🧰 Tools
🪛 actionlint (1.7.4)
82-82: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
94-94: shellcheck reported issue in this script: SC2086:info:1:55: Double quote to prevent globbing and word splitting
(shellcheck)
package.json (1)
12-12: Verify OpenZeppelin package version security.
The addition of @openzeppelin/upgrades-core is appropriate for the storage layout comparison functionality. Let's verify the version for known vulnerabilities.
✅ Verification successful
@openzeppelin/upgrades-core v1.40.0 is secure and up-to-date
The version 1.40.0 is very close to the latest version (1.41.0) and there are no known security vulnerabilities reported for this package. The latest version was published recently (November 25, 2024), indicating active maintenance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for known vulnerabilities in @openzeppelin/upgrades-core
# Get latest version and release date
echo "Checking latest version information..."
curl -s https://registry.npmjs.org/@openzeppelin/upgrades-core | jq -r '.["dist-tags"].latest, .time.modified'
# Check for security advisories
echo "Checking for security advisories..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "@openzeppelin/upgrades-core") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 673
.github/workflows/compare-layouts.yml (2)
1-13: LGTM! Well-configured workflow trigger and permissions.
The workflow trigger and permissions are properly configured following security best practices with the principle of least privilege.
74-82: Consider using a stable version instead of nightly.
Using the "nightly" version in a production workflow could lead to unexpected behavior if breaking changes are introduced. Consider pinning to a specific stable version for better reliability.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
script/compareLayouts.js (3)
4-13: Document the special mapping between Bootstrap and ClientChainGatewayWhile most mappings compare deployed vs. compiled versions of the same contract, the last mapping compares
BootstrapwithClientChainGateway. Although this is intentional (as Bootstrap upgrades to ClientChainGateway), it would be helpful to document this special case.Add a comment explaining the special case:
const fileMappings = [ { before: 'Bootstrap.deployed.json', after: 'Bootstrap.compiled.json' }, { before: 'ClientChainGateway.deployed.json', after: 'ClientChainGateway.compiled.json' }, { before: 'Vault.deployed.json', after: 'Vault.compiled.json' }, { before: 'RewardVault.deployed.json', after: 'RewardVault.compiled.json' }, { before: 'ExoCapsule.deployed.json', after: 'ExoCapsule.compiled.json' }, { before: 'ExocoreGateway.base.json', after: 'ExocoreGateway.compiled.json' }, + // Special case: Compare Bootstrap with ClientChainGateway as Bootstrap upgrades to it { before: 'Bootstrap.compiled.json', after: 'ClientChainGateway.compiled.json' }, ];
19-27: Consider using async/await for file operationsThe synchronous file operations (
fs.existsSync,fs.readFileSync) could block the event loop. Consider using async versions for better performance.Example refactor:
- if (!fs.existsSync(before)) { - console.log(`⚠️ Skipping: ${before} does not exist.`); - return; - } - if (!fs.existsSync(after)) { - console.log(`⚠️ Skipping: ${after} does not exist.`); - return; - } + await Promise.all([ + fs.promises.access(before).catch(() => { + console.log(`⚠️ Skipping: ${before} does not exist.`); + return Promise.reject(); + }), + fs.promises.access(after).catch(() => { + console.log(`⚠️ Skipping: ${after} does not exist.`); + return Promise.reject(); + }) + ]);
15-47: Consider making file paths configurableThe script assumes JSON files are in the current directory. Consider making the base directory configurable through environment variables or command-line arguments.
Example improvement:
const baseDir = process.env.LAYOUTS_DIR || '.'; const getFilePath = (filename) => path.join(baseDir, filename); fileMappings.forEach(({ before, after }) => { const beforePath = getFilePath(before); const afterPath = getFilePath(after); // ... use these paths instead of raw filenames });.github/workflows/forge-ci.yml (3)
15-21: Consider pinning to a specific nightly versionWhile using
nightlyensures you get the latest features, it could lead to unexpected behavior if breaking changes are introduced. Consider pinning to a specific nightly version for better reproducibility.- foundry-version: nightly + foundry-version: nightly-87c4cef3f88920c42fb12956b0c291049ca8d609
55-55: Enhance build artifacts cache keyThe current cache key only uses the commit SHA. Consider adding the runner OS and Foundry version to prevent potential cache conflicts:
- key: build-${{ github.event.pull_request.head.sha || github.event.after || github.sha }} + key: build-${{ runner.os }}-foundry-${{ needs.setup.outputs.cache-key }}-${{ github.event.pull_request.head.sha || github.event.after || github.sha }}
82-82: Quote forge test argumentsTo prevent potential issues with argument handling:
- run: forge test -vvv + run: forge test "-vvv"🧰 Tools
🪛 actionlint (1.7.4)
82-82: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/compare-layouts.yml(1 hunks).github/workflows/forge-ci.yml(5 hunks).github/workflows/status-comment.yml(1 hunks)script/compareLayouts.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/status-comment.yml
🧰 Additional context used
📓 Learnings (2)
.github/workflows/compare-layouts.yml (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#127
File: .github/workflows/compare-layouts.yml:122-154
Timestamp: 2024-11-27T05:03:53.004Z
Learning: The `compareLayouts.js` script is responsible for error handling related to JSON parsing and field extraction, so additional error handling in the GitHub Actions workflow is unnecessary.
script/compareLayouts.js (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#127
File: script/compareLayouts.js:12-12
Timestamp: 2024-11-27T04:58:50.251Z
Learning: In `script/compareLayouts.js`, the mapping comparing `Bootstrap.compiled.json` to `ClientChainGateway.compiled.json` is intentional because `Bootstrap` upgrades to `ClientChainGateway`.
🪛 actionlint (1.7.4)
.github/workflows/compare-layouts.yml
189-189: shellcheck reported issue in this script: SC2015:info:7:61: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
.github/workflows/forge-ci.yml
82-82: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (9)
script/compareLayouts.js (2)
1-2:
Avoid importing from internal package paths
Importing from the package's dist directory makes the code fragile as it depends on the internal structure of the package.
Apply this diff to import from the package's main entry point:
-const { getStorageUpgradeReport } = require('@openzeppelin/upgrades-core/dist/storage');
+const { getStorageUpgradeReport } = require('@openzeppelin/upgrades-core');33-33: Verify the usage of unsafeAllowCustomTypes
The unsafeAllowCustomTypes flag is being used in the storage comparison. This flag might bypass important type safety checks.
Let's check if any custom types are used in the contracts:
✅ Verification successful
The use of unsafeAllowCustomTypes is justified in this context
The codebase contains multiple contracts with custom structs and enums, particularly in storage contracts like ExoCapsuleStorage.sol and GatewayStorage.sol. The unsafeAllowCustomTypes flag is necessary here to properly compare storage layouts that include these custom types. The script is using this flag as part of OpenZeppelin's upgrade validation process, which is a standard practice when dealing with custom types in upgradeable contracts.
- The flag is used in a controlled environment (deployment script) for storage layout comparison
- The script properly handles and reports any storage incompatibilities (
report.okcheck) - Multiple contracts in the codebase use legitimate custom types that require this flag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for custom types in contract files
rg -l "struct\s+\w+\s*{" .
Length of output: 124
Script:
#!/bin/bash
# Search for custom types in contract files with fixed regex
rg -l "struct\s+\w+" .
# Also search for custom enum definitions
rg -l "enum\s+\w+" .
# Check the context of getStorageUpgradeReport usage
rg -A 5 "getStorageUpgradeReport" .
Length of output: 1710
.github/workflows/forge-ci.yml (3)
Line range hint 1-11: LGTM! Well-structured workflow triggers
The workflow configuration properly handles all necessary events including pull requests from forks, which aligns with the PR objectives.
Line range hint 84-107: LGTM! Well-structured format job
The formatting check is properly configured and maintains consistency with other jobs.
🧰 Tools
🪛 actionlint (1.7.4)
82-82: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
166-166: Fix glob pattern in zip command
The zip command uses unquoted glob patterns which could cause issues with filenames containing special characters.
- run: zip "storage-layouts.zip" "./*.compiled.json" "./ExocoreGateway.base.json"
+ run: zip storage-layouts.zip ./*.compiled.json ./ExocoreGateway.base.json.github/workflows/compare-layouts.yml (4)
1-13: LGTM! Well-configured workflow trigger and permissions.
The workflow is properly configured with:
- Clear trigger conditions based on the "Forge CI" workflow completion
- Minimal required permissions following security best practices
40-60: LGTM! Robust PR number extraction logic.
The PR number extraction logic correctly handles:
- Cross-repository PRs
- Error cases with appropriate messaging
- Output variable setting
236-244: LGTM! Proper exit code handling.
The final exit code handling ensures that the workflow fails appropriately when the comparison fails or when the parent job fails.
78-86: Consider using a stable Foundry version.
Using the nightly version in CI could lead to unexpected failures if breaking changes are introduced. Consider pinning to a stable version unless nightly features are specifically required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
.github/workflows/forge-ci.yml (4)
15-21: Well-structured reusable workflow implementationGood practice using a reusable workflow and centralizing the Foundry version. This improves maintainability and reduces version inconsistencies across the CI pipeline.
Consider documenting the version selection criteria in the repository's contributing guidelines to help maintainers understand when and why to update the Foundry version.
46-47: Add error handling to the build stepConsider adding error handling and build status reporting:
- - name: Build - run: forge build + - name: Build + id: build + run: | + if ! forge build; then + echo "::error::Build failed" + exit 1 + fi + echo "Build completed successfully"
81-83: Consider preserving test results as artifactsWhile the test summary is helpful, consider also uploading test results as artifacts for historical comparison and debugging:
- name: Test - run: forge test -vvv + run: | + mkdir -p test-results + forge test -vvv --json > test-results/results.json - name: Set test snapshot as summary run: NO_COLOR=1 forge snapshot >> $GITHUB_STEP_SUMMARY + - name: Upload test results + uses: actions/upload-artifact@v4 + with: + name: test-results-${{ github.event.pull_request.head.sha || github.event.after || github.sha }} + path: test-results/🧰 Tools
🪛 actionlint (1.7.4)
82-82: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
109-164: Well-structured storage layout extraction processThe job effectively implements the storage layout comparison workflow:
- Proper handling of base branch and PR layouts
- Efficient use of temporary storage
- Comprehensive artifact preservation
This implementation aligns well with the PR objective of carving out storage layout CI, allowing the comparison workflow to run in a trusted context with API access.
.github/workflows/compare-layouts.yml (3)
38-56: Consider enhancing error message for PR lookup failures.The error handling for PR lookup could be more informative by including the actual API response.
- if [ -z "$pr_number" ]; then - echo "Error: PR number not found for branch '${PR_BRANCH}' in repository '${PR_TARGET_REPO}'" >&2 + pr_response=$(gh pr view --repo "${PR_TARGET_REPO}" "${PR_BRANCH}" 2>&1) + if [ $? -ne 0 ]; then + echo "Error: Failed to find PR for branch '${PR_BRANCH}' in repository '${PR_TARGET_REPO}'" >&2 + echo "API Response: ${pr_response}" >&2 exit 1 fi
122-154: Consider improving the contract processing logic.The contract processing could be enhanced for better maintainability and error handling.
- declare -A contracts=( - ["Bootstrap"]="$bootstrap" - ["ClientChainGateway"]="$clientGateway" - ["Vault"]="$vault" - ["RewardVault"]="$rewardVault" - ["ExoCapsule"]="$capsule" - ) + # Define contracts configuration in a more structured way + config_file=$(mktemp) + cat > "$config_file" << EOF + { + "Bootstrap": "$bootstrap", + "ClientChainGateway": "$clientGateway", + "Vault": "$vault", + "RewardVault": "$rewardVault", + "ExoCapsule": "$capsule" + } + EOF + + # Process contracts using jq + jq -r 'to_entries[] | select(.value != null) | [.key, .value] | @tsv' "$config_file" | + while IFS=$'\t' read -r contract address; do + echo "Processing $contract at address $address" + cast storage --json "$address" --rpc-url "$RPC_URL" \ + --etherscan-api-key "$ETHERSCAN_API_KEY" > "$contract.deployed.json" || { + echo "Failed to fetch storage for $contract" >&2 + exit 1 + } + mv "$contract.deployed.json" "$pwd" + done
213-220: Consider using a case statement for better readability.The message setting logic could be more readable using a case statement.
- if [ ${{ steps.compare-layouts.outcome }} == "success" ]; then - message="✅ The $WORKFLOW_NAME workflow has completed successfully." - elif [ ${{ steps.compare-layouts.outcome }} == "failure" ]; then - message="❌ The $WORKFLOW_NAME workflow has failed!" - else - message="⏭ The $WORKFLOW_NAME workflow was skipped." - fi + case "${{ steps.compare-layouts.outcome }}" in + "success") + message="✅ The $WORKFLOW_NAME workflow has completed successfully." + ;; + "failure") + message="❌ The $WORKFLOW_NAME workflow has failed!" + ;; + *) + message="⏭ The $WORKFLOW_NAME workflow was skipped." + ;; + esac
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/compare-layouts.yml(1 hunks).github/workflows/forge-ci.yml(5 hunks)
🧰 Additional context used
📓 Learnings (2)
.github/workflows/compare-layouts.yml (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#127
File: .github/workflows/compare-layouts.yml:122-154
Timestamp: 2024-11-27T05:03:53.004Z
Learning: The `compareLayouts.js` script is responsible for error handling related to JSON parsing and field extraction, so additional error handling in the GitHub Actions workflow is unnecessary.
.github/workflows/forge-ci.yml (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#127
File: .github/workflows/forge-ci.yml:160-162
Timestamp: 2024-11-27T05:11:26.802Z
Learning: In CI scripts, it's acceptable to assume that required files exist and commands succeed; failures should be addressed by modifying the CI in the PR.
🪛 actionlint (1.7.4)
.github/workflows/compare-layouts.yml
29-29: receiver of object dereference "name" must be type of object but got "string"
(expression)
188-188: receiver of object dereference "name" must be type of object but got "string"
(expression)
.github/workflows/forge-ci.yml
82-82: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (6)
.github/workflows/forge-ci.yml (3)
Line range hint 1-11: LGTM! Well-structured workflow triggers
The workflow triggers are properly configured with clear and specific event types and branch patterns.
107-108: LGTM! Format check is properly configured
The formatting check is appropriately set up using forge fmt --check.
159-159: Fix glob pattern in zip command
The zip command uses an unquoted glob pattern which could cause issues with filenames containing special characters.
- run: zip storage-layouts.zip ./*.compiled.json ./ExocoreGateway.base.json
+ run: zip storage-layouts.zip -- ./*.compiled.json ./ExocoreGateway.base.json.github/workflows/compare-layouts.yml (3)
1-13: LGTM! Well-configured workflow permissions.
The workflow configuration follows security best practices by:
- Using workflow_run trigger for better security with forked repositories
- Explicitly defining minimal required permissions
91-94: LGTM! Proper secret handling.
The workflow correctly uses repository secrets for API keys.
75-83: Consider using a stable Foundry version.
Using nightly builds in CI can lead to unexpected failures if breaking changes are introduced. Consider pinning to a stable version unless there's a specific need for nightly features.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/reusable-foundry-setup.yml (1)
19-22: Consider enhancing the cache-key output description.While the output references are correctly updated, the description for
cache-keycould be more specific about its format (e.g., "Cache key in format 'foundry-{version}' for Foundry toolchain").script/compareLayouts.js (2)
9-10: Track the RewardVault deployment statusThe TODO comment indicates that
RewardVault.deployed.jsonwill be required after deployment. This should be tracked to ensure the flag is updated post-deployment.Would you like me to create a GitHub issue to track this TODO?
38-38: Document the usage ofunsafeAllowCustomTypesThe
unsafeAllowCustomTypesflag is used without explanation. Consider adding a comment explaining why this flag is necessary and any potential risks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/compare-layouts.yml(1 hunks).github/workflows/reusable-foundry-setup.yml(2 hunks).github/workflows/status-comment.yml(1 hunks)script/compareLayouts.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/compare-layouts.yml
- .github/workflows/status-comment.yml
🧰 Additional context used
📓 Learnings (1)
script/compareLayouts.js (2)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#127
File: script/compareLayouts.js:43-46
Timestamp: 2024-11-27T05:09:46.690Z
Learning: In `script/compareLayouts.js`, for the CI process, the existing error logging is sufficient, and adding more granular error handling is not required.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#127
File: script/compareLayouts.js:12-12
Timestamp: 2024-11-27T04:58:50.251Z
Learning: In `script/compareLayouts.js`, the mapping comparing `Bootstrap.compiled.json` to `ClientChainGateway.compiled.json` is intentional because `Bootstrap` upgrades to `ClientChainGateway`.
🔇 Additional comments (7)
.github/workflows/reusable-foundry-setup.yml (4)
9-15: LGTM! Well-documented input parameters.
The new skip-install parameter is well-documented and serves its purpose in allowing the workflow to run without installation, which aligns with the PR's objective of supporting forked repository PRs.
25-30: LGTM! Job name better reflects its purpose.
The rename from setup to install improves clarity by better describing the job's primary function while maintaining the same output structure.
54-55: LGTM! Improved cache key format.
The simplified cache key format foundry-${{ inputs.foundry-version }} is more predictable and appropriate since the job only runs on ubuntu-latest.
Line range hint 1-63: Verify workflow integration with storage layout comparison.
Since this workflow is part of the storage layout CI restructuring, ensure it properly integrates with the new Compare Storage Layouts workflow, particularly when:
- Running with
skip-install: truein the forked PR context - Running with
skip-install: falsein the trusted context
✅ Verification successful
Integration with storage layout comparison workflow verified successfully
The workflow properly integrates with both the forge-ci and compare-layouts workflows:
- In forge-ci.yml: Used with default
skip-install: falsefor trusted context - In compare-layouts.yml: Correctly implements
skip-installbased on the parent workflow's success status - Both workflows consistently use the same Foundry version (nightly)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how this workflow is called from other workflows
rg "uses: .*reusable-foundry-setup" .github/workflows/
Length of output: 239
Script:
#!/bin/bash
# Check how the workflow is called in forge-ci.yml
rg -A 10 "uses: .*reusable-foundry-setup" .github/workflows/forge-ci.yml
# Check how the workflow is called in compare-layouts.yml
rg -A 10 "uses: .*reusable-foundry-setup" .github/workflows/compare-layouts.yml
Length of output: 1077
script/compareLayouts.js (3)
1-2: Avoid importing from the 'dist' directory
Importing directly from the dist directory is fragile as it depends on the internal structure of the package. Consider importing from the package's main entry point instead.
-const { getStorageUpgradeReport } = require('@openzeppelin/upgrades-core/dist/storage');
+const { getStorageUpgradeReport } = require('@openzeppelin/upgrades-core');5-14: LGTM! Well-structured configuration
The file mappings configuration is clear and well-organized:
- Each mapping clearly defines the files to compare
- The
mustExistflag provides flexibility for handling optional files - The mapping between Bootstrap and ClientChainGateway is intentionally included for upgrade verification
16-52: LGTM! Well-implemented comparison logic
The implementation is robust and well-suited for CI:
- Proper file existence checks with clear error messages
- Effective JSON parsing and comparison using OpenZeppelin's tools
- Clear output formatting for CI logs
- Appropriate error handling and exit code management
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
.github/workflows/slither.yml (1)
15-16: Document timeout rationale in workflow.Consider adding more context in the comment about why 10 minutes was chosen as the timeout value.
- # Takes about 2 minutes + # Default timeout set to 10 minutes (typical run: 2 minutes) + # Increased buffer accounts for larger PRs and potential CI load timeout-minutes: 10.github/workflows/lint.yml (1)
Line range hint
34-37: Consider adding error notifications.The removal of the failure comment mechanism means developers won't be notified of linting failures. Consider adding GitHub status checks or annotations for better visibility.
- name: Run Solhint run: | npx solhint 'src/**/*.sol' -c ./src/.solhint.json + if [ $? -ne 0 ]; then + echo "::error::Solhint found issues. Please check the logs above." + exit 1 + fi.github/workflows/reusable-foundry-setup.yml (1)
18-22: Improve output descriptions to indicate optional availabilityThe output descriptions should clarify that these values might not be available when
skip-installis true.outputs: installation-dir: - description: "The installation directory of Foundry toolchain" + description: "The installation directory of Foundry toolchain. Empty when skip-install is true." value: ${{ jobs.install.outputs.installation-dir }} cache-key: - description: "The cache key for Foundry toolchain" + description: "The cache key for Foundry toolchain. Empty when skip-install is true." value: ${{ jobs.install.outputs.cache-key }}.github/workflows/forge-ci.yml (4)
15-21: Good use of reusable workflow patternThe setup leverages GitHub Actions' reusable workflows feature, which promotes maintainability and consistency. The comment about
foundry-versionbeing the single source of truth is valuable documentation.Consider documenting the version selection criteria (why nightly?) in the repository's contributing guidelines or CI documentation.
48-49: Add error handling and build verificationThe build step could benefit from additional error handling and verification.
- - name: Build - run: forge build + - name: Build + run: | + forge build + # Verify build artifacts exist + if [ ! -d "out" ] || [ -z "$(ls -A out)" ]; then + echo "::error::Build failed - no artifacts generated" + exit 1 + fi
84-87: Consider preserving test results as artifactsWhile the test snapshot is added to the workflow summary, preserving detailed test results as artifacts would be valuable for debugging failures.
- name: Test - run: forge test -vvv + run: | + mkdir -p test-results + forge test -vvv --json > test-results/forge-test.json || { exit_code=$?; cp test-results/forge-test.json test-results/forge-test-failed.json; exit $exit_code; } + - name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: forge-test-results + path: test-results/🧰 Tools
🪛 actionlint (1.7.4)
86-86: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
161-163: Consider making the contract list configurableThe list of contracts is currently hardcoded in the script. Consider moving it to a configuration file for better maintainability.
+ - name: Create contract list + run: | + cat > contracts.txt << EOL + Bootstrap + ClientChainGateway + RewardVault + Vault + ExocoreGateway + ExoCapsule + EOL - name: Generate storage layout files for the PR run: | - for file in Bootstrap ClientChainGateway RewardVault Vault ExocoreGateway ExoCapsule; do + while IFS= read -r file; do forge inspect src/core/${file}.sol:${file} storage-layout > ${file}.compiled.json; - done + done < contracts.txt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/workflows/compare-layouts.yml(1 hunks).github/workflows/forge-ci.yml(5 hunks).github/workflows/lint.yml(2 hunks).github/workflows/reusable-foundry-setup.yml(2 hunks).github/workflows/slither.yml(1 hunks).github/workflows/status-comment.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/compare-layouts.yml
- .github/workflows/status-comment.yml
🧰 Additional context used
📓 Learnings (2)
.github/workflows/forge-ci.yml (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#127
File: .github/workflows/forge-ci.yml:160-162
Timestamp: 2024-11-27T05:11:26.802Z
Learning: In CI scripts, it's acceptable to assume that required files exist and commands succeed; failures should be addressed by modifying the CI in the PR.
.github/workflows/reusable-foundry-setup.yml (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#127
File: .github/workflows/reusable-foundry-setup.yml:57-63
Timestamp: 2024-11-27T06:17:09.257Z
Learning: In `reusable-foundry-setup.yml`, when `skip-install` is true, the cache key may not be set, and the input description doesn't indicate that the cache key will always be available.
🪛 actionlint (1.7.4)
.github/workflows/forge-ci.yml
86-86: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/lint.yml
25-25: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (8)
.github/workflows/lint.yml (2)
Line range hint 1-12: LGTM! Clear and well-structured workflow configuration.
The workflow triggers are comprehensive, covering all necessary events (PRs, merges, and releases).
14-20: Verify if the 5-minute timeout is sufficient.
While adding a timeout is good practice, please verify that 5 minutes is enough for your codebase size.
✅ Verification successful
5-minute timeout is more than sufficient for this linting workflow
Based on the historical runs data, the linting workflow consistently completes in less than 1 minute (around 30-40 seconds). The codebase is relatively small with Solidity files, and the workflow only runs Solhint linting. The current 5-minute timeout provides ample buffer for potential variations in execution time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check historical run times of the linting workflow to ensure 5 minutes is sufficient
# Get the last 10 workflow runs and their duration
gh run list --workflow=lint.yml --limit 10 --json durationInMinutes \
| jq -r '.[] | .durationInMinutes'
Length of output: 347
Script:
#!/bin/bash
# Let's try a different approach to check the workflow execution times
# Get the last 10 workflow runs with their start and completion times
gh run list --workflow=lint.yml --limit 10 --json startedAt,updatedAt \
| jq -r '.[] | (.startedAt + " to " + .updatedAt)'
# Also, let's check the size of the codebase to estimate potential lint time
find . -type f -name "*.js" -o -name "*.jsx" -o -name "*.ts" -o -name "*.tsx" | wc -l
# And check what linting tasks are configured
cat .github/workflows/lint.yml
Length of output: 1355
.github/workflows/reusable-foundry-setup.yml (2)
25-27: LGTM! Job configuration is well structured.
The timeout of 15 minutes provides sufficient buffer for the 3-minute caching operation.
57-58: Verify the impact of simplified cache key format
The cache key format has been simplified to exclude the runner's OS. Please verify that this doesn't cause issues with cross-platform builds.
Also applies to: 62-63
✅ Verification successful
Cache key format change is safe to proceed
The simplified cache key format is acceptable because:
- This is the only workflow file using Foundry-related caching
- The cache is version-specific through
inputs.foundry-version, which is sufficient for isolation - Foundry toolchain is platform-independent, so OS-specific caching is unnecessary
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other workflows that might depend on OS-specific cache keys
rg -l "runner.os.*foundry" .github/workflows/
Length of output: 47
Script:
#!/bin/bash
# Let's check for any cache key patterns in workflow files
rg -A 2 "key:.*foundry" .github/workflows/
# Also check for any cache restore/save actions with foundry
rg -A 2 "actions/cache@.*foundry" .github/workflows/
Length of output: 355
.github/workflows/forge-ci.yml (4)
Line range hint 1-11: LGTM! Well-structured workflow configuration
The workflow triggers and branch filters are appropriately configured for the CI pipeline.
Line range hint 89-113: LGTM! Clean formatting check implementation
The formatting check is appropriately configured with proper artifact restoration.
🧰 Tools
🪛 actionlint (1.7.4)
86-86: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
166-167: Fix glob pattern in zip command
The zip command uses an unquoted glob pattern which could cause issues with filenames containing special characters.
- - name: Zip storage layout files
- run: zip storage-layouts.zip ./*.compiled.json ./ExocoreGateway.base.json
+ - name: Zip storage layout files
+ run: zip storage-layouts.zip -- ./*.compiled.json ./ExocoreGateway.base.json168-172: LGTM! Well-structured artifact upload
The artifact upload is properly configured with a unique name based on the commit SHA.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
.github/workflows/lint.yml (1)
1-1: Consider reducing the timeout duration.The configuration looks good, but since the job usually completes in 30 seconds, consider reducing the timeout from 5 minutes to 2 minutes to fail faster in case of issues.
- timeout-minutes: 5 + timeout-minutes: 2Also applies to: 14-16
.github/workflows/forge-ci.yml (3)
Line range hint
23-57: Consider adding error handling for the build stepWhile the build configuration is good, the build step could benefit from error handling and logging.
- - name: Build - run: forge build + - name: Build + run: | + echo "Starting build process..." + if ! forge build; then + echo "::error::Build failed" + exit 1 + fi + echo "Build completed successfully"
86-87: Quote the command output redirectionThe snapshot command should have quoted redirection to handle special characters safely.
- run: NO_COLOR=1 forge snapshot >> $GITHUB_STEP_SUMMARY + run: NO_COLOR=1 forge snapshot >> "$GITHUB_STEP_SUMMARY"🧰 Tools
🪛 actionlint (1.7.4)
86-86: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
1-204: Consider adding workflow status checksBased on the PR objectives mentioning the need for status posting, consider adding status check steps at key points in the workflow.
Example implementation:
- name: Set workflow status if: always() uses: actions/github-script@v7 with: script: | const status = ${{ job.status }} === 'success' ? 'success' : 'failure'; await github.rest.repos.createCommitStatus({ owner: context.repo.owner, repo: context.repo.repo, sha: context.sha, state: status, context: 'Forge CI', description: 'Forge CI workflow completed' });.github/workflows/compare-layouts.yml (3)
61-67: Enhance error handling for PR number retrieval.While the current implementation checks if the PR number is empty, it would be beneficial to add more specific error handling for different failure scenarios.
pr_number=$(gh pr view --repo "${PR_TARGET_REPO}" "${PR_BRANCH}" \ --json 'number' --jq '.number') +if [ $? -ne 0 ]; then + echo "Error: Failed to retrieve PR information. Command exited with status $?" >&2 + exit 1 +fi if [ -z "$pr_number" ]; then echo "Error: PR number not found for branch '${PR_BRANCH}' in repository '${PR_TARGET_REPO}'" >&2 exit 1 fi
161-172: Optimize layout fetching with parallel processing.Following up on @bwhour's suggestion, we can optimize the layout fetching process using GNU parallel.
+fetch_layout() { + local name=$1 + local address=$2 + local rpc_url=$3 + local etherscan_key=$4 + + echo "Processing $name at address $address" + cast storage --json "$address" \ + --rpc-url "$rpc_url" \ + --etherscan-api-key "$etherscan_key" > "$name.deployed.json" +} + +export -f fetch_layout + echo "Processing ${{ matrix.contract.name }} at address ${{ matrix.contract.address }}" RPC_URL="https://eth-sepolia.g.alchemy.com/v2/$ALCHEMY_API_KEY" -cast storage --json "${{ matrix.contract.address }}" \ - --rpc-url "$RPC_URL" \ - --etherscan-api-key "$ETHERSCAN_API_KEY" > "${{ matrix.contract.name }}.deployed.json" +parallel --jobs 3 fetch_layout {1} {2} "$RPC_URL" "$ETHERSCAN_API_KEY" ::: \ + "${{ matrix.contract.name }}" ::: "${{ matrix.contract.address }}"
236-246: Add caching for npm dependencies.Consider caching the npm dependencies to speed up the workflow execution.
- name: Set up Node.js if: ${{ github.event.workflow_run.conclusion == 'success' }} uses: actions/setup-node@v4 with: node-version: '18' + cache: 'npm' + cache-dependency-path: 'package-lock.json' - name: Clear npm cache if: ${{ github.event.workflow_run.conclusion == 'success' }} run: npm cache clean --force
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/compare-layouts.yml(1 hunks).github/workflows/forge-ci.yml(6 hunks).github/workflows/lint.yml(2 hunks)
🧰 Additional context used
📓 Learnings (2)
.github/workflows/compare-layouts.yml (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#127
File: .github/workflows/compare-layouts.yml:122-154
Timestamp: 2024-11-27T05:03:53.004Z
Learning: The `compareLayouts.js` script is responsible for error handling related to JSON parsing and field extraction, so additional error handling in the GitHub Actions workflow is unnecessary.
.github/workflows/forge-ci.yml (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#127
File: .github/workflows/forge-ci.yml:160-162
Timestamp: 2024-11-27T05:11:26.802Z
Learning: In CI scripts, it's acceptable to assume that required files exist and commands succeed; failures should be addressed by modifying the CI in the PR.
🪛 actionlint (1.7.4)
.github/workflows/compare-layouts.yml
112-112: shellcheck reported issue in this script: SC2086:info:24:21: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/forge-ci.yml
86-86: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (4)
.github/workflows/forge-ci.yml (2)
15-21: LGTM! Well-structured setup using reusable workflow pattern.
The setup job is well-organized and documented, with clear explanations for design decisions.
199-199:
Fix zip command to handle special characters
The zip command should use proper path handling to avoid issues with special characters in filenames.
- run: zip -j compiled-layouts.zip combined/*/*.json
+ run: zip -j compiled-layouts.zip ./combined/*/*.json.github/workflows/compare-layouts.yml (2)
1-13: LGTM! Well-structured workflow configuration with proper permissions.
The workflow configuration follows security best practices by explicitly defining minimum required permissions.
179-201: LGTM! Well-structured artifact handling.
The job effectively combines the deployed layouts and uses the latest version of the download-artifact action.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/lint.yml (1)
14-16: LGTM! Good job configuration with timeout.The job name is clear, and the timeout addition is a good practice. The comment about typical duration helps justify the timeout value.
Consider adding a comment explaining why 5 minutes was chosen as the timeout value, especially since it's 10x the typical duration:
# Usually done in 30 seconds - timeout-minutes: 5 + timeout-minutes: 5 # 5min timeout to account for potential npm registry delays
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/compare-layouts.yml(1 hunks).github/workflows/lint.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/compare-layouts.yml
🔇 Additional comments (3)
.github/workflows/lint.yml (3)
1-1: LGTM! Clear and concise workflow name.
The simplified name "Solhint" effectively communicates the workflow's purpose.
25-27: Changes already addressed in previous reviews.
Line range hint 35-36: Verify the Solhint configuration file location.
The Solhint configuration file is located at ./src/.solhint.json, which is non-standard. Consider moving it to the repository root for better visibility and maintainability.
Since the storage layout CI needs the API keys to run, it does not work in PRs originating from forked repos. To avoid those unnecessary errors, this PR splits the workflow into two parts: the first that generates the compiled layouts and the second that fetches the deployed layouts and compares them.
Such a split is possible because the second type of workflow runs in a trusted context, and hence, the API key is accessible to it.
How does this CI work?
compiled.jsonlayout files.cast storage.In addition, this PR now makes it such that the status of all CIs is posted to the PR: started / successful / failed. For the Compare Storage Layouts workflow, it also features posting a status manually since Github doesn't automatically generate one for workflows triggered by
workflow_run.Do note that these workflows need to go into
mainfirst, for things to successfully work. Hence it might not be immediately possible to see their results until the PR is merged. However, they can be seen in action here: MaxMustermann2#1 and MaxMustermann2#2.The advantages of this rework are:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
ClientChainGatewayStoragecontract for optimized storage allocation.Chores