Skip to content
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

ci(slither): add config file #22

Closed
wants to merge 32 commits into from
Closed

Conversation

re1ro
Copy link

@re1ro re1ro commented Sep 21, 2023

Resolves #21

  • Slither: adding default config file instead of hardcoding params in the github action
  • Slither: pinning node 18 that comes with ubuntu so it's not downloading the latest

mds1 and others added 30 commits October 3, 2022 17:18
* ci: add slither

* ci: slither code scanning integration

* ci: change configs

* build: bump pragma

* build: change pragmas
* docs: add WIP note

* ci: add scopelint to ci

* fix: add foundry install step to CI

* style: move fmt before slither so both slither tasks are adjacent in PRs

* wrong solidity formatting to test ci fmt job

* fix solidity and break toml formatting to test fmt ci job

* restore formatting
* one test should error

* chore: fix naming convention + exclude that slither check
* add unused function to fail slither and update ci

* dummy commit

* add comment

* updates
* chore: put ci profile first since it inherits default solc, fix typo

* docs: update README

* build: update forge-std

* ci: download scopelint binary

* ci: fix repo

* ci: try different binary

* docs: update README
* ci: remove 'install deps' step, foundry does this automatically

* ci: initial coverage check

* chore: install deps for coverage (not yet auto installed)

* ci: install lcov

* ci: forgot to generate lcov

* ci: comment out the lcov directory filter

* ci: filter out test and script dirs by default

* refactor: use deploy script to scaffold tests, follow best practice test structure + naming conventions

* docs: update docs

* docs: add info on coverage

* ci: forge coverage now installs missing deps

* ci: log scopelint version

* ci: remove outdated rust toolchain install

* chore: comment out broadcast call in sample script for now

* chore: add lcov.info to gitignore

* ci: preserve coverage branch info

* docs: update best practices link and coverage section

* ci: also print coverage summary in CI (though it won't be filtered)

* doc: add comments
* ci: add second coverage tool

* try another

* maybe remove versioning fixes other tool

* fixes

* test comment deletion

* test title

* remove title

* test

* always run

* restore normal counter file

* cleanup ci file
* ci: fix bug in "slither-analyze" job in ci.yml

* ci: change the target from "contracts/" to "src/"

* ci: add "security-events" write permission to "slither-analyze" job in ci.yml

* ci: remove redundant "filter-paths" arg from "slither-analyze" job
* ide gitignore

* style: capitalization

---------

Co-authored-by: Matt Solomon <[email protected]>
mds1 and others added 2 commits July 7, 2023 09:22
* chore: disable slither reentrancy detector in scripts

* build: pin to 0.8.20

* chore: absolute imports

* ci: ignore solc-version check

this check is subjective, and I don't think project should necessarily
follow the version recommendations from slither (partly because historically
the versions suggested by that check were not updated frequently)

* build: explicitly specify evm version to protect against foundry changing the default

* docs: update README
@@ -0,0 +1,4 @@
{
"detectors_to_exclude": "naming-convention,solc-version",
"filter_paths": "(./lib|./test)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the parentheses needed here? Since before when passed directly to the CLI it was "./lib|./test" without the parentheses

Copy link
Contributor

Choose a reason for hiding this comment

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

From my local testing it seems they are not needed

Suggested change
"filter_paths": "(./lib|./test)"
"filter_paths": "./lib|./test"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add slither.config.json to the template instead of hardcoding config params in the CI.
6 participants