Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions .github/workflows/test.yml
Copy link
Copy Markdown
Contributor

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

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 $RPC_URL_1 (for mainnet, for example) that would be swell


env:
  FOUNDRY_PROFILE: ci
  FORK_RPC_URL: ${{ secrets.RPC_URL_1 }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 name: Run Forge tests.


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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

      - name: Install Foundry
        uses: foundry-rs/foundry-toolchain@v1
        with:
          version: v1.5.1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for best code quality, we should deny compiler notes.

      - name: Run Forge build
        run: |
          forge build --deny notes --sizes


- name: Run Forge tests
run: forge test -vvv
14 changes: 14 additions & 0 deletions .gitignore
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
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[submodule "lib/forge-std"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't really like submodules.
Well, I also don't like using package.json because we have nothing to do with JS and adding this file means we're pretending we are a JS module, and this has weird consequences sometimes.

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 package.json. This is because that files needs to (1) contain all expected parameters, like version and (2) the version parameter needs to be bumped each time anything meaningful change. If I remember correctly, not bumping the version when the code changed lead to the frontend ignoring any changes and didn't see the new updates until we realized about this issue. This hadn't been a problem before package.json was introduced, apparently previous imports would just rely on the commit hash for changes, but that was ignored after introducing that file.

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 solhint) are still best imported through NPM, but maybe we can try to hide this from NPM, possibly storing this file in dev/package.json or something like this.

path = lib/forge-std
url = https://github.com/foundry-rs/forge-std
33 changes: 33 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,36 @@ Template for creating new smart contract projects.
This project is meant to be used as a templated during the creation of new Github repositories (will show in the `Create a new repository > Configuration > Start with a template` selector).

It will contain some useful configuration files and scripts, that can be used also with existing projects (manually copied).


## Usage

### Build

```shell
forge build
```

### Test

```shell
forge test
```

### Format

```shell
forge fmt
```

### Gas Snapshots

```shell
forge snapshot
```

### Deploy

```shell
forge script script/Counter.s.sol:CounterScript --rpc-url <your_rpc_url> --private-key <your_private_key>
```
8 changes: 8 additions & 0 deletions foundry.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"lib/forge-std": {
"tag": {
"name": "v1.14.0",
"rev": "1801b0541f4fda118a10798fd3486bb7051c5dd6"
}
}
}
Comment thread
anxolin marked this conversation as resolved.
6 changes: 6 additions & 0 deletions foundry.toml
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
Comment thread
anxolin marked this conversation as resolved.
1 change: 1 addition & 0 deletions lib/forge-std
Submodule forge-std added at 1801b0
19 changes: 19 additions & 0 deletions script/Counter.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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();
}
}
14 changes: 14 additions & 0 deletions src/Counter.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion would be to just use:

Suggested change
pragma solidity ^0.8.13;
pragma solidity ^0.8;

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 foundry.toml.
My argument for being permissive is that sometimes I need to import relatively new contracts into an old repo; specifying a strict version means that the old project wouldn't compile and still keep deterministic deployments. This could happen, for example, if we want to add a test on an old repo that includes the new contract: we'd have to bump the Solidity version of everything, and so possibly redeploy, just to make tests work.


contract Counter {
uint256 public number;

function setNumber(uint256 newNumber) public {
number = newNumber;
}

function increment() public {
number++;
}
}
24 changes: 24 additions & 0 deletions test/Counter.t.sol
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);
}
}