Skip to content

Change from_bytes methods to take fixed-size array argument#230

Merged
tarcieri merged 1 commit intodalek-cryptography:release/2.0from
mina86:a
Dec 18, 2022
Merged

Change from_bytes methods to take fixed-size array argument#230
tarcieri merged 1 commit intodalek-cryptography:release/2.0from
mina86:a

Conversation

@mina86
Copy link
Copy Markdown
Contributor

@mina86 mina86 commented Dec 1, 2022

Change from_bytes methods of all the types to take &[u8; N] argument
(with N appropriate for given type) rather than &[u8]. This does
a few things:

  • it makes SecretKey::from_bytes infallible which will simplify call
    sites (if the call site already has properly-sized array),
  • harmonises the convention with ed25519::Signature::from_bytes method
    and
  • encodes the size of the bytes representation in type system which
    means that users can assert it at compile time.

To still allow creating the objects from a slice whose size is checked
at run time, introduce TryFrom<&[u8]> implementations for affected
public types.

This is an API breaking change. The simplest way to update existing
code is to replace Foo::from_bytes with Foo::try_from. This should
cover majority of uses.

@tarcieri
Copy link
Copy Markdown
Contributor

tarcieri commented Dec 1, 2022

In general I'd suggest holding off on this until #225 is addressed, since that proposes to change the public APIs for keys

@tarcieri
Copy link
Copy Markdown
Contributor

tarcieri commented Dec 1, 2022

This was also an interesting read on the actual costs of length checks: https://blog.readyset.io/bounds-checks/

tl;dr: not much, especially in cases like this

@mina86
Copy link
Copy Markdown
Contributor Author

mina86 commented Dec 1, 2022

The main thing is that creation of SecretKey is now infallible. Another is that the expected length is now expressed in type system. Performance isn’t really my main concern.

@tarcieri
Copy link
Copy Markdown
Contributor

tarcieri commented Dec 1, 2022

I'd probably suggest making the TryFrom impls take &[u8] and using inherent methods (i.e. from_bytes) for the fixed-sized inputs. That's the approach we use with @RustCrypto.

@mina86
Copy link
Copy Markdown
Contributor Author

mina86 commented Dec 1, 2022

I can change from_bytes if you prefer though that is an API breaking change.

@mina86
Copy link
Copy Markdown
Contributor Author

mina86 commented Dec 1, 2022

So I’m looking at ed25519::Signature::from_bytes from a preview ed25519 crate and it has from_bytes(&[u8]) so should this follow that instead? Or should ed25519 crate be changed as well?

@tarcieri
Copy link
Copy Markdown
Contributor

tarcieri commented Dec 1, 2022

I'd be open to changing that method

@mina86 mina86 changed the title Provide routines for creating objects from fixed-sized arrays Change from_bytes methods to take fixed-size array argument Dec 2, 2022
@mina86
Copy link
Copy Markdown
Contributor Author

mina86 commented Dec 2, 2022

Updated as per suggestion. Happy to wait till #225 gets resolved.

@tarcieri tarcieri mentioned this pull request Dec 12, 2022
@tarcieri
Copy link
Copy Markdown
Contributor

#225 is resolved

@tarcieri
Copy link
Copy Markdown
Contributor

@mina86 you need to change the base to release/2.0

@mina86 mina86 changed the base branch from main to release/2.0 December 18, 2022 18:40
Change from_bytes methods to take `&[u8; N]` argument (with `N`
appropriate for given type) rather than `&[u8]`.  This harmonises
the convention with SigningKey and ed25519::Signature; helps type
inference; and allows users to assert bytes size to be asserted at
compile time.

Creating from a slice is still possible via `TryFrom<&[u8]>` trait.

This is an API breaking change.  The simplest way to update existing
code is to replace Foo::from_bytes with Foo::try_from.  This should
cover majority of uses.
@pinkforest pinkforest mentioned this pull request Dec 18, 2022
@tarcieri tarcieri merged commit f0b2df0 into dalek-cryptography:release/2.0 Dec 18, 2022
@mina86 mina86 deleted the a branch December 18, 2022 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants