Deprecate from_slice methods in favor of arrays #737

jamillambert · 2024-09-10T19:55:57Z

As brought up in issue rust-bitcoin/rust-bitcoin#3102 support for Rust arrays is now much better so slice-accepting methods that require a fixed length can be replaced with a method that accepts an array.

from_slice() methods have been deprecated and calls to it from within the crate have been changed to use the equivalent array method.

tcharding

Only did shallow review, but everything apart from try_from thing looks good to me, my reviews don't carry much weight in this repo though.

src/key.rs

tcharding · 2024-09-10T22:36:34Z

There are currently not formatting issues on master so there should be no need for a separate formatting patch in this PR bro.

jamillambert · 2024-09-11T06:53:59Z

Third patch removed and incorporated in first two

Kixunil

Hard no deprecating PublicKey::from_slice (but only that one). Other suggestions are not that critical but I'd obviously prefer to fix them.

src/key.rs

jamillambert · 2024-09-11T10:37:18Z

Removed deprecation of PublicKey::from_slice and addressed other suggestions. Thanks.

apoelstra · 2024-09-11T16:17:30Z

code review ack 46e06da except that the expect message is still wrong. Honestly I think you should just unwrap here. It is very obvious that let x: [u8; N] = slice[..N].try_into() will succeed and why.

Functions have been added to PrivateKey, PublicKey and XOnlyPublicKey to allow the creation of a key directly from a byte array.

The PrivateKey and XOnlyPublicKey from_slice functions have been deprecated and calls to them from within the crate have been changed to use the equivalent array function.

jamillambert · 2024-09-11T17:58:33Z

Thanks for the review. I have replaced expect with unwrap.

apoelstra

ACK 537b85b successfully ran local tests

tcharding

ACK 537b85b

My suggestions are just because we all enjoy coding round here and it amused me to find a way to re-write your changes using combinators.

tcharding · 2024-09-12T01:28:52Z

src/key.rs

    pub fn from_slice(data: &[u8]) -> Result<XOnlyPublicKey, Error> {
-        if data.is_empty() || data.len() != constants::SCHNORR_PUBLIC_KEY_SIZE {
-            return Err(Error::InvalidPublicKey);
+        match <[u8; constants::SCHNORR_PUBLIC_KEY_SIZE]>::try_from(data) {
+            Ok(data) => Self::from_byte_array(&data),
+            Err(_) => Err(InvalidPublicKey),
        }
+    }


This function could be written with combinators like this:

pub fn from_slice(data: &[u8]) -> Result<XOnlyPublicKey, Error> { <[u8; constants::SCHNORR_PUBLIC_KEY_SIZE]>::try_from(data) .map_err(|_| InvalidPublicKey) .and_then(|data| Self::from_byte_array(&data)) }

It does look quite neat with a combinator

tcharding · 2024-09-12T01:31:16Z

src/ecdh.rs

@@ -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.")]


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))) }

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

apoelstra · 2024-09-12T15:00:17Z

I also prefer the combinator version, though of course I won't hold up the PR over it.

But @Kixunil we need you to take a look at this one again since your status is "change requested".

Kixunil · 2024-09-12T16:28:41Z

I find match cleaner in cases like this where we're transforming stuff in both branches but if we want combinators then I'd use ? instead of and_then.

Kixunil

ACK 537b85b

Kixunil · 2024-09-12T16:40:54Z

src/key.rs

-            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();


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.

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?

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.)

Kixunil · 2024-09-12T16:42:51Z

src/key.rs

@@ -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]),


LOL, the previous code...

tcharding reviewed Sep 10, 2024

View reviewed changes

src/key.rs Outdated Show resolved Hide resolved

jamillambert force-pushed the 0906-from_slice branch from fd5649b to d612dc0 Compare September 10, 2024 21:23

jamillambert force-pushed the 0906-from_slice branch from d612dc0 to 0cb6b30 Compare September 11, 2024 06:53

Kixunil requested changes Sep 11, 2024

View reviewed changes

jamillambert force-pushed the 0906-from_slice branch from 0cb6b30 to 1a6339a Compare September 11, 2024 10:31

jamillambert force-pushed the 0906-from_slice branch from 1a6339a to 46e06da Compare September 11, 2024 10:53

jamillambert added 2 commits September 11, 2024 18:56

Add from_byte_array functions

1661f57

Functions have been added to PrivateKey, PublicKey and XOnlyPublicKey to allow the creation of a key directly from a byte array.

Deprecate and replace from_slice

537b85b

The PrivateKey and XOnlyPublicKey from_slice functions have been deprecated and calls to them from within the crate have been changed to use the equivalent array function.

jamillambert force-pushed the 0906-from_slice branch from 46e06da to 537b85b Compare September 11, 2024 17:57

apoelstra approved these changes Sep 11, 2024

View reviewed changes

tcharding approved these changes Sep 12, 2024

View reviewed changes

Kixunil approved these changes Sep 12, 2024

View reviewed changes

apoelstra mentioned this pull request Sep 12, 2024

fix docs for new clippy lint #740

Merged

apoelstra merged commit 3453adb into rust-bitcoin:master Sep 12, 2024
30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate from_slice methods in favor of arrays #737

Deprecate from_slice methods in favor of arrays #737

jamillambert commented Sep 10, 2024

tcharding left a comment

tcharding commented Sep 10, 2024 •

edited

Loading

jamillambert commented Sep 11, 2024

Kixunil left a comment

jamillambert commented Sep 11, 2024

apoelstra commented Sep 11, 2024

jamillambert commented Sep 11, 2024

apoelstra left a comment

tcharding left a comment

tcharding Sep 12, 2024

jamillambert Sep 12, 2024

tcharding Sep 12, 2024

jamillambert Sep 12, 2024

apoelstra commented Sep 12, 2024

Kixunil commented Sep 12, 2024

Kixunil left a comment

Kixunil Sep 12, 2024

apoelstra Sep 12, 2024

Kixunil Sep 12, 2024

apoelstra Sep 12, 2024

Kixunil Sep 12, 2024

Deprecate from_slice methods in favor of arrays #737

Deprecate from_slice methods in favor of arrays #737

Conversation

jamillambert commented Sep 10, 2024

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Sep 10, 2024 • edited Loading

jamillambert commented Sep 11, 2024

Kixunil left a comment

Choose a reason for hiding this comment

jamillambert commented Sep 11, 2024

apoelstra commented Sep 11, 2024

jamillambert commented Sep 11, 2024

apoelstra left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Sep 12, 2024

Kixunil commented Sep 12, 2024

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Sep 10, 2024 •

edited

Loading