-
Notifications
You must be signed in to change notification settings - Fork 93
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
Optional Chain ID in Baseline Solver Configuration #1207
Conversation
#[serde(flatten)] | ||
pub contracts: ContractsConfig, |
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.
Now we're mixing deny_unknown_fields and flatten which you mentioned before doesn't work. deny_unknown_fields has already been pretty helpful for me, but this is a small config file so doesn't really matter. More generally, I think not using flatten is nice because it makes the structure of the config file more obvious from the code. I like keeping a one-to-one correspondence of rust field to config field. With complicated serde instructions it tends to get harder to figure out how the serialized file is going to look. Again, this file is small so not super important, so up to you.
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.
Now we're mixing deny_unknown_fields and flatten which you mentioned before doesn't work
It doesn't work well. deny_unknown_fields
and flatten
together have a whole bunch of prickly issues:
- If you flatten a struct that also has
deny_unknown_fields
, then this won't work (the inner struct will see a whole bunch of fields from the parent and deny them). - If you
deny_unknown_fields
on untagged enums, then this also doesn't work
However, in this particular case of deny_unknown_fields
, flatten
and rename
on an enum with only single values, it does. I tested it out locally, but you can also see an example of this in the playground.
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.
So, I changed it to use a manual match (chain_id, weth)
. The reasoning is:
- As you mentioned complicated
serde
instructions make things harder to figure out, also the whole complex interaction betweendeny_unknown_fields
,flatten
andrename_all
is not obvious (for example, if you don't use therename_all
, then serialization will not work as expected) - You get nicer panic error messages
@@ -1,4 +1,5 @@ | |||
chain-id = "1" | |||
weth = "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2" | |||
# Alternatively, you can manually specify a WETH contract address: |
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.
nit: Could you make the comment refer to the chain-id
parameter? This makes the dependency more explicit and less error prone if somebody where to insert a new argument between chain-id
and weth
.
@vkgnosis pointed out in #1202 that the chain ID parameter being mandatory is not practical for local deployments or when testing out on new chains.
In fact, the baseline solver only needs to know the WETH address, so this PR changes the configuration semantics to require either:
chain-id
to use the canonical WETH contract for that chain.weth
address to use that instead, at which point theChainId
is no longer neededTest Plan
Adjusted test loading the example TOML configuration.