Skip to content
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

feat(primitive-types, serde): deserialize hex strings without 0x prefix #598

Merged
2 changes: 1 addition & 1 deletion ethereum-types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ ethbloom = { path = "../ethbloom", version = "0.11", default-features = false }
fixed-hash = { path = "../fixed-hash", version = "0.7", default-features = false, features = ["byteorder", "rustc-hex"] }
uint-crate = { path = "../uint", package = "uint", version = "0.9", default-features = false }
primitive-types = { path = "../primitive-types", version = "0.10", features = ["byteorder", "rustc-hex"], default-features = false }
impl-serde = { path = "../primitive-types/impls/serde", version = "0.3.0", default-features = false, optional = true }
impl-serde = { path = "../primitive-types/impls/serde", version = "0.3.2", default-features = false, optional = true }
impl-rlp = { path = "../primitive-types/impls/rlp", version = "0.3", default-features = false, optional = true }
impl-codec = { version = "0.5.0", path = "../primitive-types/impls/codec", default-features = false, optional = true }
scale-info = { version = "1.0", features = ["derive"], default-features = false, optional = true }
Expand Down
21 changes: 19 additions & 2 deletions ethereum-types/src/uint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,29 @@ mod tests {
assert_eq!(number, ser::from_str(&format!("{:?}", expected)).unwrap());
}

let tests = vec![
($name::from(0), "0"),
($name::from(1), "1"),
($name::from(2), "2"),
($name::from(10), "a"),
($name::from(15), "f"),
($name::from(15), "f"),
($name::from(16), "10"),
($name::from(1_000), "3e8"),
($name::from(100_000), "186a0"),
($name::from(u64::max_value()), "ffffffffffffffff"),
($name::from(u64::max_value()) + 1, "10000000000000000"),
];

for (number, expected) in tests {
assert_eq!(format!("{:?}", "0x".to_string() + expected), ser::to_string_pretty(&number).unwrap());
assert_eq!(number, ser::from_str(&format!("{:?}", expected)).unwrap());
}

// Invalid examples
assert!(ser::from_str::<$name>("\"0x\"").unwrap_err().is_data());
assert!(ser::from_str::<$name>("\"0xg\"").unwrap_err().is_data());
assert!(ser::from_str::<$name>("\"\"").unwrap_err().is_data());
assert!(ser::from_str::<$name>("\"10\"").unwrap_err().is_data());
assert!(ser::from_str::<$name>("\"0\"").unwrap_err().is_data());
}
};
}
Expand Down
30 changes: 27 additions & 3 deletions ethereum-types/tests/serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,29 @@ macro_rules! test {
assert_eq!(number, ser::from_str(&format!("{:?}", expected)).unwrap());
}

let tests = vec![
($name::from(0), "0"),
($name::from(1), "1"),
($name::from(2), "2"),
($name::from(10), "a"),
($name::from(15), "f"),
($name::from(15), "f"),
($name::from(16), "10"),
($name::from(1_000), "3e8"),
($name::from(100_000), "186a0"),
($name::from(u64::max_value()), "ffffffffffffffff"),
($name::from(u64::max_value()) + $name::from(1u64), "10000000000000000"),
];

for (number, expected) in tests {
assert_eq!(format!("{:?}", "0x".to_string() + expected), ser::to_string_pretty(&number).unwrap());
assert_eq!(number, ser::from_str(&format!("{:?}", expected)).unwrap());
}

// Invalid examples
assert!(ser::from_str::<$name>("\"0x\"").unwrap_err().is_data());
assert!(ser::from_str::<$name>("\"0xg\"").unwrap_err().is_data());
assert!(ser::from_str::<$name>("\"\"").unwrap_err().is_data());
assert!(ser::from_str::<$name>("\"10\"").unwrap_err().is_data());
assert!(ser::from_str::<$name>("\"0\"").unwrap_err().is_data());
}
};
}
Expand Down Expand Up @@ -109,8 +126,15 @@ fn test_invalid() {
}

#[test]
fn test_invalid_char() {
fn test_invalid_char_with_prefix() {
const INVALID_STR: &str = "\"0x000000000000000000000000000000000000000000000000000000000000000g\"";
const EXPECTED_MSG: &str = "invalid hex character: g, at 65 at line 1 column 68";
assert_eq!(ser::from_str::<H256>(INVALID_STR).unwrap_err().to_string(), EXPECTED_MSG);
}

#[test]
fn test_invalid_char_without_prefix() {
const INVALID_STR: &str = "\"000000000000000000000000000000000000000000000000000000000000000g\"";
const EXPECTED_MSG: &str = "invalid hex character: g, at 63 at line 1 column 66";
assert_eq!(ser::from_str::<H256>(INVALID_STR).unwrap_err().to_string(), EXPECTED_MSG);
}
2 changes: 1 addition & 1 deletion parity-util-mem/derive/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ fn malloc_size_of_derive(s: synstructure::Structure) -> proc_macro2::TokenStream
"#[ignore_malloc_size_of] should have an explanation, \
e.g. #[ignore_malloc_size_of = \"because reasons\"]"
);
}
},
syn::Meta::NameValue(syn::MetaNameValue { ref path, .. }) if path.is_ident("ignore_malloc_size_of") => true,
_ => false,
});
Expand Down
3 changes: 3 additions & 0 deletions primitive-types/impls/serde/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ The format is based on [Keep a Changelog].

