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.

Sign up for GitHub

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

Rebased version of [Alternative] Allow deserializing from owned types + support for new schnorr module #270

Merged
merged 5 commits into from
Apr 21, 2021
Merged

Rebased version of [Alternative] Allow deserializing from owned types + support for new schnorr module #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1f08a31
Replace serde macros with generic visitor module
elichai May 15, 2020
b4040f0
Update test names to reflect what they are doing
thomaseizinger Jan 12, 2021
18890d3
Extend schnorr-sig serde test suite with owned and slice variants
thomaseizinger Jan 12, 2021
e6e23e9
Rename `HexVisitor` to `FromStrVisitor`
thomaseizinger Jan 12, 2021
c2fd5ce
Remove `visit_bytes` implementation from `FromStrVisitor`
thomaseizinger Jan 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ecdh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ mod tests {
let s = Secp256k1::signing_only();
let (sk1, pk1) = s.generate_keypair(&mut thread_rng());
let (sk2, pk2) = s.generate_keypair(&mut thread_rng());

let sec1 = SharedSecret::new_with_hash(&pk1, &sk2, |x,_| x.into());
let sec2 = SharedSecret::new_with_hash(&pk2, &sk1, |x,_| x.into());
let sec_odd = SharedSecret::new_with_hash(&pk1, &sk1, |x,_| x.into());
Expand Down
68 changes: 65 additions & 3 deletions src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,32 @@ impl SecretKey {
}
}

serde_impl!(SecretKey, constants::SECRET_KEY_SIZE);
#[cfg(feature = "serde")]
impl ::serde::Serialize for SecretKey {
fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self[..])
}
}
}

#[cfg(feature = "serde")]
impl<'de> ::serde::Deserialize<'de> for SecretKey {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
if d.is_human_readable() {
d.deserialize_str(super::serde_util::FromStrVisitor::new(
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps remove super:: and add a use statement that imports using the fully qualified path (although I would remove serde_util:: also). Then we can remove super:: at no additional cost all the way through this PR. (I'm still singing the same old tune :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how useful a use statement will be.

This code is feature-gated so I would either have to feature-gate the use statement at the top of the file as well or accept that rustc will issue a dead-code warning if we compile without the serde feature flag.

For feature-gated code, I usually err on the side of having full-qualified paths, esp. if they only occur once within the code that is gated.

We could potentially include a use super::serde_util; at the beginning of the function and then reference both FromStrVisitor and BytesVisitor as serde_util::FromStrVisitor.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the response, I didn't catch the feature gated nuance when I commented. Not sure which part of the comment @sgeisler is ok'ing but I like use statement inside the function option. Its pretty minor though, so don't sweat it :)

"a hex string representing 32 byte SecretKey"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
"raw 32 bytes SecretKey",
SecretKey::from_slice
))
}
}
}

impl PublicKey {
/// Obtains a raw const pointer suitable for use with FFI functions
Expand Down Expand Up @@ -402,7 +427,32 @@ impl From<ffi::PublicKey> for PublicKey {
}
}

serde_impl_from_slice!(PublicKey);
#[cfg(feature = "serde")]
impl ::serde::Serialize for PublicKey {
fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self.serialize())
}
}
}

#[cfg(feature = "serde")]
impl<'de> ::serde::Deserialize<'de> for PublicKey {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<PublicKey, D::Error> {
if d.is_human_readable() {
d.deserialize_str(super::serde_util::FromStrVisitor::new(
"an ASCII hex string representing a public key"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
"a bytestring representing a public key",
PublicKey::from_slice
Comment on lines +449 to +451
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a breaking change? currently bytes will deserialize as hex: https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/macros.rs#L116-L125

(I'm not saying it's bad, just trying to know if this might break downstream users)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is!

The lines you commented on are within the else branch of d.is_human_readable whereas the code you linked to is within d.is_human_readable.

Also, I think the linked piece of code was never actually called because I deleted a legacy of it in c2fd5ce and all tests still pass!

Copy link
Member

Choose a reason for hiding this comment

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

hmm I see, you're right.

))
}
}
}

