Repository navigation
standard-json: check a the sources keccak256 - #677
Conversation
Signed-off-by: xermicus <cyrill@parity.io>
| /// The settings `solc` assumes when the optimizer is omitted. | ||
| pub fn solc_default() -> Self { | ||
| Self::new(false, Self::default_mode(), Details::default()) | ||
| } |
There was a problem hiding this comment.
I'd argue we should change this approach a bit (whether in this PR or another one). I don't think this crate should have to own the solc defaults, but rather if a key is optional and is omitted, then we also omit it when passing the input to solc. This way solc's defaults will always be used, instead of us having to keep it in sync.
Depending on the solc version, solc also sometimes treats an omitted key differently from an explicit false value (e.g. before 0.8.6, "settings.optimizer.enabled": false was not the same as omitting it).
Keeping fields omitted if the user also omitted them is how the other Settings fields currently work, so I think we should update this to be the same for the optimizer. (This would also handle this case: "settings.optimizer": {}, which otherwise breaks).
Something like this (plus whatever call sites need updating):
crates/solc-json-interface/src/standard_json/input/settings/optimizer/mod.rs:
pub struct Optimizer {
/// Whether the optimizer is enabled.
- pub enabled: bool,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub enabled: Option<bool>,
...
}
impl Optimizer {
/// A shortcut constructor.
pub fn new(enabled: bool, mode: char, details: Details) -> Self {
Self {
- enabled,
+ enabled: Some(enabled),
mode,
details,
}
}
...
- /// The settings `solc` assumes when the optimizer is omitted.
- pub fn solc_default() -> Self {
- Self::new(false, Self::default_mode(), Details::default())
- }
}crates/solc-json-interface/src/standard_json/input/settings/mod.rs:
pub struct Settings {
...
- #[serde(default = "Optimizer::solc_default")]
- pub optimizer: Optimizer,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub optimizer: Option<Optimizer>,
...
}The rest of the PR LGTM though.
There was a problem hiding this comment.
Yeah I think that's the right call.
Closes #663