Add support for prefixes to the FromHex trait
#58
Conversation
|
concept ACK. I'd drop the bolding from "not" in "not recommended for user input". |
tcharding
force-pushed
the
01-08-improve-traits
branch
from
dc955db to
4668969
Compare
|
Added a solution to the inner |
tcharding
force-pushed
the
01-08-improve-traits
branch
2 times, most recently
from
da62234 to
e44ed4f
Compare
There was a problem hiding this comment.
I was just writing an objection but I've just realized the from_hex method was wrong from the beginning of this crate. It changed the semantics of to allow 0x which previously wasn't allowed causing everyone who used it to accidentally implement the broken "robustness" principle. While we've strictly issued breaking versions it's still subtle and people probably thought it's the same method.
Thankfully it didn't affect most of the hashes because of reversed handling. But it does affect FromStr for Midstate.
And since the robustness principle is broken, I would prefer changing the semantics of from_hex to what it originally was and use from_maybe_prefixed_hex for the current one.
Definitely concept ACK doing something about this.
|
Regarding |
Quote for #2323 I had definitely forgotten this, this is a simple discreet problem that we can fix. Leave it with me. |
As suggested by clippy remove the needless borrow.
tcharding
force-pushed
the
01-08-improve-traits
branch
from
e44ed4f to
2b1359f
Compare
Hex strings may or may not include a prefix. We want to default to explicitly disallowing a prefix in `FromHex` because that is the behaviour we expect in `rust-bitcoin`. Add functions to the `FromHex` trait for various prefix states.
tcharding
force-pushed
the
01-08-improve-traits
branch
from
2b1359f to
c5a4cdf
Compare
| #[cfg(not(feature = "alloc"))] S: AsRef<str>, | ||
| >(s: S) -> Result<Self, Self::Error> { | ||
| if s.as_ref().starts_with("0x") { | ||
| Self::from_no_prefix_hex(s.as_ref().trim_start_matches("0x")) |
There was a problem hiding this comment.
trim_start_matches removes repeated matches so it allows 0x0x0x0x42, which is wrong. Also the whole code seems to be needlessly complicated. You should only need this:
Self::from_no_prefix_hex(s.strip_prefix("0x").unwrap_or(s))| #[rustfmt::skip] | ||
| fn from_hex< | ||
| #[cfg(feature = "alloc")] S: AsRef<str> + Into<String>, | ||
| #[cfg(not(feature = "alloc"))] S: AsRef<str>, |
There was a problem hiding this comment.
This is broken because people can call the method for non-alloc and then alloc would silently add the bound.
| /// Parses provided string as hex explicitly requiring there to not be a `0x` prefix. | ||
| /// | ||
| /// This is not recommended for user-supplied inputs because of possible confusion with decimals. | ||
| /// It should be only used for existing protocols which always encode values as hex without 0x prefix. |
There was a problem hiding this comment.
There should be a note that this is equivalent to from_hex But maybe let's not duplicate the method?
| #[cfg(not(feature = "alloc"))] | ||
| return Err(MissingPrefix(MissingPrefixError())); | ||
| } else { | ||
| Ok(Self::from_no_prefix_hex(s.as_ref().trim_start_matches("0x"))?) |
There was a problem hiding this comment.
Same problem as above, you can use s.as_ref().strip_prefix("0x").ok_or_else(|| MissingPrefixError(s.into()).into())
|
This PR is tied up with the |
|
Consumed by #67 |
Draft because error handling of
FromHexneeds attention and also I can't work out how to print backwards inhashesanymore.Add support for parsing prefixed strings as well as requiring the presence or absence of the prefix.
This code was copied from
rust-bitcoin. The error type was re-named.