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

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

Closed
2 tasks done
0xalpharush opened this issue Feb 5, 2024 · 7 comments · Fixed by #9131
Closed
2 tasks done
Assignees
Labels
A-config Area: config C-forge Command: forge T-bug Type: bug
Milestone

Comments

@0xalpharush
Copy link
Contributor

0xalpharush commented Feb 5, 2024

Component

Forge, Chisel

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

  • Foundry
  • Foundryup

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

evm_version: EvmVersion::Paris,

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.
// We now need to verify if the solc version provided is supported by the evm
// version set. If not, we bail and ask the user to provide a newer version.
// 1. Do we need solc 0.8.18 or higher?
let evm_version = self.foundry_config.evm_version;
let needs_post_merge_solc = evm_version >= EvmVersion::Paris;
// 2. Check if the version provided is less than 0.8.18 and bail,
// or leave it as-is if we don't need a post merge solc version or the version we
// have is good enough.
let v = if needs_post_merge_solc && version < Version::new(0, 8, 18) {
eyre::bail!("solc {version} is not supported by the set evm version: {evm_version}. Please install and use a version of solc higher or equal to 0.8.18.
You can also set the solc version in your foundry.toml.")
} else {
version.to_string()
};

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

For more info, see also crytic/slither#2287

@0xalpharush 0xalpharush added the T-bug Type: bug label Feb 5, 2024
@gakonst gakonst added this to Foundry Feb 5, 2024
@github-project-automation github-project-automation bot moved this to Todo in Foundry Feb 5, 2024
@mattsse
Copy link
Member

mattsse commented Feb 5, 2024

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

@0xalpharush
Copy link
Contributor Author

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
Copy link
Member

mattsse commented Feb 6, 2024

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

@0xalpharush
Copy link
Contributor Author

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

@mattsse
Copy link
Member

mattsse commented Feb 7, 2024

should I add a normalization step specifically for forge config?

@0xalpharush
Copy link
Contributor Author

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

@zerosnacks zerosnacks changed the title forge config --json returns evm version that is not compatible with solc version bug: forge config --json returns evm version that is not compatible with solc version Jul 10, 2024
@zerosnacks zerosnacks added A-config Area: config C-forge Command: forge labels Jul 10, 2024
@zerosnacks
Copy link
Member

zerosnacks commented Jul 10, 2024

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: crytic/slither#2422 (comment)

@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@grandizzy grandizzy self-assigned this Oct 24, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Nov 18, 2024
@grandizzy grandizzy moved this from Done to Completed in Foundry Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-config Area: config C-forge Command: forge T-bug Type: bug
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

4 participants