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

refactor: add VULNERABLE_SOLC_VERSIONS and logic #1477

Merged
merged 2 commits into from
Nov 30, 2022

Conversation

devtooligan
Copy link
Contributor

addresses #1476
precursor to #1424

@0xalpharush
Copy link
Contributor

I think this approach makes sense. There's probably other areas it could be used e.g. the external function detector.

@devtooligan devtooligan force-pushed the 1476-refactor-vulnerable-solc-version branch from 43dd448 to c522ef9 Compare November 22, 2022 22:40
@devtooligan
Copy link
Contributor Author

@0xalpharush fixed and ready for re-review.

question: do you prefer 1 commit per pr? so github commit --amend and github push --force-with-lease for additional changes? or do you prefer to keep all the interim history commits? i assumed the former.

@0xalpharush
Copy link
Contributor

@devtooligan There's not a strict style atm. I think force pushing is fine for small commits but if you make large changes and the commits aid in reviewing the PR, leaving the interim history makes sense.

@montyly
Copy link
Member

montyly commented Nov 28, 2022

I would recommend against force push, or changing anything that was pushed on github (see Linus's point of view on rebasing)

Github allows to squash commit when we merge the PR, so it's ok to have multiple commits in the PRs ;)

Copy link
Member

@montyly montyly left a comment

Choose a reason for hiding this comment

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

This is a great idea, I think we can clean up a lot of detectors following this schema. Thanks @devtooligan

slither/detectors/abstract_detector.py Outdated Show resolved Hide resolved
@montyly
Copy link
Member

montyly commented Nov 28, 2022

Note: when merging the PR, we need to update https://github.com/crytic/slither/wiki/Adding-a-new-detector

@devtooligan
Copy link
Contributor Author

@montyly wrote:

Note: when merging the PR, we need to update https://github.com/crytic/slither/wiki/Adding-a-new-detector

@0xalpharush maybe you or someone else can help with this. Github wiki is not set up very well for open source contributions from outsiders 😄

We will need to update the snippet in the wiki with:

VULNERABLE_SOLC_VERSIONS: List[str] = []

Then down below in the notes maybe add something like:

`VULNERABLE_SOLC_VERSIONS` lets you specify which versions of solc
are vulnerable.  If this list is not empty, the detector will
only run if the solc version is included on the list.

@devtooligan
Copy link
Contributor Author

@0xalpharush @montyly Requested changes made. Aside from updating the wiki, this should be good for final re-review and merge.

@montyly montyly changed the base branch from master to dev November 30, 2022 08:32
@montyly montyly merged commit 36711ed into crytic:dev Nov 30, 2022
@montyly
Copy link
Member

montyly commented Nov 30, 2022

I merged the PR through #1485. Thanks again @devtooligan, this was a great idea

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.

3 participants