core/types: fix discrepancy in receipt.EffectiveGasPrice json encoding tags #27114
Conversation
roberto-bayardo
requested review from
karalabe,
holiman and
rjl493456442
as code owners
|
cc @fjl |
|
I don't like adding |
Makes sense. Would leaving it as neither required nor omit empty make most sense then? That's how it was in the schema before, but the generated code still had omit empty for reasons I don't entirely understand. |
roberto-bayardo
force-pushed
the
require-effective-gas-price
branch
from
8564075
to
9ee1c33
Compare
|
Hmm. I dropped the required tag from the schema, replacing with a comment. But the regenerated gen_receipt_json.go didn't change (other than the tag disappearing). Let me confirm it will not throw errors if the field is missing. [edit] code didn't change because I had a typo in the tag originally ("gencode" instead of "gencodec" 🤦). Have confirmed that leaving it as neither required nor omitempty results in a generated parser that is tolerant of the missing field, but always puts it in the output. Alternative would be to just explicitly tag it as omitempty in the schema and leave the comment around having it this way for backwards compatibility. This way there's no more discrepancy between the schema in receipt.go and the generated code. |
roberto-bayardo
changed the title
|
This PR now leaves the field as untagged in the receipt.go schema for backwards compatibility, but adds a comment clarifying it's still technically required, removes the omitempty discrepancy in the generated code, and adds/updates test cases to codify the untagged behavior of the field. Updated the PR description accordingly, and also provided a list of the alternatives considered (tag as required, tag as omitempty). |
update schema to clarify while it's required we don't tag it as such for backwards compatibility. Update tests which were treating it as omit empty.
roberto-bayardo
force-pushed
the
require-effective-gas-price
branch
from
ef371fb
to
59cc387
Compare
holiman
changed the title
lightclient
force-pushed
the
require-effective-gas-price
branch
from
fe4b700
to
5c338d4
Compare
the execution api spec suggests receipt.EffectiveGasPrice should be required, but it's currently not tagged as required in its receipt.go schema. Further, the field is tagged as 'omitempty' in the generated code even though it's not tagged as such in the schema.
This PR leaves the field as untagged in the receipt.go schema for backwards compatibility, adds a comment clarifying it's still technically required, removes the omitempty discrepancy in the generated code, and adds/updates test cases to codify the untagged behavior of the field.
Alternatives considered:
tag as required in schema + generated code. Dismissed due to backwards compatilbity concerns as it could break interop w/ older servers that do not require the field.
tag as omitempty in both schema + generated code. I don't have a strong reason yet to exclude this alternative, though I don't have a reason to prefer it either.