ci: unify solidity ci #314
Conversation
WalkthroughThe pull request includes significant modifications to the GitHub Actions workflows related to Solidity smart contracts. The Changes
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: 3
🧹 Outside diff range and nitpick comments (6)
dev/contracts/deploy-ephemeral (1)
8-9: Fix typo in commentThe word "dependencies" is misspelled in the comment.
-# Update depencencies +# Update dependencies.github/workflows/solidity.yml (5)
13-15: Consider making the concurrency group more specific.While the current concurrency setup works, it could be improved to be more granular. Consider including the workflow name or job name in the group to prevent blocking unrelated workflow runs.
concurrency: - group: ci-solidity-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true
40-44: Improve cache key specificity.The cache key could be more specific to improve cache hits and prevent unintended cache sharing.
with: path: contracts - key: ci-solidity-${{ github.ref }} + key: ci-solidity-${{ github.ref }}-${{ hashFiles('contracts/**/**.sol') }}
73-74: Enhance test execution and reporting.Consider adding more test options and capturing test results:
- Add fail-fast option
- Capture test results for GitHub Actions
- name: Run Forge tests - run: forge test -vvv + run: | + forge test -vvv --fail-fast > test-results.txt + cat test-results.txt + + - name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: forge-test-results + path: contracts/test-results.txt
94-95: Expand linting scope.Consider checking both source and test files for consistent formatting.
- name: Run Forge fmt - run: forge fmt contracts/src --check + run: forge fmt . --check
1-2: Add workflow status badge.Consider adding a workflow status badge to your README.md to provide visibility into the CI status.
Add this markdown to your README.md:
[](https://github.com/xmtp/xmtpd/actions/workflows/solidity.yml)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/workflows/lint.yml(1 hunks).github/workflows/solidity-slither.yml(0 hunks).github/workflows/solidity.yml(1 hunks).github/workflows/test.yml(1 hunks)dev/contracts/deploy-ephemeral(1 hunks)dev/contracts/deploy-testnet(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/solidity-slither.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/lint.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/solidity.yml
47-47: shellcheck reported issue in this script: SC2086:info:1:37: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
dev/contracts/deploy-ephemeral (1)
9-9: Verify the forge command
The forge soldeer update command seems unusual. Should this be forge update instead?
✅ Verification successful
Let me gather more information to verify if this is a typo or a custom command.
Let me check the content of the soldeer.lock file and look for any custom scripts that might define this command.
Let me check the foundry.toml configuration to understand the soldeer setup.
forge soldeer update is a valid command - it's a dependency management tool
Based on the evidence found in the codebase:
- The presence of
soldeer.lockfile with dependency specifications - Configuration in
foundry.tomlwith a[soldeer]section - Multiple deployment scripts consistently using this command
This appears to be a legitimate dependency management tool for Forge/Foundry, similar to npm's package-lock.json, used to manage Solidity dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if 'soldeer' is a valid forge subcommand
forge --help | grep -A 5 "Available Commands:"
# Check if this command exists in other files
rg "forge soldeer" -A 2 -B 2
Length of output: 1062
Script:
#!/bin/bash
# Check for any documentation or README files that might explain the tooling
fd -e md | xargs rg -i "forge|soldeer" -A 2 -B 2
# Check if there's any custom script or configuration related to soldeer
fd soldeer
Length of output: 1388
Script:
#!/bin/bash
# Check the content of soldeer.lock
cat contracts/soldeer.lock
# Look for any custom scripts or configurations that might define this command
fd -e toml -e json | xargs rg "soldeer"
Length of output: 758
Script:
#!/bin/bash
# Check the content of foundry.toml
cat contracts/foundry.toml
Length of output: 396
dev/contracts/deploy-testnet (1)
7-8: Fix typo in comment
The word "dependencies" is misspelled in the comment.
-# Update depencencies
+# Update dependenciesThe forge soldeer update command needs verification as mentioned in the deploy-ephemeral script review.
.github/workflows/test.yml (1)
6-7: LGTM: Workflow trigger optimization
The addition of paths-ignore for the contracts/** directory aligns perfectly with the PR objective of separating Solidity CI concerns. This ensures that the test workflow only runs when relevant files are changed.
Also applies to: 9-10
Summary
contract/is changed.contracts/.Summary by CodeRabbit
New Features
Bug Fixes
Chores