search

foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
8.03k stars 1.64k forks source link

bug: `forge config --json` returns evm version that is not compatible with solc version #7014

Open 0xalpharush opened 6 months ago

0xalpharush commented 6 months ago

Component

Forge, Chisel

Have you ensured that all of these are up to date?

What version of Foundry are you on?

forge 0.2.0 (84d9842 2024-02-02T00:19:34.098872000Z)

What command(s) is the bug in?

forge config --json

Operating System

macOS (Apple Silicon)

Describe the bug

When the EVM version isn't set in the foundry.toml but the solc version is, Foundry may incorrectly use EvmVersion::Paris without validating that it is available for the given solc verison e.g. solc 0.8.17. I would expect to be able to pass the output of forge config --json to solc without running into errors related solc configurations. I'd also expect incompatibilities that are explicitly defined in the foundry.tom to be reported in a diagnostic.

I looked into fixing this but I'm not familiar with the figment provider library and wasn't sure how to add a call to normalize_version that will produce a correct solc version and evm version combo cleanly (I assume someone with more knowledge of the toolchain and architecture can consolidate the validation and error handling better than myself).

Relevant code for forge config https://github.com/foundry-rs/foundry/blob/7922fd5482f9561699e0fe5a903c90b3fa1fc50d/crates/config/src/lib.rs#L1774 Relevant code for chisel which I believe also performs insufficient validation i.e it can produce other misconfigurations for solc version and evm version pairs besides 0.8.17 and Paris. https://github.com/foundry-rs/foundry/blob/7922fd5482f9561699e0fe5a903c90b3fa1fc50d/crates/chisel/src/session_source.rs#L108-L121

Also, probably here and here could use the same fix.

For more info, see also https://github.com/crytic/slither/issues/2287

mattsse commented 6 months ago

this only affects the config part, right? because before invoking solc this should always get normalized

0xalpharush commented 6 months ago

I only experienced the bug in forge config. The rest of these are just areas where it doesn't seem to be normalized like it is when creating the standard json input for solc here. I'm not sure how the configuration works for chisel in order to test it

mattsse commented 6 months ago

@0xalpharush Do you have any suggestions on how to improve the chisel version check?

0xalpharush commented 6 months ago

I think we also need this validation when auto_detect_solc is used (default IIRC)

mattsse commented 6 months ago

should I add a normalization step specifically for forge config?

0xalpharush commented 6 months ago

Yes, I think it is taken care of for forge build and forge test already

zerosnacks commented 1 month ago

Note: looks like this is still an active issue - for future reference

When no solc_version has been set it yields:

{
  "evm_version": "paris",
}

With solc_version set to 0.8.15 in foundry.toml it correctly yields:

{
  "evm_version": "london",
}

Related: https://github.com/crytic/slither/issues/2422#issuecomment-2054159134