Closed zacknewman closed 1 month ago
See the existing VerifyingKey::from_bytes
method: https://docs.rs/ed25519-dalek/latest/ed25519_dalek/struct.VerifyingKey.html#method.from_bytes
There's also an existing impl of TryFrom<&[u8]>
. Since point decompression is fallible anyway, a separate TryFrom<&[u8; 32]>
adds little value, especially since [u8; 32]
will deref to &[u8]
.
You've misunderstood the request. I'm not suggesting a TryFrom<&[u8; 32]>
implementation. I'm suggesting a TryFrom<[u8; 32]>
implementation (i.e., converting an array not a slice). Typically in Rust one prefers explicit Clone
s and "expensive" Copy
s instead of taking a reference just to clone
/copy internally. While 32 bytes is not that expensive, it's more than what I'm used to seeing being implicitly copied/clone
d which is what from_bytes
does.
There is no advantage to an owned API over borrowing, only disadvantages. Just borrow the input.
What disadvantages? There is a reason the standard library doesn't have functions like u128::from_ne_bytes
that takes a reference. You're internally needing to own it; thus it makes sense to be explicit with that. You can easily keep the slice conversions, but it's a bizarre API choice unless you can actually provide the "disadvantages". After all, why have two methods that return a slice and array? Why not just have one? Then one can "just borrow the output" or "just dereference the output".
32-bytes is significantly larger than a 64-bit pointer, which is the threshold at which borrowing becomes advantageous over a copy.
Regardless, it makes no sense to support both owned and borrowed APIs here. That’s just needless, pointless complexity. Just borrow and use the existing APIs.
It’s not a “bizarre API choice”. It’s idiomatic Rust. What you are suggesting is a “bizarre API choice”.
So why are you returning32 bytes via to_bytes
since it's "significantly larger"? Just return a pointer.
Because the caller can potentially make use of an owned result. These constructors can’t. They have to do point decoding.
And 16 bytes is significantly larger than a 64-bit pointer; yet core
seems to still take a [u8; 16]
and not a reference for functions like u128::from_ne_bytes
.
Internally the function is owning the result that is why it dereferences the reference; so the constructor needs the owned result just like calling code does.
You’re talking about a value that’s twice the size of a pointer, versus four times. And that’s for consistency with the other methods that are pointer size or smaller.
And they don’t offer both options, they chose the single option that made the most sense for their use case, as have we.
Then I guess you should remove functions like Scalar::from_bytes_mod_order
because it takes the array.
We’re not going to be making any breaking changes any time soon. There is ample previous discussion of that function’s signature I can link you when I’m not on a phone.
I'm making a point about the alleged "consistency". VerifyingKey
implements From<EdwardsPoint>
which internally contains a [u8; 32]
; so if one wants to avoid the implicit copy, one has to add a separate dependency to curve25519-dalek
in order to have direct access to the contained array in CompressedEdwardsY
then manually call decompress
just to pass the EdwardsPoint
into from
.
It would seem that adding a function that converts an array to a VerifyingKey
makes code more consistent and doesn't entail a breaking change.
See #491, and no it’s not more consistent, Scalar::from_bytes_mod_order
is the only API that accepts an owned argument. There is no point to adding duplicate APIs just to save you typing &
.
Again, From<EdwardsPoint>
is implemented by VerifiyingKey
; instead of From<&EdwardsPoint>
despite you're alleged claim of passing arguments that are four times the size of a 64-bit pointer. So yes, it makes it more consistent.
We could certainly count the number of functions that are defined that take arguments that are 32 bytes or more instead of unsupported claims.
This discussion isn’t going anywhere productive. Please review #491.
Currently there is no way to directly convert
[u8; 32]
into aVerifyingKey
. I propose adding aTryFrom<[u8; 32]>
implementation and a correspondingfrom_array
function. This would not only add symmetry since one can already convert to an array viaVerifyingKey::to_bytes
, but it will also allow one to explicitly copy the array instead of the implicit copy that occurs inVerifyingKey::from_bytes
.VerifyingKey::from_bytes
can then be re-written to just redirect to this array conversion instead.