Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

fix(core): fix decimal string IntOrHex parsing #2359

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

Rjected
Copy link
Contributor

@Rjected Rjected commented Apr 18, 2023

Motivation

Previously, we would parse decimal strings like "100000" as hex, because they did not match a raw serde_json::Number variant in IntOrHex. This caused problems when parsing genesis fields with from_int_or_hex, for example:

            "alloc": {
                "0x3E951C9f69a06Bc3AD71fF7358DbC56bEd94b9F2": {
                  "balance": "1000000000000000000000000000"
                },
             }

This would parse "1000000000000000000000000000" instead as 0x1000000000000000000000000000.

This did not fail U256 parsing because its FromStr implementation accepts both 0x-prefixed and non-0x-prefixed strings, and will deserialize both as hex.

Solution

Check for 0x prefixes in from_int_or_hex and from_u64_or_hex if the value matches a String instead of serde_json::Number. If there is a prefix, deserialize as hex. Otherwise, the value deserializes as a decimal string.

Tests are added for the failing Genesis alloc, and unit tests for the from_int_or_hex method itself are added.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines -523 to +529
match IntOrHex::deserialize(deserializer)? {
IntOrHex::Hex(s) => U256::from_str(s.as_str()).map_err(serde::de::Error::custom),
IntOrHex::Int(n) => U256::from_dec_str(&n.to_string()).map_err(serde::de::Error::custom),
match IntOrString::deserialize(deserializer)? {
IntOrString::JsonString(s) => {
if s.starts_with("0x") {
U256::from_str(s.as_str()).map_err(serde::de::Error::custom)
} else {
U256::from_dec_str(&s).map_err(serde::de::Error::custom)
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @prestwich for ethers-next, you wouldn't believe how much time pinpointing this took us

@gakonst gakonst merged commit 80eac38 into gakonst:master Apr 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants