-
Notifications
You must be signed in to change notification settings - Fork 0
Add default foundry #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| name: CI | ||
|
|
||
| permissions: {} | ||
|
|
||
| on: | ||
| push: | ||
| pull_request: | ||
| workflow_dispatch: | ||
|
|
||
| env: | ||
| FOUNDRY_PROFILE: ci | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fork tests are really common, so would be nice to have this secret built in: Also, if we can define org level secrets for all our RPCs we might want to use like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree the template should account for forked tests, but I wouldn't do it like this otherwise any random GitHub Action has access to this secret. I personally like to keep e2e tests separate from actual test in the workflow as well, but for simplicity we can just add the secret to |
||
|
|
||
| jobs: | ||
| check: | ||
| name: Foundry project | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| steps: | ||
| - uses: actions/checkout@v5 | ||
| with: | ||
| persist-credentials: false | ||
| submodules: recursive | ||
|
|
||
| - name: Install Foundry | ||
| uses: foundry-rs/foundry-toolchain@v1 | ||
|
Comment on lines
+25
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should lock the foundry version. Its an extremely common thing that foundry just updates and things break. We will need to remember to update the template project from time to time unforutnately though
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to do that in an interation PR. This PR aims to not change anything :) Added a out of scope section to be more explicit on this. |
||
|
|
||
| - name: Show Forge version | ||
| run: forge --version | ||
|
|
||
| - name: Run Forge fmt | ||
| run: forge fmt --check | ||
|
|
||
| - name: Run Forge build | ||
| run: forge build --sizes | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for best code quality, we should deny compiler notes. |
||
|
|
||
| - name: Run Forge tests | ||
| run: forge test -vvv | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # Compiler files | ||
| cache/ | ||
| out/ | ||
|
|
||
| # Ignores development broadcast logs | ||
| !/broadcast | ||
| /broadcast/*/31337/ | ||
| /broadcast/**/dry-run/ | ||
|
|
||
| # Docs | ||
| docs/ | ||
|
|
||
| # Dotenv file | ||
| .env |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| [submodule "lib/forge-std"] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we may consider replacing the gitmodules with NPM modules. In which case, submodules aren't necessary This makes a lot of sense because cannon and extra linters we might use comes via a npm module also
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also don't really like submodules. As a concrete example, an issue I (somewhat) remember in the past is about the frontend having problems importing and updating the code from a contract repo once we introduced a I don't have an obvious best choice here, I'm fine with the code as it is at this point. Later if we think it's better, we can make this into a NPM package. I think we may want to eventually do that because a few linting tools (like |
||
| path = lib/forge-std | ||
| url = https://github.com/foundry-rs/forge-std | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "lib/forge-std": { | ||
| "tag": { | ||
| "name": "v1.14.0", | ||
| "rev": "1801b0541f4fda118a10798fd3486bb7051c5dd6" | ||
| } | ||
| } | ||
| } | ||
|
anxolin marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| [profile.default] | ||
| src = "src" | ||
| out = "out" | ||
| libs = ["lib"] | ||
|
|
||
| # See more config options https://github.com/foundry-rs/foundry/blob/master/crates/config/README.md#all-options | ||
|
anxolin marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // SPDX-License-Identifier: UNLICENSED | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can probably remove this file. Most of the time we only use scripts for on-chain deployments, and thats pending/out of this PR scope. |
||
| pragma solidity ^0.8.13; | ||
|
|
||
| import {Script} from "forge-std/Script.sol"; | ||
| import {Counter} from "../src/Counter.sol"; | ||
|
|
||
| contract CounterScript is Script { | ||
| Counter public counter; | ||
|
|
||
| function setUp() public {} | ||
|
|
||
| function run() public { | ||
| vm.startBroadcast(); | ||
|
|
||
| counter = new Counter(); | ||
|
|
||
| vm.stopBroadcast(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,14 @@ | ||||||
| // SPDX-License-Identifier: UNLICENSED | ||||||
| pragma solidity ^0.8.13; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can probably tighten the required solidity version here to 0.8.30 or so
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My suggestion would be to just use:
Suggested change
I know it's sometimes recommended to be tight in the versioning, but I never found the arguments convincing. I think it's fine to be permissive here while strict in |
||||||
|
|
||||||
| contract Counter { | ||||||
| uint256 public number; | ||||||
|
|
||||||
| function setNumber(uint256 newNumber) public { | ||||||
| number = newNumber; | ||||||
| } | ||||||
|
|
||||||
| function increment() public { | ||||||
| number++; | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // SPDX-License-Identifier: UNLICENSED | ||
| pragma solidity ^0.8.13; | ||
|
|
||
| import {Test} from "forge-std/Test.sol"; | ||
| import {Counter} from "../src/Counter.sol"; | ||
|
|
||
| contract CounterTest is Test { | ||
| Counter public counter; | ||
|
|
||
| function setUp() public { | ||
| counter = new Counter(); | ||
| counter.setNumber(0); | ||
| } | ||
|
|
||
| function test_Increment() public { | ||
| counter.increment(); | ||
| assertEq(counter.number(), 1); | ||
| } | ||
|
|
||
| function testFuzz_SetNumber(uint256 x) public { | ||
| counter.setNumber(x); | ||
| assertEq(counter.number(), x); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reccomend here we actually use a configuration file similar to what we have got with Euler, as the one provided by default with foundry has some deficiencies imo
https://github.com/cowprotocol/euler-integration-contracts/blob/master/.github/workflows/test.yml
I will also comment on specific points to track