-
Notifications
You must be signed in to change notification settings - Fork 225
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
feat(primitive-types, serde): deserialize hex strings without 0x prefix #598
Conversation
49a4773
to
9574a9b
Compare
@ordian fixed the remaining tests |
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.
@tomusdrw is this going to break anything? We're less restrictive here, but also
assert_eq!(serialize(deserialize(s)?), s)
is broken in some cases now.
Other than that, LGTM.
We've been trying to be strict to avoid ambiguity between hex and decimal encoding. Also it used to be part of the Ethereum JSON-RPC spec to always prefix hex values with |
sounds good. Merge? |
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.
LGTM, the changelog needs to be updated not sure if any project is using these types in the ethereum JSON-RPC interface
anymore but still...
|
||
let mut bytes = vec![0u8; (v.len() - 1) / 2]; | ||
from_hex_raw(v, &mut bytes)?; | ||
let mut bytes = vec![0u8; (v.len() + 1) / 2]; |
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.
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..
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.
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.
@niklasad1 I updated the changelog |
Ok, thanks. I'll merge and publish it today. |
Published. |
Support hex strings which are not prefixed with
0x
.Example from geth's debug_traceTransaction (
storage
key):