feat: improvements#9
Conversation
igorroncevic
force-pushed
the
fix/additions
branch
from
beb7f85 to
f72f434
Compare
igorroncevic
force-pushed
the
fix/additions
branch
2 times, most recently
from
c8c484b to
c5ffb64
Compare
igorroncevic
force-pushed
the
fix/additions
branch
from
c5ffb64 to
33e4bf5
Compare
anxolin
left a comment
anxolin
left a comment
There was a problem hiding this comment.
Nice! there's a lot of quality-of-live things in this PR.
As mentioned, in general, we prefer to break PRs into smaller scopped ones.
When we do too many things at once:
Easier to miss something
Harder to have conversations
More chances for the PR to get stuck for items you care less
Each item is relatively small, so no need to change this time imo, but please keep it in mind PRs should do just 1 thing as a general rule
Build is failing, its better you keep in draft and submit only after CI passes
https://github.com/cowprotocol/contracts-template/actions/runs/25370595345/job/74392861668
This action is outdated:
Lastly, I see your PR is oppinionated, that's not a bad thing, but you should explain things more and add more test instructions to facilitate the review.
|
|
||
| - name: "Show the Foundry config" | ||
| shell: "bash" | ||
| run: "forge --version && echo '\n' && forge config" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think that adds noise, that's why make help exists.
| - name: "Setup" | ||
| uses: "./.github/actions/setup" | ||
|
|
||
| - name: "Build the contracts and print their size" |
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
no issue, just made it simpler for reviewers to check the output by providing a link to it.
Maybe, if its useful, consider writing in the summary
| uses: "./.github/actions/setup" | ||
|
|
||
| - name: "Install solhint" | ||
| run: "npm i -g solhint" |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
We should add linting instructions locally. no?
Also, should the commit be blocked by the linter so it doesn't allow us to commit?
There was a problem hiding this comment.
What is the good practice here?
I've always installed Solhint globally to avoid package.json for a single package.
control the version
We could hardcode the version both locally and in the CI, good point!
We should add linting instructions locally. no?
make help or simply make shows the user what to run to do it.
should the commit be blocked by the linter so it doesn't allow us to commit?
pre-commit helps with this, although devs can always choose to use --no-verify to skip that. That's why we lint in the CI too.
| run: "npm i -g solhint" | ||
|
|
||
| - name: "Print solhint version" | ||
| run: "solhint --version" |
There was a problem hiding this comment.
if this is relevant, should it be in the summary?
There was a problem hiding this comment.
It's relevant, especially in case a new version releases and locally lint passes (because of the old version), but doesn't on CI.
There was a problem hiding this comment.
then why not writing in the summary?
|
|
||
| # Fuzz | ||
| fuzz = { runs = 1_000 } | ||
| invariant = { runs = 100, depth = 20, fail_on_revert = true } |
There was a problem hiding this comment.
fine with it, but any context on why u went with these specifically (4x the default runs)
There was a problem hiding this comment.
Locally, 1000 runs is plenty, and it could catch some bugs without running for 5+ minutes. However, the 10x runs in CI serves as a final catch all, for those extreme edge cases.
| deny = "warnings" # Why not always: sometimes you just want to code and see what comes out | ||
| fuzz.seed = '0' # It makes CI reproducible, but still on a local machine it tries different parameters and so we can see edge cases if needed. | ||
| [lint] | ||
| exclude_lints = ["asm-keccak256","unwrapped-modifier-logic","pascal-case-struct","mixed-case-function","unsafe-typecast","unsafe-cheatcode","unchecked-call","block-timestamp"] |
There was a problem hiding this comment.
why? I feel thjis needs a bit of documentation/justification
There was a problem hiding this comment.
Fair, I'll remove it so we can see which lints are unnecessary as time goes on. These are just some I often removed from before.
| [rpc_endpoints] | ||
| mainnet = "{ETH_RPC_URL}" |
There was a problem hiding this comment.
Not strong oppinion, but would it make sense RPC_URL_1 as a pattern for our envs?
There was a problem hiding this comment.
Didn't ring a bell for me immediately. I don't know all chain IDs by heart.
| 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 |
There was a problem hiding this comment.
Following thre README leads to failure (missing dependency solhint). Ideally we don't rely on global packages, adds friction to someone entering the project, and more importantly, they are error proned (each person can have a different version).
❯ make all
make clean && \
make build && \
make fmt && \
make slither && \
make coverage-check && \
make snapshot
forge clean
forge build --force
[⠊] Compiling...
[⠢] Compiling 23 files with Solc 0.8.34
[⠆] Solc 0.8.34 finished in 1.20s
Compiler run successful!
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"
/bin/sh: solhint: command not found
make[1]: *** [fmt] Error 127
make: *** [all] Error 2
There was a problem hiding this comment.
Should I make it a dev dependency, install it with npm i -D?
Slither needs to be global tho, as it's installed with pip.
| @@ -0,0 +1,65 @@ | |||
| ifneq (,$(wildcard ./.env)) | |||
There was a problem hiding this comment.
not strongly opposed, but there's more modern tools today that are also frendlier for other platforms like windows.
What's your oppinion on
https://just.systems/
There was a problem hiding this comment.
Just is fine, but I've preferred Makefile. I can migrate to Just if others want it!
I agree, but this failure you linked is non-deterministic, not something we're in control of. It happens. :) |
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
There are way too many things going on at the same time:
- We're introducing a lot of new tooling, all of which with bespoke new configurations.
- We take care of introducing detailed config files for each tool, not trusting (or duplicating!) default values. Config values should be introduced with an intent, but I mostly don't see it as often this isn't explained or clear from context.
- There is no explanation on how to test each new feature, we just introduce a lot of things and leave the details to later, leaving to the reviewer the burden of making sure things work as expected.
Anxo says that "each item is relatively small" but I disagree, unless we don't care about understanding what's going on. There are hundreds of parameters being introduced and used. I've got 31 comments in this PR alone and the only thing that stopped me from adding more is that I expect some comments to become moot with future changes (as well as my ignorance of a few tools and lack of desire of going through even more third-party docs).
I think this PR should be split in multiple ones to have a proper discussion on each. Anxo can explicitly override this and I'll continue with the review here but I think it sets a bad precedent.
The point of PRs is the discussion. The discussion makes sure we understand what's happening, why choices are made, and prevent mistakes that will take longer to debug at a later stage.
PRs with hundreds of comments and tens of changed files are horrible for this, they overwhelm the reviewer and make them say "yeah this is bad but there's already a lot going on so let's ignore this."
This is my suggested PR split:
- Update CI workflow.
- Update Foundry configs.
- Introduce Slither (and Python).
- Introduce Solhint (and Javascript).
- Introduce Makefile (or alternative).
- Introduce git hook tooling.
- Introduce coverage.
Another thing: there's value in simplicity. Ideally we should write the least amount of code that achieve a goal. Too much code is bad because it hides intent. If there's a single config line, it's easy to see why it's important. If there are too many, and if they say little, then we lose track of what's meaningful. The goal is understanding the code. Future changes will be easier as a consequence.
I appreciate the desire to have an impact and make large changes, but it's important that the change we make are intentional, well understood, and directed towards a clear purpose.
| - name: "Show the Foundry config" | ||
| shell: "bash" | ||
| run: "forge --version && echo '\n' && forge config" |
There was a problem hiding this comment.
Nit, I'm in favor of splitting this, the use of \n is weird and makes it a bit harder to search for the relevant log.
(I removed shell: "bash" because it seems unnecessary, considering that everywhere else we don't use it.)
| - name: "Show the Foundry config" | |
| shell: "bash" | |
| run: "forge --version && echo '\n' && forge config" | |
| - name: "Show the Foundry version" | |
| run: "forge --version" | |
| - name: "Show the Foundry config" | |
| run: "forge config" |
| branches: | ||
| - "main" |
There was a problem hiding this comment.
Nit: let's have CI run on push, otherwise one first creates a PR and only then notices that CI is failing.
| branches: | |
| - "main" |
| permissions: {} | ||
|
|
There was a problem hiding this comment.
Unnecessary entry?
| permissions: {} |
| ### Add dependencies | ||
|
|
||
| ```shell | ||
| forge fmt | ||
| forge install <dependency> | ||
| ``` | ||
|
|
||
| ### Gas Snapshots | ||
| ### Set up remappings | ||
|
|
||
| ```shell | ||
| forge snapshot | ||
| In `remappings.txt`, add the remappings for the dependencies, e.g.: | ||
|
|
||
| ``` | ||
| @openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/ | ||
| @openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/ | ||
| ``` | ||
|
|
||
| This will allow you to write shorter import paths in the contracts. |
There was a problem hiding this comment.
Nit: I don't think we need guidelines for adding a dependency, the since it's a very rare occurrence.
Also, I don't think remappings are that important to appear in the readme, also the amount of space saved is negligible at the price of some extra indirection:
@openzeppelin/contracts-upgradeable/
|----------------------------------|
|-----------------------------------------------|
lib/openzeppelin-contracts-upgradeable/contracts/
|
|
||
| ### Deploy | ||
|
|
||
| Add `--broadcast` to send the transaction. Add `--verify` to verify the contract using the Etherscan settings in `foundry.toml`. |
There was a problem hiding this comment.
Add --broadcast... to what? 😛
| forge clean | ||
|
|
||
| build: ## Build the contracts | ||
| forge build --force |
There was a problem hiding this comment.
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 --force here.
| forge build --force | |
| forge build |
| 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" |
There was a problem hiding this comment.
Why is this part of fmt as well? Shouldn't it look different from lint?
| 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 |
There was a problem hiding this comment.
This isn't doing what we expect it to do: the command would ignores files like ContestPlayer.sol and DescriptiveResults.sol. Same comment below.
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "detectors_to_exclude": "solc-version,timestamp,unused-return,naming-convention,low-level-calls,calls-loop,costly-loop,assembly-usage,immutable-states,different-pragma-directives-are-used,reentrancy-events,uninitialized-state-variables,incorrect-equality,too-many-digits,uninitialized-local", | |||
| solhint -c test/.solhint.json --max-warnings 0 "test/**/*.t.sol" | ||
|
|
||
| slither: ## Run slither (requires 0.11.0) | ||
| slither . --include-paths "(src)" --fail-low --config-file slither.config.json |
There was a problem hiding this comment.
Here we use --include-paths "(src)", on the config file we use "filter_paths": "(lib/|test/|script/)".
I think we should pick only one of the two and not having both whitelist and blacklists.
No, I don't want to override anything. I'm fine with splitting. I actually prefer it as mentioned. I just was a bit forgiving in this first PR from @igorroncevic to not demotivate on his first contribution, so I agreed to put more on the reviewers :P But let's go with the split approach. I definetly don't want to set a bad precendence, and even less do so in the template project! Let's have proper PRs, with proper descritions and test instructions and independent discussions. We want good foundations, given the number of comments this generated it can also be faster to get things merge if we split and have separate discussions. |
|
Closing in favor of smaller PRs. |
3 participants
Description
This PR improves the Foundry template with common project defaults for CI, linting, testing, deployment, and local development.
What was added
.env.examplewith common deployment and RPC variables..pre-commit-config.yamlfor local linting, Slither, and gas snapshot checks before commits..solhintignoreas the default ignore file..vscode/settings.jsonto use Solidity0.8.34and the Solidity formatter.Makefilewith common commands: build, lint, fmt, slither, test, coverage, and snapshot.deployments.jsonas a simple placeholder for deployed contract addresses and transaction hashes.foundry.tomlwith compiler, optimizer, formatter, fuzzing, invariant, RPC, Etherscan, and deterministic deployment settings.script/shell/mine-salt.shto help find CREATE2 salts.slither.config.jsonwith default Slither settings.