-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix(config): prints error when accessing invalid etherscan config #9951
base: master
Are you sure you want to change the base?
fix(config): prints error when accessing invalid etherscan config #9951
Conversation
crates/config/src/etherscan.rs
Outdated
match config { | ||
Ok(c) if c.chain == Some(chain) => return Some(Ok(c)), | ||
Err(e) => return Some(Err(e)), | ||
Err(e) => { | ||
warn!("invalid etherscan config for {:?}: {:?}", name, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @andelf thanks for your PR!
Would you mind changing warn!
to let _ = sh_warn!(..)
This will make it so the warning is outputted to the user's terminal rather than to the tracing
target for internal debugging
Would you mind adding a unit test that validates the change?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's deeply coupled with other config testings. So I guess it would be better to refuse invalid config and output errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments addressed. PTAL.
982c0dc
to
c2e14b3
Compare
Perhaps I'm missing something but running the example case in your description with the current version of Foundry results in:
or alternatively if the
https://book.getfoundry.sh/reference/config/etherscan specifically states the ability to specify custom chains (even if incorrect) and will throw this error Would you mind giving a brief example of the issue that produces the reported error? |
@zerosnacks It's counterintuitive that another invalid name in conditions:
|
@@ -127,12 +127,10 @@ impl StorageArgs { | |||
} | |||
} | |||
|
|||
if !self.etherscan.has_key() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check does not match the later api_key assignment.
In my case, this check fails, but config.get_etherscan_api_key works.
bdff14b
to
544d035
Compare
Motivation
If any
[etherscan]
config entry has a wrong name, then the whole[etherscan]
becomes unreachable.This is difficult to debug since the only error message is "Error: Invalid API Key".
Actually user has already provided a correct key.
To reproduce, add the following to
~/.foundry/foundry.toml
:Then all etherscan operations becomes "Invalid API Key" errors.
Solution
Skip invalid entry in-- the config unit tests depend on this behaviourResolvedEtherscanConfig
and prints error log.[etherscan]
section is invalidPR Checklist