chore: add local dependencies#11
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| @@ -0,0 +1 @@ | |||
| slither-analyzer==0.11.5 | |||
There was a problem hiding this comment.
ah jeez is slither actually in python? lame.
I'd almost prefer using the docker release (Docker use is widespread in this org) https://github.com/crytic/slither#using-docker to avoid having this repo depend on foundry, node, and python. But I agree we need slither.
There was a problem hiding this comment.
I'd stick with venv, it'd be a bit cumbersome to also have to run Docker for each of the deps.
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
Looks good!
The local commands are a bit long, but this will be solved with the makefile.
| "license": "UNLICENSED", | ||
| "devDependencies": { | ||
| "solhint": "6.0.3" | ||
| } |
There was a problem hiding this comment.
Just came to my mind: we can reduce the risk of supply-chain attacks by not installing packages that have been released, say, less than 2 days ago. This gives a new package 2 days to be scrutinized by the community before we install it.
This config parameter has different names based on the chosen NPM manager. For example, for pnpm it's minimumReleaseAge.
There was a problem hiding this comment.
Done, I've set the value to 7 days, as the other CoW repos do!
There was a problem hiding this comment.
Approved. I reviewed #13 first and commented on some things that came from this PR.
I would not use nowadays pip, can we use uv has many advantages. See https://docs.astral.sh/uv/guides/migration/pip-to-project/#migrating-to-a-uv-project
Please, make sure you select the correct base branch.
|
nit: Just a small comment more. For future PR, I would personally prefer that instead of adding in one PR the "local dependencies" (with slither and solhint), its more common to introduce the binaries in the PR you introduce the tools. |
4 participants
Description
Add local pinned JS and Python dependencies under
dev/.Context
This gives the template local tool versions instead of asking developers to install Solhint or Slither globally.
Out of Scope
This PR only adds the dependency setup. It does not add Solhint config, Slither config, Justfile commands, CI, or hooks.
Testing Instructions
npm install --prefix dev.python -m venv dev/.venv.dev/.venv/bin/pip install -r dev/requirements.txt.dev/node_modules/.bin/solhint --version.dev/.venv/bin/slither --version.