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: add slither job #714

Merged
merged 5 commits into from
Oct 26, 2023
Merged

ci: add slither job #714

merged 5 commits into from
Oct 26, 2023

Conversation

scorpion9979
Copy link
Contributor

Addresses #418.
Recreated from #431.

chore: fix solc remap path in slither config
@scorpion9979 scorpion9979 mentioned this pull request Oct 20, 2023
@scorpion9979
Copy link
Contributor Author

scorpion9979 commented Oct 20, 2023

@andreivladbrg I've updated the GitHub action, as it was failing due to attempting to call forge from the Slither job's Docker container.

Context:

  1. job run
  2. comment on a slither-action issue

@andreivladbrg
Copy link
Member

It looks like the slither job keeps on failing, I had a look and I couldn't figure it out why.

@scorpion9979 can you help us?

@scorpion9979
Copy link
Contributor Author

scorpion9979 commented Oct 24, 2023

@andreivladbrg It seems like the GitHub action is still attempting to call forge from inside the job's Docker container. I found another possible workaround at crytic/slither-action#44 (comment), which I just pushed a commit for. It might take a few trials to get this right.

Update: It was another failed attempt. I'll experiment more and run the workflow in my personal repo. I'll get back to you once I think it's ready, so please feel free to ignore any workflow approval requests for now.

revert: ci: use cached build for slither job
@andreivladbrg
Copy link
Member

I'll experiment more and run the workflow in my personal repo. I'll get back to you once I think it's ready

thank you 🙏🏻

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@andreivladbrg
Copy link
Member

andreivladbrg commented Oct 24, 2023

@scorpion9979 It looks like the slither job has passed.Am I missing something?
The fork tests are failing because you probably don't have set up RPC_URL_MAINNET in the github secrets of your repo.

@scorpion9979
Copy link
Contributor Author

@andreivladbrg Thanks! I just re-ran all CI jobs after setting up the GitHub secrets in my forked repo and all are now passing. I just double-checked to make sure the Slither job is as clean as possible, and I think this PR is now ready to be reviewed and merged.

@andreivladbrg
Copy link
Member

The fork tests have failed again. I've seen on your fork that they worked, and I also tested them locally and they worked.
I'm not sure what the issue is now

Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

Looks good.

Good job @scorpion9979

@andreivladbrg andreivladbrg merged commit 3883dc3 into sablier-labs:main Oct 26, 2023
10 of 11 checks passed
@PaulRBerg
Copy link
Member

PaulRBerg commented Oct 26, 2023

Thanks again for your contribution, @scorpion9979!

@andreivladbrg did you look over the Slither report to check whether it reported something that we should address?

@scorpion9979 scorpion9979 deleted the ci/slither branch October 26, 2023 12:49
@andreivladbrg
Copy link
Member

did you look over the Slither report to check whether it reported something that we should address?

Yeah, I did and found nothing that we should address. You can see the report here.

@PaulRBerg
Copy link
Member

Great.

You can see the report here.

Could you please mark all items in the code scan report as "False positive", "Used in tests", or "Won't fix"?

SCR-20231027-omso

andreivladbrg added a commit that referenced this pull request Dec 15, 2023
* ci: add slither job
chore: fix solc remap path in slither config

* ci: use cached build for slither job

* ci: use solc 0.8.21 in slither job

* ci: revise slither job
revert: ci: use cached build for slither job

* ci: remove superfluous "solc-version" config in slither job not required when a compilation framework is used

---------

Co-authored-by: andreivladbrg <[email protected]>
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