impl PartialOrd for PublicKey {
fn partial_cmp(&self, other: &PublicKey) -> Option<::core::cmp::Ordering> {
Expand Down Expand Up @@ -818,7 +868,7 @@ mod test {

#[cfg(feature = "serde")]
#[test]
fn test_signature_serde() {
fn test_serde() {
use serde_test::{Configure, Token, assert_tokens};
static SK_BYTES: [u8; 32] = [
1, 1, 1, 1, 1, 1, 1, 1,
Expand Down Expand Up @@ -846,8 +896,20 @@ mod test {
let pk = PublicKey::from_secret_key(&s, &sk);

assert_tokens(&sk.compact(), &[Token::BorrowedBytes(&SK_BYTES[..])]);
assert_tokens(&sk.compact(), &[Token::Bytes(&SK_BYTES)]);
assert_tokens(&sk.compact(), &[Token::ByteBuf(&SK_BYTES)]);

assert_tokens(&sk.readable(), &[Token::BorrowedStr(SK_STR)]);
assert_tokens(&sk.readable(), &[Token::Str(SK_STR)]);
assert_tokens(&sk.readable(), &[Token::String(SK_STR)]);

assert_tokens(&pk.compact(), &[Token::BorrowedBytes(&PK_BYTES[..])]);
assert_tokens(&pk.compact(), &[Token::Bytes(&PK_BYTES)]);
assert_tokens(&pk.compact(), &[Token::ByteBuf(&PK_BYTES)]);

assert_tokens(&pk.readable(), &[Token::BorrowedStr(PK_STR)]);
assert_tokens(&pk.readable(), &[Token::Str(PK_STR)]);
assert_tokens(&pk.readable(), &[Token::String(PK_STR)]);

}
}
26 changes: 17 additions & 9 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ pub mod key;
pub mod schnorrsig;
#[cfg(feature = "recovery")]
pub mod recovery;
#[cfg(feature = "serde")]
mod serde_util;

pub use key::SecretKey;
pub use key::PublicKey;
Expand Down Expand Up @@ -435,21 +437,21 @@ impl ::serde::Serialize for Signature {
} else {
s.serialize_bytes(&self.serialize_der())
}

}
}

#[cfg(feature = "serde")]
impl<'de> ::serde::Deserialize<'de> for Signature {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Signature, D::Error> {
use ::serde::de::Error;
use str::FromStr;
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
if d.is_human_readable() {
let sl: &str = ::serde::Deserialize::deserialize(d)?;
Signature::from_str(sl).map_err(D::Error::custom)
d.deserialize_str(serde_util::FromStrVisitor::new(
"a hex string representing a DER encoded Signature"
))
} else {
let sl: &[u8] = ::serde::Deserialize::deserialize(d)?;
Signature::from_der(sl).map_err(D::Error::custom)
d.deserialize_bytes(serde_util::BytesVisitor::new(
"raw byte stream, that represents a DER encoded Signature",
Signature::from_der
))
}
}
}
Expand Down Expand Up @@ -1239,7 +1241,7 @@ mod tests {
#[cfg(feature = "serde")]
#[cfg(not(rust_secp_fuzz))] // fixed sig vectors can't work with fuzz-sigs
#[test]
fn test_signature_serde() {
fn test_serde() {
use serde_test::{Configure, Token, assert_tokens};

let s = Secp256k1::new();
Expand All @@ -1260,7 +1262,13 @@ mod tests {
";

assert_tokens(&sig.compact(), &[Token::BorrowedBytes(&SIG_BYTES[..])]);
assert_tokens(&sig.compact(), &[Token::Bytes(&SIG_BYTES)]);
assert_tokens(&sig.compact(), &[Token::ByteBuf(&SIG_BYTES)]);

assert_tokens(&sig.readable(), &[Token::BorrowedStr(SIG_STR)]);
assert_tokens(&sig.readable(), &[Token::Str(SIG_STR)]);
assert_tokens(&sig.readable(), &[Token::String(SIG_STR)]);

}

#[cfg(feature = "global-context")]
Expand Down
118 changes: 0 additions & 118 deletions src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,121 +43,3 @@ macro_rules! impl_from_array_len {
)+
}
}

#[cfg(feature="serde")]
/// Implements `Serialize` and `Deserialize` for a type `$t` which represents
/// a newtype over a byte-slice over length `$len`. Type `$t` must implement
/// the `FromStr` and `Display` trait.
macro_rules! serde_impl(
($t:ident, $len:expr) => (
impl ::serde::Serialize for $t {
fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self[..])
}
}
}

impl<'de> ::serde::Deserialize<'de> for $t {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<$t, D::Error> {
use ::serde::de::Error;
use core::str::FromStr;

if d.is_human_readable() {
let sl: &str = ::serde::Deserialize::deserialize(d)?;
$t::from_str(sl).map_err(D::Error::custom)
} else {
let sl: &[u8] = ::serde::Deserialize::deserialize(d)?;
if sl.len() != $len {
Err(D::Error::invalid_length(sl.len(), &stringify!($len)))
} else {
let mut ret = [0; $len];
ret.copy_from_slice(sl);
Ok($t(ret))
}
}
}
}
)
);

#[cfg(not(feature="serde"))]
macro_rules! serde_impl(
($t:ident, $len:expr) => ()
);

#[cfg(feature = "serde")]
macro_rules! serde_impl_from_slice {
($t: ident) => {
impl ::serde::Serialize for $t {
fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self.serialize())
}
}
}

