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

Deprecate from_slice methods in favor of arrays #737

Merged
merged 2 commits into from
Sep 12, 2024
Merged

Deprecate from_slice methods in favor of arrays #737

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1661f57
Add `from_byte_array` functions
jamillambert Sep 10, 2024
537b85b
Deprecate and replace `from_slice`
jamillambert Sep 10, 2024
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
1 change: 1 addition & 0 deletions src/ecdh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ impl SharedSecret {
pub fn from_bytes(bytes: [u8; SHARED_SECRET_SIZE]) -> SharedSecret { SharedSecret(bytes) }

/// Creates a shared secret from `bytes` slice.
#[deprecated(since = "TBD", note = "Use `from_bytes` instead.")]
Copy link
Member

Choose a reason for hiding this comment

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

I would have expected with this addition to see a change to call from_bytes. If you like the combinator style you could use

    pub fn from_slice(bytes: &[u8]) -> Result<SharedSecret, Error> {
        <[u8; SHARED_SECRET_SIZE]>::try_from(bytes)
            .map_err(|_| Error::InvalidSharedSecret)
            .and_then(|a| Ok(Self::from_bytes(a)))
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I need to change anything again I'll add this too, thanks

#[inline]
pub fn from_slice(bytes: &[u8]) -> Result<SharedSecret, Error> {
match bytes.len() {
Expand Down
23 changes: 14 additions & 9 deletions src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ impl str::FromStr for SecretKey {
fn from_str(s: &str) -> Result<SecretKey, Error> {
let mut res = [0u8; constants::SECRET_KEY_SIZE];
match from_hex(s, &mut res) {
Ok(constants::SECRET_KEY_SIZE) => SecretKey::from_slice(&res),
Ok(constants::SECRET_KEY_SIZE) => SecretKey::from_byte_array(&res),
_ => Err(Error::InvalidSecretKey),
}
}
Expand All @@ -138,7 +138,7 @@ impl str::FromStr for SecretKey {
/// use secp256k1::{SecretKey, Secp256k1, PublicKey};
///
/// let secp = Secp256k1::new();
/// let secret_key = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order");
/// let secret_key = SecretKey::from_byte_array(&[0xcd; 32]).expect("32 bytes, within curve order");
/// let public_key = PublicKey::from_secret_key(&secp, &secret_key);
/// # }
/// ```
Expand Down Expand Up @@ -168,9 +168,13 @@ impl str::FromStr for PublicKey {
fn from_str(s: &str) -> Result<PublicKey, Error> {
let mut res = [0u8; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE];
match from_hex(s, &mut res) {
Ok(constants::PUBLIC_KEY_SIZE) =>
PublicKey::from_slice(&res[0..constants::PUBLIC_KEY_SIZE]),
Ok(constants::UNCOMPRESSED_PUBLIC_KEY_SIZE) => PublicKey::from_slice(&res),
Ok(constants::PUBLIC_KEY_SIZE) => {
let bytes: [u8; constants::PUBLIC_KEY_SIZE] =
res[0..constants::PUBLIC_KEY_SIZE].try_into().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strictly speaking, it'd be better to create a reference here (simply annotating the type with & should work because the conversion is defined) to avoid useless copying but there's a good chance compiler will optimize it out anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Are you saying you can convert a &[u8] to a &[u8; N] via try_into and the compiler will just cast the pointers rather than copying?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, that's a huge win and I feel like a clippy lint should exist for it. (Though it'd be a bit tough in general to tell when the array is only used by reference.)

PublicKey::from_byte_array_compressed(&bytes)
}
Ok(constants::UNCOMPRESSED_PUBLIC_KEY_SIZE) =>
PublicKey::from_byte_array_uncompressed(&res),
_ => Err(Error::InvalidPublicKey),
}
}
Expand Down Expand Up @@ -211,6 +215,7 @@ impl SecretKey {
/// use secp256k1::SecretKey;
/// let sk = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order");
/// ```
#[deprecated(since = "TBD", note = "Use `from_byte_array` instead.")]
#[inline]
pub fn from_slice(data: &[u8]) -> Result<SecretKey, Error> {
match <[u8; constants::SECRET_KEY_SIZE]>::try_from(data) {
Expand Down Expand Up @@ -362,7 +367,7 @@ impl SecretKey {
impl<T: ThirtyTwoByteHash> From<T> for SecretKey {
/// Converts a 32-byte hash directly to a secret key without error paths.
fn from(t: T) -> SecretKey {
SecretKey::from_slice(&t.into_32()).expect("failed to create secret key")
SecretKey::from_byte_array(&t.into_32()).expect("failed to create secret key")
}
}

Expand Down Expand Up @@ -542,7 +547,7 @@ impl PublicKey {
};
buf[1..].clone_from_slice(&pk.serialize());

PublicKey::from_slice(&buf).expect("we know the buffer is valid")
PublicKey::from_byte_array_compressed(&buf).expect("we know the buffer is valid")
}

#[inline]
Expand Down Expand Up @@ -1156,8 +1161,7 @@ impl str::FromStr for XOnlyPublicKey {
fn from_str(s: &str) -> Result<XOnlyPublicKey, Error> {
let mut res = [0u8; constants::SCHNORR_PUBLIC_KEY_SIZE];
match from_hex(s, &mut res) {
Ok(constants::SCHNORR_PUBLIC_KEY_SIZE) =>
XOnlyPublicKey::from_slice(&res[0..constants::SCHNORR_PUBLIC_KEY_SIZE]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

LOL, the previous code...

Ok(constants::SCHNORR_PUBLIC_KEY_SIZE) => XOnlyPublicKey::from_byte_array(&res),
_ => Err(Error::InvalidPublicKey),
}
}
Expand Down Expand Up @@ -1203,6 +1207,7 @@ impl XOnlyPublicKey {
///
/// Returns [`Error::InvalidPublicKey`] if the length of the data slice is not 32 bytes or the
/// slice does not represent a valid Secp256k1 point x coordinate.
#[deprecated(since = "TBD", note = "Use `from_byte_array` instead.")]
#[inline]
pub fn from_slice(data: &[u8]) -> Result<XOnlyPublicKey, Error> {
match <[u8; constants::SCHNORR_PUBLIC_KEY_SIZE]>::try_from(data) {
Expand Down
1 change: 1 addition & 0 deletions src/schnorr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ impl Signature {
pub fn from_byte_array(sig: [u8; constants::SCHNORR_SIGNATURE_SIZE]) -> Self { Self(sig) }

/// Creates a `Signature` directly from a slice.
#[deprecated(since = "TBD", note = "Use `from_byte_array` instead.")]
#[inline]
pub fn from_slice(data: &[u8]) -> Result<Signature, Error> {
match data.len() {
Expand Down
Loading