ci: add job to check contract sizes #60
Conversation
WalkthroughThe change involves renaming the GitHub Actions workflow to "Foundry" and adding a step to check contract sizes before running tests. This enhancement aims to improve the workflow's functionality by incorporating a size constraint validation process for smart contracts, ensuring they meet specified limits for efficient resource management. Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .github/workflows/contracts_size.yml (1 hunks)
- check_contracts_size.sh (1 hunks)
Additional comments: 2
.github/workflows/contracts_size.yml (1)
- 1-25: The GitHub Actions workflow is well-structured and aligns with the PR objectives to automate the process of contract size verification. It correctly triggers on pull requests and pushes to the main branch, checks out the code including submodules, installs the Foundry toolchain, and checks the contract sizes using the provided shell script. This addition to the CI pipeline is a proactive approach to ensuring that contracts remain within the size limits.
check_contracts_size.sh (1)
- 1-28: The shell script is well-implemented with clear documentation, appropriate error handling, and a robust regex pattern for parsing the output of
forge build --sizes. The logic to determine if a contract exceeds the allowed size limit is correctly implemented, making this script a valuable addition to the CI pipeline for ensuring contracts remain within size constraints.
nicopernas
force-pushed
the
nico/check-contract-sizes
branch
from
43dd83f
to
390b0c2
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/foundry.yml (2 hunks)
Additional comments: 2
.github/workflows/foundry.yml (2)
- 1-1: The workflow name has been updated from "Tests" to "Foundry" to reflect the broader scope of tasks it now handles, including contract size checks and testing. This change improves clarity and aligns the workflow name with its expanded responsibilities.
- 27-27: The "Tests" step remains unchanged and correctly positioned after the contract size check. This ordering ensures that contracts are only tested if they meet size constraints, optimizing the CI process by preventing unnecessary test runs on non-compliant contracts.
.github/workflows/foundry.yml
Outdated
| - name: Check contract sizes | ||
| run: forge build --sizes | ||
|
|
||
| - name: Tests |
There was a problem hiding this comment.
I thought it would be nice to have "all foundry" commands under the same job, so we install the dependency only once. However, it also means that oversized contracts won't get the tests run... I guess that's ok but maybe someone else has strong opinions about it? @RnkSngh what do you think?
There was a problem hiding this comment.
Ah yes I was also meaning to ask on if we should just lump everything with the same install dependencies under one job; so i think this is a good idea.
Does the forge build sizes actually throw on if we have contracts that are above the size limit? Is
that what we want here? I think some of the test contracts might be above the size limit as well so wondering if we can somehow exclude those
it also means that oversized contracts won't get the tests run
Wondering if we can have the forge test command go before forge build --sizes if we're worried about this then? I think forge test should build them, and since there are no changes to contracts, forge build--sizes will just use the already compiled contracts so it should be fine to have this go after
.github/workflows/foundry.yml
Outdated
| - name: Check contract sizes | ||
| run: forge build --sizes | ||
|
|
||
| - name: Tests |
There was a problem hiding this comment.
Ah yes I was also meaning to ask on if we should just lump everything with the same install dependencies under one job; so i think this is a good idea.
Does the forge build sizes actually throw on if we have contracts that are above the size limit? Is
that what we want here? I think some of the test contracts might be above the size limit as well so wondering if we can somehow exclude those
it also means that oversized contracts won't get the tests run
Wondering if we can have the forge test command go before forge build --sizes if we're worried about this then? I think forge test should build them, and since there are no changes to contracts, forge build--sizes will just use the already compiled contracts so it should be fine to have this go after
| @@ -21,5 +21,8 @@ jobs: | |||
| - name: Install Foundry | |||
| uses: foundry-rs/foundry-toolchain@v1 | |||
|
|
|||
| - name: Test | |||
| - name: Check contract sizes | |||
There was a problem hiding this comment.
Might also be worth adding forge coverage to the ci as well. right now forge coverage is throwing a compile error, but if we add the line src/=lib/optimism/packages/contracts-bedrock/src/ to remappings.txt file, its hould fix it (looks like it's some bug in the optimism library)
There was a problem hiding this comment.
(thanks to @Inkvi for the suggestion)
nicopernas
force-pushed
the
nico/check-contract-sizes
branch
from
390b0c2
to
d29b280
Compare
Summary by CodeRabbit