-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
Closes #11063 This PR makes the `eip1108 fields` as `Option` such that serde deserialization succeeds without being specified. Also a `json file` is added to test it.
self.eip1108_transition_price.into() | ||
} else { | ||
self.price.into() | ||
match (self.eip1108_transition_at, self.eip1108_transition_price) { |
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 became a little more complex unfortunately
However, it will only enable the eip
iff both fields are specified in the chain spec. Do we want a warning/trace if only one field is specified (probably a bug in the case)?
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.
Also this two matches are not DRY
but it would require a trait object or enum such as Eip1108
to replace eip_transition_at
and eip_price
(price types will be different). I prefer not to do because most of this will be fixed by #11039
@niklasad1 could you elaborate on how this relates to #11039? Would this be a spec breaking change and #11039 not, just refactoring the internal impl? |
#11008 was an unintentional breaking change this PR is just a hack to fix it. #11039 is re-write of this mess to handle different prices and price models in more Thoughts? |
We probably want to backport one future-proof change, ideally backwards (with 2.5.7 format) compatible. So I would propose to put this PR on hold until we see what would the accepted pricing formats in #11039 look like. |
Yeah, agree. |
Closes #11063
This PR makes the
eip1108 fields
asOption
such that serde deserialization succeeds without being specified.Also a
json file
is added to test it.