## [Unreleased]

## [0.3.2] - 2021-10-11
ordian marked this conversation as resolved.
Show resolved Hide resolved
- Supported decoding of hex strings without `0x` prefix. [#598](https://github.com/paritytech/parity-common/pull/598)

## [0.3.1] - 2020-05-05
- Added `no_std` support. [#385](https://github.com/paritytech/parity-common/pull/385)

Expand Down
2 changes: 1 addition & 1 deletion primitive-types/impls/serde/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "impl-serde"
version = "0.3.1"
version = "0.3.2"
authors = ["Parity Technologies <[email protected]>"]
edition = "2018"
license = "MIT OR Apache-2.0"
Expand Down
89 changes: 61 additions & 28 deletions primitive-types/impls/serde/src/serialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ fn to_hex_raw<'a>(v: &'a mut [u8], bytes: &[u8], skip_leading_zero: bool) -> &'a
#[derive(Debug, PartialEq, Eq)]
pub enum FromHexError {
/// The `0x` prefix is missing.
#[deprecated(since = "0.3.2", note = "We support non 0x-prefixed hex strings")]
ordian marked this conversation as resolved.
Show resolved Hide resolved
MissingPrefix,
/// Invalid (non-hex) character encountered.
InvalidHex {
Expand All @@ -82,38 +83,34 @@ impl std::error::Error for FromHexError {}
impl fmt::Display for FromHexError {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
match *self {
#[allow(deprecated)]
Self::MissingPrefix => write!(fmt, "0x prefix is missing"),
Self::InvalidHex { character, index } => write!(fmt, "invalid hex character: {}, at {}", character, index),
}
}
}

/// Decode given hex string into a vector of bytes.
/// Decode given (both 0x-prefixed or not) hex string into a vector of bytes.
///
/// Returns an error if the string is not prefixed with `0x`
/// or non-hex characters are present.
/// Returns an error if non-hex characters are present.
pub fn from_hex(v: &str) -> Result<Vec<u8>, FromHexError> {
if !v.starts_with("0x") {
return Err(FromHexError::MissingPrefix)
}
let (v, stripped) = v.strip_prefix("0x").map_or((v, false), |v| (v, true));

let mut bytes = vec![0u8; (v.len() - 1) / 2];
from_hex_raw(v, &mut bytes)?;
let mut bytes = vec![0u8; (v.len() + 1) / 2];
Copy link
Member

Choose a reason for hiding this comment

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

dq: is (v.len() + 1) / 2 used to work in case the hex string is not divisible by 2?

when it's even we would allocate one extra byte but yeah it was like this before too..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, there're some tests like

assert_eq!(from_hex("102"), Ok(vec![1, 2]));

which wouldn't pass if bytes.len() would be 1 instead of 2, so we need to round up. It was - 1 previously because we were always expecting 0x prefix to present.

from_hex_raw(v, &mut bytes, stripped)?;
Ok(bytes)
}

/// Decode given 0x-prefixed hex string into provided slice.
/// Decode given 0x-prefix-stripped hex string into provided slice.
/// Used internally by `from_hex` and `deserialize_check_len`.
///
/// The method will panic if:
/// 1. `v` is shorter than 2 characters (you need to check 0x prefix outside).
/// 2. `bytes` have incorrect length (make sure to allocate enough beforehand).
fn from_hex_raw<'a>(v: &str, bytes: &mut [u8]) -> Result<usize, FromHexError> {
let bytes_len = v.len() - 2;
/// The method will panic if `bytes` have incorrect length (make sure to allocate enough beforehand).
fn from_hex_raw(v: &str, bytes: &mut [u8], stripped: bool) -> Result<usize, FromHexError> {
let bytes_len = v.len();
let mut modulus = bytes_len % 2;
let mut buf = 0;
let mut pos = 0;
for (index, byte) in v.bytes().enumerate().skip(2) {
for (index, byte) in v.bytes().enumerate() {
buf <<= 4;

match byte {
Expand All @@ -126,7 +123,7 @@ fn from_hex_raw<'a>(v: &str, bytes: &mut [u8]) -> Result<usize, FromHexError> {
},
b => {
let character = char::from(b);
return Err(FromHexError::InvalidHex { character, index })
return Err(FromHexError::InvalidHex { character, index: index + if stripped { 2 } else { 0 } })
},
}

Expand Down Expand Up @@ -208,7 +205,7 @@ where
type Value = Vec<u8>;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
write!(formatter, "a 0x-prefixed hex string")
write!(formatter, "a (both 0x-prefixed or not) hex string")
}

fn visit_str<E: de::Error>(self, v: &str) -> Result<Self::Value, E> {
Expand Down Expand Up @@ -237,30 +234,28 @@ where
type Value = usize;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
write!(formatter, "a 0x-prefixed hex string with {}", self.len)
write!(formatter, "a (both 0x-prefixed or not) hex string with {}", self.len)
}

fn visit_str<E: de::Error>(self, v: &str) -> Result<Self::Value, E> {
if !v.starts_with("0x") {
return Err(E::custom(FromHexError::MissingPrefix))
}
let (v, stripped) = v.strip_prefix("0x").map_or((v, false), |v| (v, true));

let len = v.len();
let is_len_valid = match self.len {
ExpectedLen::Exact(ref slice) => len == 2 * slice.len() + 2,
ExpectedLen::Between(min, ref slice) => len <= 2 * slice.len() + 2 && len > 2 * min + 2,
ExpectedLen::Exact(ref slice) => len == 2 * slice.len(),
ExpectedLen::Between(min, ref slice) => len <= 2 * slice.len() && len > 2 * min,
};

if !is_len_valid {
return Err(E::invalid_length(v.len() - 2, &self))
return Err(E::invalid_length(v.len(), &self))
}

let bytes = match self.len {
ExpectedLen::Exact(slice) => slice,
ExpectedLen::Between(_, slice) => slice,
};

from_hex_raw(v, bytes).map_err(E::custom)
from_hex_raw(v, bytes, stripped).map_err(E::custom)
}

fn visit_string<E: de::Error>(self, v: String) -> Result<Self::Value, E> {
Expand All @@ -280,7 +275,7 @@ mod tests {
struct Bytes(#[serde(with = "super")] Vec<u8>);

#[test]
fn should_not_fail_on_short_string() {
fn should_not_fail_on_short_string_with_prefix() {
let a: Bytes = serde_json::from_str("\"0x\"").unwrap();
let b: Bytes = serde_json::from_str("\"0x1\"").unwrap();
let c: Bytes = serde_json::from_str("\"0x12\"").unwrap();
Expand All @@ -297,7 +292,7 @@ mod tests {
}

#[test]
fn should_not_fail_on_other_strings() {
fn should_not_fail_on_other_strings_with_prefix() {
let a: Bytes =
serde_json::from_str("\"0x7f864e18e3dd8b58386310d2fe0919eef27c6e558564b7f67f22d99d20f587\"").unwrap();
let b: Bytes =
Expand All @@ -310,6 +305,37 @@ mod tests {
assert_eq!(c.0.len(), 32);
}

#[test]
fn should_not_fail_on_short_string_without_prefix() {
let a: Bytes = serde_json::from_str("\"\"").unwrap();
let b: Bytes = serde_json::from_str("\"1\"").unwrap();
let c: Bytes = serde_json::from_str("\"12\"").unwrap();
let d: Bytes = serde_json::from_str("\"123\"").unwrap();
let e: Bytes = serde_json::from_str("\"1234\"").unwrap();
let f: Bytes = serde_json::from_str("\"12345\"").unwrap();

assert!(a.0.is_empty());
assert_eq!(b.0, vec![1]);
assert_eq!(c.0, vec![0x12]);
assert_eq!(d.0, vec![0x1, 0x23]);
assert_eq!(e.0, vec![0x12, 0x34]);
assert_eq!(f.0, vec![0x1, 0x23, 0x45]);
}

#[test]
fn should_not_fail_on_other_strings_without_prefix() {
let a: Bytes =
serde_json::from_str("\"7f864e18e3dd8b58386310d2fe0919eef27c6e558564b7f67f22d99d20f587\"").unwrap();
let b: Bytes =
serde_json::from_str("\"7f864e18e3dd8b58386310d2fe0919eef27c6e558564b7f67f22d99d20f587b\"").unwrap();
let c: Bytes =
serde_json::from_str("\"7f864e18e3dd8b58386310d2fe0919eef27c6e558564b7f67f22d99d20f587b4\"").unwrap();

assert_eq!(a.0.len(), 31);
assert_eq!(b.0.len(), 32);
assert_eq!(c.0.len(), 32);
}

#[test]
fn should_serialize_and_deserialize_empty_bytes() {
let bytes = Bytes(Vec::new());
Expand All @@ -323,7 +349,7 @@ mod tests {
}

#[test]
fn should_encode_to_and_from_hex() {
fn should_encode_to_and_from_hex_with_prefix() {
assert_eq!(to_hex(&[0, 1, 2], true), "0x102");
assert_eq!(to_hex(&[0, 1, 2], false), "0x000102");
assert_eq!(to_hex(&[0], true), "0x0");
Expand All @@ -334,4 +360,11 @@ mod tests {
assert_eq!(from_hex("0x102"), Ok(vec![1, 2]));
assert_eq!(from_hex("0xf"), Ok(vec![0xf]));
}

#[test]
fn should_decode_hex_without_prefix() {
assert_eq!(from_hex("0102"), Ok(vec![1, 2]));
assert_eq!(from_hex("102"), Ok(vec![1, 2]));
assert_eq!(from_hex("f"), Ok(vec![0xf]));
}
}