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

Slither CI workflow #192

Closed
wants to merge 6 commits into from
Closed

Slither CI workflow #192

wants to merge 6 commits into from

Conversation

GarrettJMU
Copy link

@GarrettJMU GarrettJMU commented Mar 3, 2023

Motivation

Coming from issue:
#70

Change Summary

Add slither to the ci workflow

Merge Checklist

Additional Context

n/a

@varunsrin
Copy link
Member

Thanks for making this PR.

Unfortunately, this seems to fail because slither is taking > 10 minutes to run. I don't think its a good idea to have such an expensive test suite run on every commit to a branch. I'm going to explore some options for speeding this up, open to suggestions if you have any

@GarrettJMU
Copy link
Author

GarrettJMU commented Mar 4, 2023

@varunsrin - Hm - I think a few other things. It doesn't look like we're not omitting node_modules but maybe taking a step back, what do you want to test here?

Do we also need to test all the folders in lib? Seems like right now we are just omitting forge-std. I just pushed an update to omit lib altogether and might be interesting to see what time tradeoff that has.

I even tried removing some of the inefficient scans like low-level-calls and still timing out. There is something expensive in their analysis that's chewing away.

@varunsrin
Copy link
Member

varunsrin commented Mar 5, 2023

Yeah, i made another PR where I got it down to 11 minutes, but that's still painfully slow for every PR. Appreciate your work on this but it might not make sense to merge right now given how slow it is and just do the checks manually before contract releases.

@varunsrin
Copy link
Member

closing for now since we're not moving forward, thanks for trying @GarrettJMU

@varunsrin varunsrin closed this Mar 9, 2023
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.

2 participants