impl<'de> ::serde::Deserialize<'de> for $t {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<$t, D::Error> {
if d.is_human_readable() {
struct HexVisitor;

impl<'de> ::serde::de::Visitor<'de> for HexVisitor {
type Value = $t;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("an ASCII hex string")
}

fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
where
E: ::serde::de::Error,
{
if let Ok(hex) = str::from_utf8(v) {
str::FromStr::from_str(hex).map_err(E::custom)
} else {
Err(E::invalid_value(::serde::de::Unexpected::Bytes(v), &self))
}
}

fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
where
E: ::serde::de::Error,
{
str::FromStr::from_str(v).map_err(E::custom)
}
}
d.deserialize_str(HexVisitor)
} else {
struct BytesVisitor;

impl<'de> ::serde::de::Visitor<'de> for BytesVisitor {
type Value = $t;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("a bytestring")
}

fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
where
E: ::serde::de::Error,
{
$t::from_slice(v).map_err(E::custom)
}
}

d.deserialize_bytes(BytesVisitor)
}
}
}
};
}

#[cfg(not(feature = "serde"))]
macro_rules! serde_impl_from_slice(
($t:ident) => ()
);
79 changes: 76 additions & 3 deletions src/schnorrsig.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,33 @@ use {Message, Signing, Verification};
pub struct Signature([u8; constants::SCHNORRSIG_SIGNATURE_SIZE]);
impl_array_newtype!(Signature, u8, constants::SCHNORRSIG_SIGNATURE_SIZE);
impl_pretty_debug!(Signature);
serde_impl!(Signature, constants::SCHNORRSIG_SIGNATURE_SIZE);

#[cfg(feature = "serde")]
impl ::serde::Serialize for Signature {
fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self[..])
}
}
}

#[cfg(feature = "serde")]
impl<'de> ::serde::Deserialize<'de> for Signature {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
if d.is_human_readable() {
d.deserialize_str(super::serde_util::FromStrVisitor::new(
"a hex string representing 64 byte schnorr signature"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
"raw 64 bytes schnorr signature",
Signature::from_slice
))
}
}
}

impl fmt::LowerHex for Signature {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
Expand Down Expand Up @@ -376,7 +402,32 @@ impl From<::key::PublicKey> for PublicKey {
}
}

serde_impl_from_slice!(PublicKey);
#[cfg(feature = "serde")]
impl ::serde::Serialize for PublicKey {
fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self.serialize())
}
}
}

#[cfg(feature = "serde")]
impl<'de> ::serde::Deserialize<'de> for PublicKey {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
if d.is_human_readable() {
d.deserialize_str(super::serde_util::FromStrVisitor::new(
"a hex string representing 32 byte schnorr public key"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
"raw 32 bytes schnorr public key",
PublicKey::from_slice
))
}
}
}

impl<C: Signing> Secp256k1<C> {
fn schnorrsig_sign_helper(
Expand Down Expand Up @@ -724,7 +775,7 @@ mod tests {
#[cfg(feature = "serde")]
#[cfg(not(rust_secp_fuzz))] // fixed sig vectors can't work with fuzz-sigs
#[test]
fn test_signature_serde() {
fn test_serde() {
use serde_test::{assert_tokens, Configure, Token};

let s = Secp256k1::new();
Expand All @@ -745,8 +796,30 @@ mod tests {
14d0bf1a8953506fb460f58be141af767fd112535fb3922ef217308e2c26706f1eeb432b3dba9a01082f9e4d4ef5678ad0d9d532c0dfa907b568722d0b0119ba\
";

static PK_BYTES: [u8; 32] = [
24, 132, 87, 129, 246, 49, 196, 143, 28, 151, 9, 226, 48, 146, 6, 125, 6, 131, 127,
48, 170, 12, 208, 84, 74, 200, 135, 254, 145, 221, 209, 102
];
static PK_STR: &'static str = "\
18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\
";
let pk = PublicKey::from_slice(&PK_BYTES).unwrap();

assert_tokens(&sig.compact(), &[Token::BorrowedBytes(&SIG_BYTES[..])]);
assert_tokens(&sig.compact(), &[Token::Bytes(&SIG_BYTES[..])]);
assert_tokens(&sig.compact(), &[Token::ByteBuf(&SIG_BYTES[..])]);

assert_tokens(&sig.readable(), &[Token::BorrowedStr(SIG_STR)]);
assert_tokens(&sig.readable(), &[Token::Str(SIG_STR)]);
assert_tokens(&sig.readable(), &[Token::String(SIG_STR)]);

assert_tokens(&pk.compact(), &[Token::BorrowedBytes(&PK_BYTES[..])]);
assert_tokens(&pk.compact(), &[Token::Bytes(&PK_BYTES[..])]);
assert_tokens(&pk.compact(), &[Token::ByteBuf(&PK_BYTES[..])]);

assert_tokens(&pk.readable(), &[Token::BorrowedStr(PK_STR)]);
assert_tokens(&pk.readable(), &[Token::Str(PK_STR)]);
assert_tokens(&pk.readable(), &[Token::String(PK_STR)]);
}
#[test]
fn test_addition() {
Expand Down
Loading