Skip to content

Conversation

zugdev
Copy link
Contributor

@zugdev zugdev commented Aug 17, 2025

Motivation

#9939 is a feature request for --solc-version as a CLI argument for cast storage. This is my first PR, please be as strict as possible so I can learn.

Solution

User can override the detected version by passing cast storage --solc-version <version>, I've added a conditional in case the argument is passed.

I have a few observations:

  1. It's not clear by spec, whether it should be a global cast argument
  2. Should I forbid the user from passing a version older than MIN_SOLC or an invalid compiler version for that artifact? The warning and error message are already displayed in each case.
  3. Should I add a test set?

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@zugdev
Copy link
Contributor Author

zugdev commented Aug 28, 2025

bumping for feedback!

@grandizzy
Copy link
Collaborator

grandizzy commented Aug 28, 2025

thank you!

It's not clear by spec, whether it should be a global cast argument

IMO storage related should be fine, we can extend if needed on other commands

Should I forbid the user from passing a version older than MIN_SOLC or an invalid compiler version for that artifact?

The warning and error message are already displayed in each case.
Nope, don't think we should

@zerosnacks wdyt on points above?

Should I add a test set?

Yes, please add a test for this, there are samples in cast.rs

@zugdev
Copy link
Contributor Author

zugdev commented Sep 2, 2025

IMO storage related should be fine, we can extend if needed on other commands

This makes sense.

The warning and error message are already displayed in each case. Nope, don't think we should

I agree.

Yes, please add a test for this, there are samples in cast.rs

I didn't find a file named cast.rs but I checked others for context (e.g. cast/cmd/call.rs and tests/cli/main.rs).

My test set includes tests

  1. to ensure solc version is correctly unwrapped;
  2. to assert success if a valid version is provided;
  3. to assert failure, and the appropriate error message, if the provided version isn't valid.

Please, feel free to tag me with requests and feedback.

@zugdev
Copy link
Contributor Author

zugdev commented Sep 13, 2025

Bumping! i don't mean to be annoying, I just want to work on other issues and I can make better PRs after this first review.

@onbjerg onbjerg self-assigned this Sep 15, 2025
@zugdev zugdev requested a review from onbjerg September 15, 2025 21:40
Copy link
Contributor

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

one last thing, otherwise a rebase and it lgtm :)

Comment on lines 168 to 176
project.compiler = {
if let Some(solc_req) = self.solc_version {
SolcCompiler::Specific(Solc::find_or_install(&solc_req)?)
} else if auto_detect {
SolcCompiler::AutoDetect
} else {
} else {
SolcCompiler::Specific(Solc::find_or_install(&version)?)
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't formatted correctly, just run cargo +nightly fmt --all and it will fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, sorry :o

@onbjerg
Copy link
Contributor

onbjerg commented Sep 19, 2025

Hi! @zugdev still missing a rebase :D Sorry about that

@zugdev
Copy link
Contributor Author

zugdev commented Sep 19, 2025

Hi! @zugdev still missing a rebase :D Sorry about that

My bad will do it in a few minutes.

@zugdev
Copy link
Contributor Author

zugdev commented Sep 19, 2025

pls give me a day no bandwidth today

@zugdev
Copy link
Contributor Author

zugdev commented Sep 21, 2025

Hey! I had to rewrite a little bit to merge with some nice changes @DaniPopes introduced. Let me know about anything.

@grandizzy grandizzy moved this to In Progress in Foundry Sep 22, 2025
@onbjerg onbjerg merged commit a70e6c0 into foundry-rs:master Sep 22, 2025
39 of 48 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Foundry Sep 22, 2025
@grandizzy grandizzy moved this from Done to Completed in Foundry Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

3 participants