Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

Configure slither and add to CI #1109

Merged
merged 24 commits into from
Jun 8, 2022
Merged

Configure slither and add to CI #1109

merged 24 commits into from
Jun 8, 2022

Conversation

sveitser
Copy link
Contributor

@sveitser sveitser commented Jun 3, 2022

Close #1108

@@ -20,7 +20,7 @@ pre-commit-hooks = {git = "https://github.com/Lucas-C/pre-commit-hooks"}
black = "*"
hdwallet = "^1.3.2"
ipython = "^7.28.0"
slither-analyzer = "^0.8.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sveitser sveitser marked this pull request as ready for review June 8, 2022 11:26
slither.config.json Outdated Show resolved Hide resolved
sveitser added 3 commits June 8, 2022 14:05
We run all the pre-commit hooks on the CI but also have a separate
slither workflow. Having a slither pre-commit hook would run it
twice on the CI.

I think it is rather slow for a pre-commit hook.
- Add slither to `run-ci-steps` script.
- Remove `prepend-timestamps` script. GitHub Actions has this built in
  now.
@sveitser
Copy link
Contributor Author

sveitser commented Jun 8, 2022

There's something wrong with the code scanning job

image

@sveitser
Copy link
Contributor Author

sveitser commented Jun 8, 2022

Running the "Slither /slither" job again completed the "Code scanning results / Slither" check. 🤷

@sveitser sveitser enabled auto-merge (squash) June 8, 2022 15:02
@philippecamacho
Copy link
Collaborator

Is it possible to run slither locally? If this is the case can we add a section to the README.md?

@philippecamacho
Copy link
Collaborator

Is it possible to run slither locally? If this is the case can we add a section to the README.md?

Just saw the Slither.md page.

@philippecamacho
Copy link
Collaborator

When I run slither on my machine I get this error (in red). Is it needed to do something special so that the CI does not complain?

ERC20 is re-used:
	- ERC20 (node_modules/@openzeppelin/contracts/token/ERC20/ERC20.sol#34-355)
	- ERC20 (node_modules/@rari-capital/solmate/src/tokens/ERC20.sol#8-195)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#name-reused

Copy link
Collaborator

@philippecamacho philippecamacho left a comment

Choose a reason for hiding this comment

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

Just added a question.
LGTM.

@sveitser sveitser merged commit 2bfa928 into main Jun 8, 2022
@sveitser sveitser deleted the configure-slither branch June 8, 2022 16:57
@sveitser
Copy link
Contributor Author

sveitser commented Jun 8, 2022

When I run slither on my machine I get this error (in red). Is it needed to do something special so that the CI does not complain?

ERC20 is re-used:
	- ERC20 (node_modules/@openzeppelin/contracts/token/ERC20/ERC20.sol#34-355)
	- ERC20 (node_modules/@rari-capital/solmate/src/tokens/ERC20.sol#8-195)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#name-reused

@philippecamacho These should be excluded because of the slither config file. I think it complains because due to the name collision it cannot analyze both contracts. However in this case they aren't our contracts anyway so it was easier to just ignore them. No idea why that's not working for you though. Is it the only error you get?

@philippecamacho
Copy link
Collaborator

When I run slither on my machine I get this error (in red). Is it needed to do something special so that the CI does not complain?

ERC20 is re-used:
	- ERC20 (node_modules/@openzeppelin/contracts/token/ERC20/ERC20.sol#34-355)
	- ERC20 (node_modules/@rari-capital/solmate/src/tokens/ERC20.sol#8-195)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#name-reused

@philippecamacho These should be excluded because of the slither config file. I think it complains because due to the name collision it cannot analyze both contracts. However in this case they aren't our contracts anyway so it was easier to just ignore them. No idea why that's not working for you though. Is it the only error you get?

It is the only "red" printed error I have yes. Then I have orange and plenty of green messages.

sveitser added a commit that referenced this pull request Jun 9, 2022
This is expected to fail on the current revision of this repo because of
some non-functional but bytecode affecting changes to the merkle tree
contract in #1109 that have not been deployed.

To verify one can checkout 0d13dec and
run the `etherscan-verify` of this commit but I already ran it and
all contracts are now verified.

To test the script the easiest way is to do a throwaway deployment on
Goerli and run the `etherscan-verify` script.

Close #1123
sveitser added a commit that referenced this pull request Jun 9, 2022
This is expected to fail on the current revision of this repo because of
some non-functional but bytecode affecting changes to the merkle tree
contract in #1109 that have not been deployed.

To verify one can checkout 0d13dec and
run the `etherscan-verify` of this commit but I already ran it and
all contracts are now verified.

To test the script the easiest way is to do a throwaway deployment on
Goerli and run the `etherscan-verify` script.

Close #1123
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure slither and analyze results
2 participants