Skip to content

Conversation

vzctl
Copy link
Contributor

@vzctl vzctl commented Aug 3, 2022

It is a common practice to set ./dist as an outDir compilation option in tsconfig.json (see e.g. here or here).

It is also unlikely that dist folder would contain non-autogenerated files.

We are reusing dist as an output destination for autogerenated, hardhat-flattened contracts as well and would like to exclude them from the analysis for solc-detection since flattened contracts also include all the dependencies, similarly to node_modules.

There are two ways of solving the problem:

  • a simple fix to add dist together with node_modules (proposed in this PR)
  • add another configuration option to allow this list of excludes to be user-defined.

Exclude dist folder from the grep
@CLAassistant
Copy link

CLAassistant commented Aug 3, 2022

CLA assistant check
All committers have signed the CLA.

@elopez
Copy link
Member

elopez commented Aug 16, 2022

Hi @vzctl, thanks for the PR! Could you provide a bit more context around the change, like which kind of projects have a dist folder, and why is it best to ignore solidity code in it?

@vzctl
Copy link
Contributor Author

vzctl commented Aug 16, 2022

Hi! Thanks, updated the PR description with more context.

@elopez elopez changed the base branch from main to dev September 5, 2022 20:42
@elopez
Copy link
Member

elopez commented Sep 5, 2022

Sounds good, thanks! We can add dist here. Keep in mind this is meant to cover the simpler use cases though (target being a single .sol file or a folder with standalone .sol files). Projects in where this "heuristic" fails can set solc-version in the action and skip the detection altogether. Projects using a framework (eg hardhat) handle their own solidity versioning and don't need one installed beforehand, so the detection is a bit pointless there too.

@elopez elopez merged commit 481d27b into crytic:dev Sep 5, 2022
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