Skip to content
Open
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
9 changes: 9 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,12 @@ docs/

# Dotenv file
.env

# Local development dependencies
dev/node_modules/
dev/.venv/

# Local reports
coverage.txt
lcov.info
.gas-snapshot
66 changes: 66 additions & 0 deletions Justfile
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I know that in the previous iteration of this PR you were suggested to use a Justfile, but since we have package.json already in use and most of these commands are one-liners, we can use package.json scripts instead, right?

If a command is longer than one line or would be better on multiple lines, a script can be added like in a scripts/ directory and called by pnpm.

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.

Fair point, but in case of some longer commands (like snapshots, Solhint etc) it's easier to just write just <target> instead of remembering the full command.

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.

It's a bit annoying to use NPM scripts here because package.json is in dev and so it would be something like npm run --prefix dev whatever.

The Just file looks more easy to work with than NPM scripts so I'm weakly in favor of keeping the new tool. But I'm also fine with dropping it for simplicity if we decide that.

Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
set shell := ["bash", "-eu", "-o", "pipefail", "-c"]
set quiet # Doesn't print the command that is being run

COVERAGE_MIN := env_var_or_default("COVERAGE_MIN", "100")
JUST := just_executable()

# Runs `just help`
default: help

# Show available recipes
help:
{{JUST}} --list

# Compile contracts with `forge build`
build:
forge build

# Compile all contracts with `forge build --force`
build-all:
forge build --force

# Format Solidity sources with `forge fmt`
fmt:
forge fmt

# Check formatting and run `solhint` on `src`/`script`/`test`
lint:
forge fmt --check
dev/node_modules/.bin/solhint --max-warnings 0 '**/*.sol'

# Run Slither static analysis on `src`
slither:
dev/.venv/bin/slither src --config-file slither.config.json

# Run tests with `forge test`
test:
forge test --force --isolate -vvv --show-progress --gas-snapshot-check true
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.

Some comments from here and here still apply. Notably I'd drop --force from everywhere but build-all.
Also applies to snapshot.


# Print coverage summary (excludes `test`/`script` files)
coverage-summary:
forge coverage --no-match-coverage "^(test|script)/" --report summary

# Generate lcov coverage report (excludes `test`/`script` files)
coverage-lcov:
forge coverage --no-match-coverage "^(test|script)/" --report lcov

# Fail if total coverage is below `COVERAGE_MIN` (default `100`)
coverage-check:
{{JUST}} coverage-summary > coverage.txt
coverage="$(awk '/^\| Total/ {gsub(/%/, "", $4); print $4}' coverage.txt)"; \
Comment on lines +47 to +50
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 we use the minimum of all four coverage values here instead of the first?
I won't push much for this because it makes this line even harder to read, but if we keep it as it is we should write a comment about this.

cat coverage.txt; \
if [ -z "$coverage" ]; then echo "Failed to extract coverage percentage."; exit 1; fi; \
awk "BEGIN {exit !($coverage >= {{COVERAGE_MIN}})}" || { echo "Coverage $coverage% is below {{COVERAGE_MIN}}%."; exit 1; }; \
rm coverage.txt

# Generate gas snapshots with `forge snapshot`
snapshot:
forge snapshot --force --isolate --desc --show-progress

# Run build, lint, slither, test, coverage-check, snapshot
all:
{{JUST}} build
{{JUST}} lint
{{JUST}} slither
{{JUST}} coverage-check
{{JUST}} snapshot
43 changes: 25 additions & 18 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,31 +6,38 @@ This project is meant to be used as a templated during the creation of new Githu

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


## Usage

### Just commands

Install `just` on your machine, then run `just --list` to see the available commands.
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.

Very small nit, it just looks nicer!

Suggested change
Install `just` on your machine, then run `just --list` to see the available commands.
Install `just` on your machine, then run `just help` to see the available commands.


The commands do not load `.env` files.
For local deploys, pass RPC URLs and private keys through your shell or secret manager.
Do not commit private keys.

### Build

```shell
forge build
just build
```

### Test

```shell
forge test
just test
```

### Format

```shell
forge fmt
just fmt
```

### Gas Snapshots

```shell
forge snapshot
just snapshot
```

### Deploy
Expand All @@ -44,18 +51,18 @@ forge script script/Counter.s.sol:CounterScript --rpc-url <your_rpc_url> --priva
The following operations need to be performed after this repository has been created.

- [ ] In GitHub repo settings:
- [ ] Add a new ruleset called "Protected branches" and include the following changes:
- Enforcement status: active
- Target branches: Include default branch
- Require linear history
- Require a pull request before merging
- Required approvals: 1
- Allowed merge methods: Squash
- Block force pushes
- [ ] In General → Features → Pull requests:
- Select "Pull request title and description" in "Default commit message" option
- Unckeck "Allow merge commits" option
- Check "Allow auto-merge" option
- [ ] Add a new ruleset called "Protected branches" and include the following changes:
- Enforcement status: active
- Target branches: Include default branch
- Require linear history
- Require a pull request before merging
- Required approvals: 1
- Allowed merge methods: Squash
- Block force pushes
- [ ] In General → Features → Pull requests:
- Select "Pull request title and description" in "Default commit message" option
- Unckeck "Allow merge commits" option
- Check "Allow auto-merge" option
- [ ] Run `forge install` to install the dependencies. This will create a new `foundry.lock` file which you should commit to the project
- [ ] Make sure you use the [latest version of Solidity](https://github.com/argotorg/solidity/releases) by updating the `solc` version in `foundry.toml`
- [ ] Once all entries in this list are checked, delete this section from the readme
- [ ] Once all entries in this list are checked, delete this section from the readme
3 changes: 3 additions & 0 deletions snapshots/CounterTest.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"increment - success": "43476"
}
Comment on lines +1 to +3
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.

How was this file generated? I removed it and tried to recreate it with any commands from just and it didn't work.

Loading