-
Notifications
You must be signed in to change notification settings - Fork 38
Replace custom hex parsing with hex-conservative crate #259
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -720,6 +720,60 @@ impl<'de> Deserialize<'de> for Nonce { | |||
| } | ||||
| } | ||||
|
|
||||
| /// Error decoding hexadecimal string into tweak-like value. | ||||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||||
| pub enum TweakHexDecodeError { | ||||
| /// Invalid hexadecimal string. | ||||
| InvalidHex(hex_conservative::DecodeFixedLengthBytesError), | ||||
| /// Invalid tweak after decoding hexadecimal string. | ||||
| InvalidTweak(secp256k1_zkp::Error), | ||||
| } | ||||
|
|
||||
| impl fmt::Display for TweakHexDecodeError { | ||||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||||
| match self { | ||||
| TweakHexDecodeError::InvalidHex(err) => { | ||||
| write!(f, "Invalid hex: {}", err) | ||||
| } | ||||
| TweakHexDecodeError::InvalidTweak(err) => { | ||||
| write!(f, "Invalid tweak: {}", err) | ||||
| } | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| #[doc(hidden)] | ||||
| impl From<hex_conservative::DecodeFixedLengthBytesError> for TweakHexDecodeError { | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 775d7cd: We don't have to fix this here (it's a problem throughout the crate) but I would like to get rid of these |
||||
| fn from(err: hex_conservative::DecodeFixedLengthBytesError) -> Self { | ||||
| TweakHexDecodeError::InvalidHex(err) | ||||
| } | ||||
| } | ||||
|
|
||||
| #[doc(hidden)] | ||||
| impl From<secp256k1_zkp::Error> for TweakHexDecodeError { | ||||
| fn from(err: secp256k1_zkp::Error) -> Self { | ||||
| TweakHexDecodeError::InvalidTweak(err) | ||||
| } | ||||
| } | ||||
|
|
||||
| impl From<TweakHexDecodeError> for encode::Error { | ||||
| fn from(value: TweakHexDecodeError) -> Self { | ||||
| match value { | ||||
| TweakHexDecodeError::InvalidHex(err) => encode::Error::HexFixedError(err), | ||||
| TweakHexDecodeError::InvalidTweak(err) => encode::Error::Secp256k1zkp(err), | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| impl std::error::Error for TweakHexDecodeError { | ||||
| fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { | ||||
| match self { | ||||
| TweakHexDecodeError::InvalidHex(err) => Some(err), | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 775d7cd: Also don't need to fix it here but we should use |
||||
| TweakHexDecodeError::InvalidTweak(err) => Some(err), | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| /// Blinding factor used for asset commitments. | ||||
| #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash)] | ||||
| pub struct AssetBlindingFactor(pub(crate) Tweak); | ||||
|
|
@@ -747,16 +801,13 @@ impl AssetBlindingFactor { | |||
| } | ||||
|
|
||||
| impl hex::FromHex for AssetBlindingFactor { | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 775d7cd: I don't think we should keep the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, it's used for the serde It seems to me that we can replace it with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't than all Line 58 in f6ffc78
|
||||
| fn from_byte_iter<I>(iter: I) -> Result<Self, hex::Error> | ||||
| where I: Iterator<Item=Result<u8, hex::Error>> + | ||||
| ExactSizeIterator + | ||||
| DoubleEndedIterator | ||||
| { | ||||
| let slice = <[u8; 32]>::from_byte_iter(iter.rev())?; | ||||
| // Incorrect Return Error | ||||
| // See: https://github.com/rust-bitcoin/bitcoin_hashes/issues/124 | ||||
| let inner = Tweak::from_inner(slice) | ||||
| .map_err(|_e| hex::Error::InvalidChar(0))?; | ||||
| type Err = TweakHexDecodeError; | ||||
|
|
||||
| fn from_hex(s: &str) -> Result<Self, Self::Err> { | ||||
| let mut slice: [u8; 32] = hex_conservative::decode_to_array(s)?; | ||||
| slice.reverse(); | ||||
|
|
||||
| let inner = Tweak::from_inner(slice)?; | ||||
| Ok(AssetBlindingFactor(inner)) | ||||
| } | ||||
| } | ||||
|
|
@@ -951,16 +1002,13 @@ impl Neg for ValueBlindingFactor { | |||
| } | ||||
|
|
||||
| impl hex::FromHex for ValueBlindingFactor { | ||||
| fn from_byte_iter<I>(iter: I) -> Result<Self, hex::Error> | ||||
| where I: Iterator<Item=Result<u8, hex::Error>> + | ||||
| ExactSizeIterator + | ||||
| DoubleEndedIterator | ||||
| { | ||||
| let slice = <[u8; 32]>::from_byte_iter(iter.rev())?; | ||||
| // Incorrect Return Error | ||||
| // See: https://github.com/rust-bitcoin/bitcoin_hashes/issues/124 | ||||
| let inner = Tweak::from_inner(slice) | ||||
| .map_err(|_e| hex::Error::InvalidChar(0))?; | ||||
| type Err = TweakHexDecodeError; | ||||
|
|
||||
| fn from_hex(s: &str) -> Result<Self, Self::Err> { | ||||
| let mut slice: [u8; 32] = hex_conservative::decode_to_array(s)?; | ||||
| slice.reverse(); | ||||
|
|
||||
| let inner = Tweak::from_inner(slice)?; | ||||
| Ok(ValueBlindingFactor(inner)) | ||||
| } | ||||
| } | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ use std::io::Cursor; | |
| use std::{any, error, fmt, io, mem}; | ||
|
|
||
| use bitcoin::ScriptBuf; | ||
| use hex_conservative::{DecodeFixedLengthBytesError, DecodeVariableLengthBytesError}; | ||
| use secp256k1_zkp::{self, RangeProof, SurjectionProof, Tweak}; | ||
|
|
||
| use crate::hashes::{sha256, Hash}; | ||
|
|
@@ -54,8 +55,10 @@ pub enum Error { | |
| Secp256k1zkp(secp256k1_zkp::Error), | ||
| /// Pset related Errors | ||
| PsetError(pset::Error), | ||
| /// Hex parsing errors | ||
| HexError(crate::hex::Error), | ||
| /// Hex fixed parsing errors | ||
| HexFixedError(DecodeFixedLengthBytesError), | ||
| /// Hex variable parsing errors | ||
| HexVariableError(DecodeVariableLengthBytesError), | ||
|
Comment on lines
+58
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really sure if it's okay to keep both this way. We can alternatively keep only one variant as before |
||
| /// Got a time-based locktime when expecting a height-based one, or vice-versa | ||
| BadLockTime(crate::LockTime), | ||
| /// `VarInt` was encoded in a non-minimal way. | ||
|
|
@@ -83,7 +86,8 @@ impl fmt::Display for Error { | |
| Error::Secp256k1(ref e) => write!(f, "{}", e), | ||
| Error::Secp256k1zkp(ref e) => write!(f, "{}", e), | ||
| Error::PsetError(ref e) => write!(f, "Pset Error: {}", e), | ||
| Error::HexError(ref e) => write!(f, "Hex error {}", e), | ||
| Error::HexFixedError(ref e) => write!(f, "Hex fixed error: {}", e), | ||
| Error::HexVariableError(ref e) => write!(f, "Hex variable error: {}", e), | ||
| Error::BadLockTime(ref lt) => write!(f, "Invalid locktime {}", lt), | ||
| Error::NonMinimalVarInt => write!(f, "non-minimal varint"), | ||
| } | ||
|
|
@@ -134,9 +138,16 @@ impl From<secp256k1_zkp::Error> for Error { | |
| } | ||
|
|
||
| #[doc(hidden)] | ||
| impl From<crate::hex::Error> for Error { | ||
| fn from(e: crate::hex::Error) -> Self { | ||
| Error::HexError(e) | ||
| impl From<DecodeFixedLengthBytesError> for Error { | ||
| fn from(e: DecodeFixedLengthBytesError) -> Self { | ||
| Error::HexFixedError(e) | ||
| } | ||
| } | ||
|
|
||
| #[doc(hidden)] | ||
| impl From<DecodeVariableLengthBytesError> for Error { | ||
| fn from(e: DecodeVariableLengthBytesError) -> Self { | ||
| Error::HexVariableError(e) | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Hi @Velnbur thanks for the PR, could you please regenerate the Cargo.lock file and include it as Cargo-latest.lock ?