crytic / slither-action

GNU Affero General Public License v3.0
127 stars 19 forks source link

feat(entrypoint): detect hardhat solc version #43

Closed vzctl closed 1 year ago

vzctl commented 1 year ago

Hi,

This feature adds solc version detection support using hardhat config. It takes priority over pragma solidity grepping.

We are deploying slither-action across many solidity contracts - most of them are using hardhat and fetching version information from there is much more robust.

elopez commented 1 year ago

Hi @vzctl! Thanks for the PR. Can you provide a bit of context on why this is needed? Neither the solc-version option nor the auto-detection has any impact when the target is using hardhat -- as hardhat is used to do the contract compilation, and hardhat does not use the solc binary installed by solc-select.

vzctl commented 1 year ago

Hi!

Here is an error I get without this hardhat-detection feature:

[-] SOLCVER was not set; guessing.
[-] Guessed 0.8.
usage: solc-select install [-h] [INSTALL_VERSIONS ...]
solc-select install: error: argument INSTALL_VERSIONS: Invalid version '0.8'.

This is triggered because install_solc function execution is unconditional and the guessing is done incorrectly.

My use case is that I need to run a shared Slither workflow for multiple contracts across our org.

elopez commented 1 year ago

You should be able to specify a valid solc version with the solc-version option in the action. As you're using hardhat it will have no real effect in the compilation, but it will disable the guessing.

We should probably tighten the guessing code to only guess valid versions (the one it guessed here is missing the patch version component)

vzctl commented 1 year ago

That is correct. However, I am using a shared workflow that is configured by multiple teams of developers and asking every one of them to specify solc-version one more time (they are already specifying it in the hardhat configs) would introduce too much friction and would be quite redundant.

elopez commented 1 year ago

But you can just set, say, solc-version: 0.8.0 on your main workflow, without asking developers to change anything else 🤔 -- the solc binary is not going to be called by anything here if you're using hardhat, so it doesn't matter if it's not the same version as the one set in hardhat or anywhere else.

I agree it's redundant though, fixing the detection to try valid things only and/or having a way to turn it off would be nice.

vzctl commented 1 year ago

To give you even more context:

The shared workflow does not know what build system the individual contracts are using. Because of the points above I can't hardcode any versions in the shared workflow.

Please let me know if you have a better idea how to move forward!

elopez commented 1 year ago

But are any of the teams using just plain .sol files in a folder? That's the only case in where solc-version "correctness" really has any impact. When you're using a framework (like truffle, hardhat, or foundry) the framework takes care of getting the correct compilers it needs, so having a placeholder solc-version value has no impact in that case.

Personally I'd prefer to remove the guessing code and only install solc if a user passes solc-version explicitly, but it'd be a breaking change. Fixing the guessing logic to only proceed if it finds a sensible value and to not "hard-fail" if it doesn't would be a nicer middle ground, what do you think?

vzctl commented 1 year ago

I see, thanks for the detailed explanation - super helpful! I just tested an incorrectly hardcoded solc-version with hardhat and it behaved exactly like you described.

I will reopen this in case I encounter situations where we need to test individual .sol files inside of a hardhat project without compiling them using hardhat itself - which seems very unlikely.