-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(config): support numeric keys in [fork.<chain>]
section
#11340
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
Conversation
…oundry into rusowsky/extend-fork-cheats
…oundry into rusowsky/extend-fork-cheats
@grandizzy i made the error more explicit |
One possible issue is that users may want to also be able to use the I would be in favor of merging the current proposal and waiting for community feedback on it and address this then. |
@zerosnacks should i not error if the rpc chains are not supported by alloy chains then? |
What would the side effect of not error-ing be? |
i was using alloy-chains to convert between names and ids. If they provide a non-supported chain, it could work as long as the chain key is a uint (the id). i'd have to adjust the cheatcode impl slightly though, as i was operating on the premise that they would always be alloy chains (not a big deal) |
Ah nice, I think this would be preferable as an end user and worth implementing |
crates/config/src/fork_config.rs
Outdated
// Determine the canonical key for this entry | ||
let canonical_key = if let Ok(chain_id) = key.parse::<u64>() { | ||
if let Some(named) = alloy_chains::Chain::from_id(chain_id).named() { | ||
named.as_str().to_string() |
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.
any reason we can't fall back to just integers if they are not supported in alloy_chains
?
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.
yup, that's what we were discussing above.
addressed with 7ccb5d9
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.
Looks good! Pending @grandizzy / @onbjerg
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.
lgtm
Motivation
teams that automate the setup of multi-chain configs, will benefit from being able to use the chain id rather than the chain name.
Solution
normalize the chain key in
fork.<chain>
to become the canonical name defined byalloy-chains
. This keeps the UX of named chains, and also adds support for ids