-
Notifications
You must be signed in to change notification settings - Fork 11
Add BIP375 fields #45
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
e1938eb
59c80b8
9177caa
57b8fc3
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 |
|---|---|---|
|
|
@@ -413,6 +413,27 @@ pub enum Error { | |
| LockTime(absolute::ConversionError), | ||
| /// Unsupported PSBT version. | ||
| UnsupportedVersion(version::UnsupportedVersionError), | ||
| /// Invalid scan key for BIP-375 silent payments (expected 33 bytes). | ||
| InvalidScanKey { | ||
| /// The length that was provided. | ||
| got: usize, | ||
| /// The expected length. | ||
| expected: usize, | ||
| }, | ||
| /// Invalid ECDH share for BIP-375 silent payments (expected 33 bytes). | ||
| InvalidEcdhShare { | ||
| /// The length that was provided. | ||
| got: usize, | ||
| /// The expected length. | ||
| expected: usize, | ||
| }, | ||
| /// Invalid DLEQ proof for BIP-375 silent payments (expected 64 bytes). | ||
| InvalidDleqProof { | ||
| /// The length that was provided. | ||
| got: usize, | ||
| /// The expected length. | ||
| expected: usize, | ||
| }, | ||
|
Comment on lines
+431
to
+436
Owner
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. We can simplify the error handling by using nested errors, eg /// Invalid DLEQ proof for BIP-375 silent payments (expected 64 bytes).
InvalidDleqProof(dleq::InvalidLengthError),
Contributor
Author
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. dleq.rs is conditionally imported with feature flag, are you good with this in serialize.rs::Error?
Owner
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. From a 2 second look, it looks fine to me mate. |
||
| } | ||
|
|
||
| impl fmt::Display for Error { | ||
|
|
@@ -440,6 +461,15 @@ impl fmt::Display for Error { | |
| f.write_str("data not consumed entirely when explicitly deserializing"), | ||
| LockTime(ref e) => write_err!(f, "parsed locktime invalid"; e), | ||
| UnsupportedVersion(ref e) => write_err!(f, "unsupported version"; e), | ||
| InvalidScanKey { got, expected } => { | ||
| write!(f, "invalid scan key: got {} bytes, expected {}", got, expected) | ||
| } | ||
| InvalidEcdhShare { got, expected } => { | ||
| write!(f, "invalid ECDH share: got {} bytes, expected {}", got, expected) | ||
| } | ||
| InvalidDleqProof { got, expected } => { | ||
| write!(f, "invalid DLEQ proof: got {} bytes, expected {}", got, expected) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -467,7 +497,10 @@ impl std::error::Error for Error { | |
| | InvalidLeafVersion | ||
| | Taproot(_) | ||
| | TapTree(_) | ||
| | PartialDataConsumption => None, | ||
| | PartialDataConsumption | ||
| | InvalidScanKey { .. } | ||
| | InvalidEcdhShare { .. } | ||
| | InvalidDleqProof { .. } => None, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| // SPDX-License-Identifier: CC0-1.0 | ||
|
|
||
| //! BIP-375: Support for silent payments in PSBTs. | ||
| //! | ||
| //! This module provides type-safe wrapper for BIP-374 dleq proof field. | ||
| use core::fmt; | ||
|
|
||
| use crate::prelude::*; | ||
| use crate::serialize::{Deserialize, Serialize}; | ||
|
|
||
| /// A 64-byte DLEQ proof (BIP-374). | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub struct DleqProof(pub [u8; 64]); | ||
|
|
||
| #[cfg(feature = "serde")] | ||
| impl actual_serde::Serialize for DleqProof { | ||
| fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: actual_serde::Serializer, | ||
| { | ||
| if serializer.is_human_readable() { | ||
| serializer.serialize_str(&bitcoin::hex::DisplayHex::to_lower_hex_string(&self.0[..])) | ||
| } else { | ||
| serializer.serialize_bytes(&self.0[..]) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "serde")] | ||
| impl<'de> actual_serde::Deserialize<'de> for DleqProof { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: actual_serde::Deserializer<'de>, | ||
| { | ||
| if deserializer.is_human_readable() { | ||
| struct HexVisitor; | ||
| impl actual_serde::de::Visitor<'_> for HexVisitor { | ||
| type Value = DleqProof; | ||
|
|
||
| fn expecting(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { | ||
| f.write_str("a 64-byte hex string") | ||
| } | ||
|
|
||
| fn visit_str<E>(self, s: &str) -> Result<Self::Value, E> | ||
| where | ||
| E: actual_serde::de::Error, | ||
| { | ||
| use bitcoin::hex::FromHex; | ||
| let vec = Vec::<u8>::from_hex(s).map_err(E::custom)?; | ||
| DleqProof::try_from(vec).map_err(|e| { | ||
| E::custom(format!("expected {} bytes, got {}", e.expected, e.got)) | ||
| }) | ||
| } | ||
| } | ||
| deserializer.deserialize_str(HexVisitor) | ||
| } else { | ||
| struct BytesVisitor; | ||
| impl actual_serde::de::Visitor<'_> for BytesVisitor { | ||
| type Value = DleqProof; | ||
|
|
||
| fn expecting(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { | ||
| f.write_str("64 bytes") | ||
| } | ||
|
|
||
| fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E> | ||
| where | ||
| E: actual_serde::de::Error, | ||
| { | ||
| DleqProof::try_from(v).map_err(|e| { | ||
| E::custom(format!("expected {} bytes, got {}", e.expected, e.got)) | ||
| }) | ||
| } | ||
| } | ||
| deserializer.deserialize_bytes(BytesVisitor) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl DleqProof { | ||
| /// Creates a new [`DleqProof`] from a 64-byte array. | ||
| pub fn new(bytes: [u8; 64]) -> Self { DleqProof(bytes) } | ||
|
Comment on lines
+81
to
+82
Owner
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. Perhaps drop this? We have |
||
|
|
||
| /// Returns the inner 64-byte array. | ||
| pub fn as_bytes(&self) -> &[u8; 64] { &self.0 } | ||
| } | ||
|
|
||
| impl From<[u8; 64]> for DleqProof { | ||
| fn from(bytes: [u8; 64]) -> Self { DleqProof(bytes) } | ||
| } | ||
|
|
||
| impl AsRef<[u8]> for DleqProof { | ||
| fn as_ref(&self) -> &[u8] { &self.0 } | ||
| } | ||
|
|
||
| impl TryFrom<&[u8]> for DleqProof { | ||
| type Error = InvalidLengthError; | ||
|
|
||
| fn try_from(slice: &[u8]) -> Result<Self, Self::Error> { | ||
| <[u8; 64]>::try_from(slice) | ||
| .map(DleqProof) | ||
| .map_err(|_| InvalidLengthError { got: slice.len(), expected: 64 }) | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<Vec<u8>> for DleqProof { | ||
| type Error = InvalidLengthError; | ||
|
|
||
| fn try_from(v: Vec<u8>) -> Result<Self, Self::Error> { Self::try_from(v.as_slice()) } | ||
| } | ||
|
|
||
| impl Serialize for DleqProof { | ||
| fn serialize(&self) -> Vec<u8> { self.0.to_vec() } | ||
| } | ||
|
|
||
| impl Deserialize for DleqProof { | ||
| fn deserialize(bytes: &[u8]) -> Result<Self, crate::serialize::Error> { | ||
| DleqProof::try_from(bytes).map_err(|e| crate::serialize::Error::InvalidDleqProof { | ||
| got: e.got, | ||
| expected: e.expected, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /// Error returned when a byte array has an invalid length for a dleq proof. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct InvalidLengthError { | ||
| /// The length that was provided. | ||
| pub got: usize, | ||
| /// The expected length. | ||
| pub expected: usize, | ||
| } | ||
|
|
||
| impl fmt::Display for InvalidLengthError { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| write!(f, "invalid length for BIP-375 type: got {}, expected {}", self.got, self.expected) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "std")] | ||
| impl std::error::Error for InvalidLengthError {} | ||
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.
In this case I prefer the locality and duplicity of the code rather than the neat but more complex macro abstraction. I recommend to remove and replace this macro by the actual code in the
decodemethod.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.
I considered the theme of v2_impl_psbt_insert_pair when making the decision.
Would having separate macros
insert_sp_ecdhandinsert_sp_dleqbe a reasonable middle ground?I am happy to alter this if inline is the preference of the maintainers, just let me know.
Thank you for the review!
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.
I haven't forgotten you mate, just a bit snowed under atm.
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.
This addition is not urgent, take your time!
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.
I definitely have a bias against macros, but in this case, it does look like it's following the existing patterns of the crate. That makes it easier to digest and possibly refactor in the future if we clean up the macros. But I'm a new contributor here, so will defer to @tcharding 's take on it.