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

Jump to bottom

Removed Default from SerializedSignature #458

Merged
merged 1 commit into from
Jun 22, 2022
Merged

Removed Default from SerializedSignature #458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ The major change in this version is the increase of the Minimum Supported Rust V
* Key tweaking methods renamed and refactored to use a more [functional-style](https://github.com/rust-bitcoin/rust-secp256k1/pull/406), they now accept a [new Scalar](https://github.com/rust-bitcoin/rust-secp256k1/pull/445) type instead of raw slices
* Update [`rand` dependency to 0.8](https://github.com/rust-bitcoin/rust-secp256k1/pull/331)
* `KeyPair::from_secret_key` [borrows SecretKey](https://github.com/rust-bitcoin/rust-secp256k1/pull/430) instead of taking ownership
* `SerializedSignature` no longer implements `Default`

## New features/APIs

Expand All @@ -18,6 +19,7 @@ The major change in this version is the increase of the Minimum Supported Rust V
* [Implemented `TryFrom` for `Parity`](https://github.com/rust-bitcoin/rust-secp256k1/pull/409)
* The [alloc feature](https://github.com/rust-bitcoin/rust-secp256k1/pull/331) can be used on targets with allocators without a standard library
* `SharedSecret` can be created from a slice, parsed from a hex string, or [(de)serialized using serde](https://github.com/rust-bitcoin/rust-secp256k1/pull/418)
* `SerializedSignature` implements `IntoIterator` (both owned and shared reference)
* We now [derive `std::hash::Hash` for `Signature`](https://github.com/rust-bitcoin/rust-secp256k1/pull/441)

## Other improvements
Expand Down
10 changes: 4 additions & 6 deletions src/ecdsa/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,20 +157,18 @@ impl Signature {
#[inline]
/// Serializes the signature in DER format
pub fn serialize_der(&self) -> SerializedSignature {
let mut ret = SerializedSignature::default();
let mut len: usize = ret.capacity();
let mut data = [0u8; serialized_signature::MAX_LEN];
let mut len: usize = serialized_signature::MAX_LEN;
unsafe {
let err = ffi::secp256k1_ecdsa_signature_serialize_der(
ffi::secp256k1_context_no_precomp,
ret.get_data_mut_ptr(),
data.as_mut_ptr(),
&mut len,
self.as_c_ptr(),
);
debug_assert!(err == 1);
assert!(len <= serialized_signature::MAX_LEN, "libsecp256k1 set length to {} but the maximum is {}", len, serialized_signature::MAX_LEN);
ret.set_len(len);
SerializedSignature::from_raw_parts(data, len)
}
ret
}

#[inline]
Expand Down
28 changes: 13 additions & 15 deletions src/ecdsa/serialized_signature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,6 @@ impl fmt::Display for SerializedSignature {
}
}

impl Default for SerializedSignature {
#[inline]
fn default() -> SerializedSignature {
SerializedSignature {
data: [0u8; MAX_LEN],
len: 0,
}
}
}

impl PartialEq for SerializedSignature {
#[inline]
fn eq(&self, other: &SerializedSignature) -> bool {
Expand Down Expand Up @@ -91,10 +81,18 @@ impl<'a> IntoIterator for &'a SerializedSignature {
}

impl SerializedSignature {
/// Get a pointer to the underlying data with the specified capacity.
/// Creates `SerializedSignature` from data and length.
///
/// ## Panics
///
/// If `len` > `MAX_LEN`
Comment on lines +84 to +88
Copy link
Member

Choose a reason for hiding this comment

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

Pedantic style question I've been wondering about, if you have an opinion please. Since I started writing docs like these I've switched from using the format above to using no space after the heading when it looks 'spacey' (lots of whitespace). But its leading to non-uniform docs because once there is more text it looks better with the space. Do you have an opinion?

Without the space kinda looks nicer

    /// Creates `SerializedSignature` from data and length.
    ///
    /// ## Panics
    /// If `len` > `MAX_LEN`

And a made up more wordy one with the newline seems to look nicer.

    /// Creates `SerializedSignature` from data and length.
    ///
    /// Once there is a long description that takes up a bunch of space saotnhsoaetsaotus s ossao
    /// and goes onto a second line stnaho osanteusoan soa esnthaosu .
    ///
    /// ## Panics
    ///
    /// If `len` > `MAX_LEN`

Should we favor dogmatic uniformity (my go to :), or go with different newline spacing depending on the situation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like to always have empty line after heading. I think changing this one would be just noise.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'm going to follow suit. I'll change any I come across that I wrote the other way :) Cheers.

#[inline]
pub(crate) fn get_data_mut_ptr(&mut self) -> *mut u8 {
self.data.as_mut_ptr()
pub(crate) fn from_raw_parts(data: [u8; MAX_LEN], len: usize) -> Self {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should have #[track_caller] in MSRV 1.46

assert!(len <= MAX_LEN, "attempt to set length to {} but the maximum is {}", len, MAX_LEN);
SerializedSignature {
data,
len,
}
}

/// Get the capacity of the underlying data buffer.
Expand All @@ -111,7 +109,7 @@ impl SerializedSignature {

/// Set the length of the object.
#[inline]
pub(crate) fn set_len(&mut self, len: usize) {
pub(crate) fn set_len_unchecked(&mut self, len: usize) {
self.len = len;
}

Expand Down Expand Up @@ -218,7 +216,7 @@ mod into_iter {
// reach this
let new_len = self.signature.len() - 1;
let byte = self.signature[new_len];
self.signature.set_len(new_len);
self.signature.set_len_unchecked(new_len);
Some(byte)
}
}
Expand Down