-
Notifications
You must be signed in to change notification settings - Fork 390
Stabilize ChainConfig serde for Human-Readable & Binary Formats #2436
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
base: main
Are you sure you want to change the base?
Conversation
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.
removing hex support will break existing setups, so we cant remove this
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.
Only for JSON files that contain hex, which neither alloy nor geth ever produce. Do you still think we should support hex deserialization in human-readable input?
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.
Arguably, using alloy-serde::opt
also for serialization, would be an even more breaking change.
So the only alternative would be to add a alloy-genesis specific deserializer, which uses default when non-human-readable and alloy_serde::quantity::opt::deserialize when human-readable.
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.
Addressed in 823a16b
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 should remain unchanged to ensure this change is not breaking
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.
I'm not sure I really understand why this would break, but I've reverted this change: d6521f1
crates/serde/src/ttd.rs
Outdated
Some(value) => { | ||
// convert into an u128 when possible | ||
let number = value.try_into().map_err(ser::Error::custom)?; | ||
serializer.serialize_u128(number) |
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 does not work for json, the entire reason for this workaround is that this does not work for mainnet TTD because does not fit in json number without enabling arb precision, whcih we shoudl not do
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.
It does work with serde_json, and I've also added a tests for that.
Serde has (limited) support for u128, even when the arbitrary precision feature is not enabled. However, there is no support for deserializing u128 inside an untagged enum: serde-rs/serde#2230
And as I understand the comments, the workaround has been implemented to support exactly untagged enums. (I even added a test for this particular case).
If we don't need to consider untagged enums (which are irrelevant for ChainConfig), we could even get rid of the workaround altogether and use u128.
@mattsse I changed it so that the deserialization is now 100% identical to and compatible with the existing version. |
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.
I'm a bit confused why we need to change this via the deserde impls
because this still keeps only deserialize_with = "deserialize_u64_opt"
which doesn't seem symmetrical to me
wouldn't this still deserialize hex and serialize u64?
crates/genesis/src/lib.rs
Outdated
if deserializer.is_human_readable() { | ||
match Option::<String>::deserialize(deserializer)? { | ||
Some(ref s) => { | ||
if s == "0x" { | ||
return Ok(None); | ||
} | ||
B256::from_str(s).map(Some).map_err(D::Error::custom) | ||
} | ||
None => Ok(None), | ||
} | ||
B256::from_str(s).map(Some).map_err(D::Error::custom) | ||
} else { | ||
Ok(None) | ||
Option::<B256>::deserialize(deserializer) | ||
} |
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.
hmm, why we need to change this here, is this because the non human readable impl for b256 is different?
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.
In binary, a B256 will not be serialized as a string, but as 32 bytes. Thus in a roundtrip, the Option::deserialize would fail. It only works for human-readable where it is serialized as a hex string.
oh I think I get it now |
I think it would conceptually be cleaner (symmetry and Geth compatibility) to only support parsing JSON numbers and not also hex strings. |
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.
okay, I think this makes sense, I only have one last question about removed test cases and one nit
cool, I think this is good now |
|
||
/// Custom deserialization function for `Option<u64>`. | ||
/// | ||
/// This function allows it to be deserialized form a number or a "quantity" hex 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.
There's a small typo in the docstring: form a number
should be from a number
.
/// This function allows it to be deserialized form a number or a "quantity" hex string. | |
/// This function allows it to be deserialized from a number or a "quantity" hex string. |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Motivation
This PR refactors Genesis serde logic to fix roundtrip errors and ensure a clear distinction between human-readable (JSON) and binary formats.
Fixes #2435
Solution
This PR implements the following changes:
deserialize_private_key
,deserialize_storage_map
, and introduced thettd
module. All now useis_human_readable()
to ensure symmetric (de)serialization:u128
) in human-readable contexts and via standard U256 serde for binary formats.deserialize_json_ttd_opt
is kept for backward compatibility.ChainConfig
now also useis_human_readable()
to ensure symmetric (de)serialization:With this PR it is now possible to (de)serialize
Genesis
using binary formats (such as serde_cbor). For JSON, the deserialization behavior remains completely unchanged to preserve backward compatibility, only for serialization it fixes the issue that theterminal_total_difficulty
was not serialized as a number.PR Checklist