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

feat(ci): run slither in main workflow #8

Merged
merged 25 commits into from
Jun 26, 2024
Merged

Conversation

sripwoud
Copy link
Member

@sripwoud sripwoud commented Jun 24, 2024

Integrate slither via its GH action into the main.yml workflow:

  • write a comment in the PR informing the results will be available in the repo's security section
  • upload SARIF report to GH (code scanning support)
  • worklfow optimizations (sorry, this should have deserved its own PR ^^)
    • build deps in parallel and share node_modules between jobs
    • tests and slither jobs run in parallel
    • tests will only run for the impacted packages
    • slither will only run for the impacted packages
    • compilation happens in a parallel job and the compilation results are shared with the other jobs using the actions/upload-artifact and actions/download-artifact
      image
      image

Closes #7 ?

Notes

As this PR does not change any files that trigger the slither workflow, I tested it in another branch, see #9

@sripwoud sripwoud requested a review from cedoor as a code owner June 24, 2024 14:48
@sripwoud sripwoud self-assigned this Jun 24, 2024
@sripwoud sripwoud marked this pull request as draft June 24, 2024 19:24
@sripwoud sripwoud force-pushed the feat/slither-workflow branch 10 times, most recently from 9a5cf34 to 47070ce Compare June 24, 2024 20:35
@sripwoud sripwoud force-pushed the feat/slither-workflow branch 2 times, most recently from 3cab4e0 to 3f2e629 Compare June 24, 2024 20:43
@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.

@sripwoud sripwoud marked this pull request as draft June 25, 2024 09:46
@sripwoud sripwoud changed the title ci: run slither in main workflow feat(ci): run slither in main workflow Jun 25, 2024
@sripwoud sripwoud marked this pull request as ready for review June 25, 2024 15:21
@sripwoud
Copy link
Member Author

sripwoud commented Jun 25, 2024

I invite the contracts authors to check the slither alerts in the code scanning section:
https://github.com/privacy-scaling-explorations/zk-kit.solidity/security/code-scanning?query=pr%3A8

@0xjei @cedoor @vimwitch @LCamel @vplasencia

pull_request:

concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
cancel-in-progress: true

jobs:
style:
deps:
Copy link
Member

Choose a reason for hiding this comment

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

How does this job work exactly?

Looks like the workflow doesn't install dependencies until there's some change in the yarn.lock file right?

So, I imagine the other jobs like tests, style and compile should be quite faster right?

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding is that job contexts are isolated.
So when you do e.g

job:
  steps:
    - uses: actions/setup-node@v4
      with:
        cache: yarn

It only caches deps in this particular job.
As the workflow is running several jobs that depends on the same lock file (we are in a monorepo), I thought it is redundant to run yarn in all jobs and was looking for a way to share the node_modules content between jobs.
One solution is to use directly actions/cache (actions/setup-node cache feature uses it under the hood).
It should speed up the whole workflow but I haven't measured it.

Copy link
Member

Choose a reason for hiding this comment

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

I see! Looks great and we should definitely re-use it in the other repos imo.

.github/workflows/main.yml Show resolved Hide resolved
steps:
- uses: actions/checkout@v4

# FIXME this does not work as a way to restore compilation results for slither job but it does for the compile job ??
Copy link
Member

Choose a reason for hiding this comment

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

This is not clear. Were you trying to re-use the already generated artifacts here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I could reuse the artifacts generated from the compile job yes.
Like I am successfully doing in the tests job. see your previous question
#8 (comment)

But somehow I had errors with slither if using actions/dowload-artifact to get the contracts compilation artifacts. So I am compiling again here (not a big deal, just less efficient/ slower).
I can't find an example failing workflow. So let me test again.

Copy link
Member

Choose a reason for hiding this comment

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

I see! If it doesn't work it's still fine agree.

Copy link
Member

@cedoor cedoor left a comment

Choose a reason for hiding this comment

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

Great work @sripwoud

I think some optimizations can be re-implemented in the other repos 👍🏽

@sripwoud sripwoud merged commit cdeb8e4 into main Jun 26, 2024
7 checks passed
@sripwoud sripwoud deleted the feat/slither-workflow branch June 26, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✔️ Done
Development

Successfully merging this pull request may close these issues.

Support Slither, a new static analyzer for Solidity
2 participants