-
Notifications
You must be signed in to change notification settings - Fork 0
feat: improvements #9
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,10 @@ | ||
| ######################################## | ||
| ## Keys and Addresses ## | ||
| ######################################## | ||
| PRIVATE_KEY= | ||
| DEPLOYER_ADDRESS= | ||
|
|
||
| ######################################## | ||
| ## RPC URLs ## | ||
| ######################################## | ||
| ETH_RPC_URL= | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,14 @@ | ||||||||||||||||||
| name: "Setup" | ||||||||||||||||||
| description: "Install Foundry" | ||||||||||||||||||
|
|
||||||||||||||||||
| runs: | ||||||||||||||||||
| using: "composite" | ||||||||||||||||||
| steps: | ||||||||||||||||||
| - name: "Install Foundry" | ||||||||||||||||||
| uses: "foundry-rs/foundry-toolchain@c7450ba673e133f5ee30098b3b54f444d3a2ca2d" | ||||||||||||||||||
| with: | ||||||||||||||||||
| version: "stable" | ||||||||||||||||||
|
|
||||||||||||||||||
| - name: "Show the Foundry config" | ||||||||||||||||||
| shell: "bash" | ||||||||||||||||||
| run: "forge --version && echo '\n' && forge config" | ||||||||||||||||||
|
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. ubernit: in test instructions, nice to provide a link to check this https://github.com/cowprotocol/contracts-template/actions/runs/25370595345/job/74392802415#step:3:28
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. I think that adds noise, that's why
Comment on lines
+12
to
+14
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. Nit, I'm in favor of splitting this, the use of (I removed
Suggested change
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,99 @@ | ||||||
| name: "CI" | ||||||
|
|
||||||
| permissions: {} | ||||||
|
|
||||||
|
Comment on lines
+3
to
+4
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. Unnecessary entry?
Suggested change
|
||||||
| # Uncomment to use secrets | ||||||
| # env: | ||||||
| # ETHERSCAN_API_KEY: ${{ secrets.ETHERSCAN_API_KEY }} | ||||||
| # ETH_RPC_URL: ${{ secrets.ETH_RPC_URL }} | ||||||
|
Comment on lines
+5
to
+8
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. These secrets will be seen by all jobs, and we don't necessarily trust all of them to keep the secret safe. We should move these envs to where they're needed (that is, testing). |
||||||
|
|
||||||
| on: | ||||||
| workflow_dispatch: | ||||||
| pull_request: | ||||||
| push: | ||||||
| branches: | ||||||
| - "main" | ||||||
|
Comment on lines
+14
to
+15
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. Nit: let's have CI run on push, otherwise one first creates a PR and only then notices that CI is failing.
Suggested change
|
||||||
|
|
||||||
| jobs: | ||||||
| build: | ||||||
| runs-on: "ubuntu-latest" | ||||||
| steps: | ||||||
| - name: "Check out the repo" | ||||||
| uses: "actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd" | ||||||
| with: | ||||||
| submodules: recursive | ||||||
|
|
||||||
| - name: "Setup" | ||||||
| uses: "./.github/actions/setup" | ||||||
|
|
||||||
| - name: "Build the contracts and print their size" | ||||||
|
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.
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. What's the issue there? This is a very meaningful output especially if there are many contract which might go over the limit or get close to it. I'd keep it.
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. no issue, just made it simpler for reviewers to check the output by providing a link to it. |
||||||
| run: "forge build --sizes --deny warnings" | ||||||
|
|
||||||
| format: | ||||||
| runs-on: "ubuntu-latest" | ||||||
| needs: build | ||||||
| steps: | ||||||
| - name: "Check out the repo" | ||||||
| uses: "actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd" | ||||||
| with: | ||||||
| submodules: recursive | ||||||
|
|
||||||
| - name: "Setup" | ||||||
| uses: "./.github/actions/setup" | ||||||
|
|
||||||
| - name: "Install solhint" | ||||||
| run: "npm i -g solhint" | ||||||
|
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. What is the good practice here? if we rely on npm for linting, shouldn't we have it in a package.json? (so we can run locally, and more importantly control the version) I think @fedgiac might be oppinionated about the lintting and the addition of a package.json into the project (something we try to avoid, but if we can't get a good linting otherwise, I guess there's no way around?)
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 add linting instructions locally. no?
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.
I've always installed Solhint globally to avoid package.json for a single package.
We could hardcode the version both locally and in the CI, good point!
|
||||||
|
|
||||||
| - name: "Print solhint version" | ||||||
| run: "solhint --version" | ||||||
|
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. if this is relevant, should it be in the summary?
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. It's relevant, especially in case a new version releases and locally lint passes (because of the old version), but doesn't on 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. then why not writing in the summary? |
||||||
|
|
||||||
| - name: "Lint the code" | ||||||
| run: "make lint" | ||||||
|
|
||||||
| slither: | ||||||
| runs-on: ubuntu-latest | ||||||
| needs: build | ||||||
| steps: | ||||||
| - name: "Check out the repo" | ||||||
| uses: "actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd" | ||||||
| with: | ||||||
| submodules: recursive | ||||||
|
|
||||||
| - name: "Setup" | ||||||
| uses: "./.github/actions/setup" | ||||||
|
|
||||||
| - name: "Build contracts in src/ folder" | ||||||
| run: forge build --build-info --skip test script | ||||||
|
|
||||||
| - name: "Run slither analyzer" | ||||||
| uses: crytic/slither-action@f197989dea5b53e986d0f88c60a034ddd77ec9a8 | ||||||
| with: | ||||||
| ignore-compile: false | ||||||
| slither-version: "0.11.0" | ||||||
| fail-on: "low" | ||||||
|
|
||||||
| test: | ||||||
| runs-on: "ubuntu-latest" | ||||||
| needs: build | ||||||
| steps: | ||||||
| - name: "Check out the repo" | ||||||
| uses: "actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd" | ||||||
| with: | ||||||
| submodules: recursive | ||||||
|
|
||||||
| - name: "Setup" | ||||||
| uses: "./.github/actions/setup" | ||||||
|
|
||||||
| - name: "Generate a fuzz seed that changes weekly to avoid burning through RPC allowance" | ||||||
| run: > | ||||||
| echo "FOUNDRY_FUZZ_SEED=$( | ||||||
| echo $(($EPOCHSECONDS - $EPOCHSECONDS % 604800)) | ||||||
| )" >> $GITHUB_ENV | ||||||
|
Comment on lines
+87
to
+91
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. How does the fuzz seed affect the RPC allowance? |
||||||
|
|
||||||
| - name: "Run the tests" | ||||||
| run: "FOUNDRY_PROFILE=ci forge test --force --isolate -vvv --show-progress --gas-snapshot-check true" | ||||||
|
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.
update: I see this matches |
||||||
|
|
||||||
| - name: "Check test coverage" | ||||||
| env: | ||||||
| COVERAGE_MIN: 100 | ||||||
| run: make coverage-check | ||||||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| repos: | ||
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
|
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 don't know how this tool works, but this looks like a hidden source of supply-chain attacks: it takes code from a random GitHub repo and executes it on each commit. |
||
| rev: v4.5.0 | ||
| hooks: | ||
| - id: mixed-line-ending | ||
| name: Mixed Line Ending (convert to LF) | ||
| args: ["--fix=lf"] | ||
| description: Forces to replace line ending by the UNIX 'lf' character. | ||
| exclude: "^docs/autogen" | ||
|
|
||
| - repo: local | ||
|
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 think all of this should be a pre-push more than a pre-commit, otherwise it makes committing too slow. |
||
| hooks: | ||
| - id: format | ||
| name: Run lint | ||
| description: Run lint with `make lint` | ||
| language: system | ||
| entry: make lint | ||
| exclude: "^lib/" | ||
| types: [solidity] | ||
| pass_filenames: false | ||
|
|
||
| - id: slither | ||
| name: Run Slither | ||
| description: Run Slither with `make slither` | ||
| language: system | ||
| entry: make slither | ||
| types: [solidity] | ||
| pass_filenames: false | ||
|
|
||
| - id: snapshot | ||
| name: Generate gas snapshot (might take a while) | ||
| description: Generate gas snapshot with `make snapshot` (might take a while) | ||
| language: system | ||
| entry: bash -c 'make snapshot-pre-commit 2>&1 | tee /dev/tty' | ||
| types: [solidity] | ||
| pass_filenames: true | ||
| require_serial: true | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| { | ||
| "extends": "solhint:recommended", | ||
| "rules": { | ||
| "code-complexity": ["error", 8], | ||
| "compiler-version": ["error", ">=0.8.34"], | ||
| "func-visibility": [ | ||
| "error", | ||
| { | ||
| "ignoreConstructors": true | ||
| } | ||
| ], | ||
| "max-line-length": ["error", 125], | ||
| "one-contract-per-file": "error", | ||
| "named-parameters-mapping": "error", | ||
| "func-param-name-mixedcase": "error", | ||
| "modifier-name-mixedcase": "error", | ||
| "ordering": "error", | ||
| "func-name-mixedcase": "error", | ||
| "private-vars-leading-underscore": "off", | ||
|
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. do you have some context on your lint picks? Specially the overrides to disable some rules?
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. This is just something I'm used to. Most of them are good practices anyways! Btw not all are overrides, just explicit settings in case defaults ever change. |
||
| "no-console": "error", | ||
| "not-rely-on-time": "off", | ||
| "gas-custom-errors": "error", | ||
| "no-unused-vars": "error", | ||
| "no-complex-fallback": "off", | ||
| "payable-fallback": "off", | ||
| "avoid-tx-origin": "error", | ||
| "no-inline-assembly": "off", | ||
| "avoid-low-level-calls": "off", | ||
| "import-path-check": "off", | ||
| "func-named-parameters": "warn", | ||
| "interface-starts-with-i": "error", | ||
| "use-natspec": [ | ||
| "warn", | ||
| { | ||
| "notice": { | ||
| "enabled": true | ||
| }, | ||
| "param": { | ||
| "enabled": true | ||
| }, | ||
| "return": { | ||
| "enabled": true | ||
| }, | ||
| "author": { "enabled": false } | ||
| } | ||
| ], | ||
| "gas-strict-inequalities": "off", | ||
| "no-empty-blocks": "off", | ||
| "gas-indexed-events": "off", | ||
| "gas-increment-by-one": "off", | ||
| "gas-struct-packing": "off", | ||
| "function-max-lines": "off", | ||
| "max-states-count": "off" | ||
|
Comment on lines
+3
to
+53
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 think there are too many entries here and I think most of them aren't necessary. Similar comment for the other two files, but there I expect turning things off will be more common. We can always add and remove entries to this at a later point. Let's try to keep things simple as much as we can. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "solidity.compileUsingRemoteVersion": "v0.8.34+commit.80d5c536", | ||
| "[solidity]": { | ||
| "editor.defaultFormatter": "JuanBlanco.solidity" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,65 @@ | ||||||
| ifneq (,$(wildcard ./.env)) | ||||||
|
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. not strongly opposed, but there's more modern tools today that are also frendlier for other platforms like windows. What's your oppinion on
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. Just is fine, but I've preferred Makefile. I can migrate to Just if others want it! |
||||||
| include .env | ||||||
| export | ||||||
| endif | ||||||
|
|
||||||
| .PHONY: all clean build lint slither test fmt coverage-check snapshot | ||||||
|
|
||||||
| help: ## Print all targets and descriptions | ||||||
| @awk 'BEGIN {FS = ":.*##"; printf "\nUsage:\n make \033[36m<target>\033[0m\n"} /^[.a-zA-Z0-9_-]+:.*?##/ { printf " \033[36m%-25s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } END { printf "\n" }' $(MAKEFILE_LIST) | ||||||
|
|
||||||
| all: ## Build, fmt, slither, check coverage, and snapshot gas | ||||||
|
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. Following thre README leads to failure (missing dependency
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. Should I make it a dev dependency, install it with Slither needs to be global tho, as it's installed with |
||||||
| make clean && \ | ||||||
| make build && \ | ||||||
| make fmt && \ | ||||||
| make slither && \ | ||||||
| make coverage-check && \ | ||||||
| make snapshot | ||||||
|
|
||||||
| clean: ## Clean the project | ||||||
| forge clean | ||||||
|
|
||||||
| build: ## Build the contracts | ||||||
| forge build --force | ||||||
|
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. Nit, it's faster to use the cache on day-to-day operations. For large contracts, rebuilding can take many seconds. Same comment for all occurrences of
Suggested change
|
||||||
|
|
||||||
| lint: ## Run lint | ||||||
| forge fmt --check && \ | ||||||
| solhint -c .solhint.json --max-warnings 0 "src/**/*.sol" && \ | ||||||
| solhint -c script/.solhint.json --max-warnings 0 "script/**/*.sol" && \ | ||||||
| solhint -c test/.solhint.json --max-warnings 0 "test/**/*.t.sol" | ||||||
|
|
||||||
| fmt: ## Format the contracts | ||||||
| forge fmt && \ | ||||||
| solhint -c .solhint.json --max-warnings 0 "src/**/*.sol" && \ | ||||||
| solhint -c script/.solhint.json --max-warnings 0 "script/**/*.sol" && \ | ||||||
| solhint -c test/.solhint.json --max-warnings 0 "test/**/*.t.sol" | ||||||
|
Comment on lines
+33
to
+35
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. Why is this part of |
||||||
|
|
||||||
| slither: ## Run slither (requires 0.11.0) | ||||||
| slither . --include-paths "(src)" --fail-low --config-file slither.config.json | ||||||
|
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. Here we use I think we should pick only one of the two and not having both whitelist and blacklists. |
||||||
|
|
||||||
| test: ## Run tests | ||||||
| forge test --force --isolate -vvv --show-progress --gas-snapshot-check true | ||||||
|
|
||||||
| coverage-summary: ## Run tests and generate coverage summary | ||||||
| forge coverage --no-match-coverage "(test|script)" --force --report summary | ||||||
|
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. This isn't doing what we expect it to do: the command would ignores files like |
||||||
|
|
||||||
| coverage-lcov: ## Run tests and generate coverage lcov report | ||||||
| forge coverage --no-match-coverage "(test|script)" --force --report lcov | ||||||
|
|
||||||
| COVERAGE_MIN := 100 | ||||||
| coverage-check: ## Check if test coverage is above the minimum | ||||||
| make coverage-summary | tee coverage.txt | ||||||
| @coverage=$$(grep "| Total" coverage.txt | awk '{print $$4}' | sed 's/%//'); \ | ||||||
| if [ -z "$$coverage" ]; then \ | ||||||
| echo "\n❌ Failed to extract coverage percentage.\n"; \ | ||||||
| exit 1; \ | ||||||
| elif [ $$(echo "$$coverage < $(COVERAGE_MIN)" | bc -l) -eq 1 ]; then \ | ||||||
| echo "\n❌ Current coverage of $$coverage% below the minimum of $(COVERAGE_MIN)%.\n"; \ | ||||||
| exit 1; \ | ||||||
| else \ | ||||||
| echo "\n✅ Current coverage of $$coverage% meets the minimum of $(COVERAGE_MIN)%.\n"; \ | ||||||
| fi | ||||||
| @rm coverage.txt | ||||||
|
|
||||||
| snapshot: ## Create a snapshot | ||||||
| forge snapshot --force --isolate --desc --show-progress | ||||||

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.
We shouldn't have env files with private keys. They will be committed and made public. Yes this happened.
It's so bad that I'd drop the file completely and leave each dev to manage env themselves just to avoid that they add the private key to it. Deployments are super rare anyway, it's ok to take a bit more time to export the secret key from somewhere. Even better, the private key isn't stored as a string but on dedicated wallet.