Chore: ed25519 dalek update in next
#4382
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## next #4382 +/- ##
===========================================
+ Coverage 55.87% 66.10% +10.23%
===========================================
Files 447 447
Lines 320060 320041 -19
===========================================
+ Hits 178820 211551 +32731
+ Misses 141240 108490 -32750 ☔ View full report in Codecov by Sentry. |
kantai
force-pushed
the
chore/ed25519-dalek-update
branch
from
f51653a
to
2438f65
Compare
| } | ||
|
|
||
| /// Verify that a given byte string is a well-formed EdDSA public | ||
| /// key (i.e. it's a compressed Edwards point that is valid), and return | ||
| /// a VRFPublicKey if so | ||
| pub fn from_bytes(pubkey_bytes: &[u8]) -> Option<VRFPublicKey> { |
There was a problem hiding this comment.
I know you didn't introduce this function in this PR, but this really should be try_from_bytes(), or even better, impl TryFrom<&[u8]> for VRFPublicKey, and return a Result
There was a problem hiding this comment.
Yeah, I agree, but I don't want to do too much refactoring in this PR (and this would mean a pretty fair amount of refactoring).
| let key = ed25519_dalek::VerifyingKey::from_bytes(&pubkey_slice).ok()?; | ||
| Some(VRFPublicKey(key)) |
There was a problem hiding this comment.
Simpler way to write this:
| let key = ed25519_dalek::VerifyingKey::from_bytes(&pubkey_slice).ok()?; | |
| Some(VRFPublicKey(key)) | |
| ed25519_dalek::VerifyingKey::from_bytes(&pubkey_slice).map(VRFPublicKey).ok() |
There was a problem hiding this comment.
I'm a little conflicted about this kind of thing -- it's actually not obvious to me which of the above is simpler. Sometimes the functional chaining in option and result can be hard to decipher (especially because it often means that the Err type survives to the end of the chain).
| let bytes = hex_bytes(h).ok()?; | ||
| Self::from_bytes(bytes.as_slice()) |
There was a problem hiding this comment.
Slightly simpler:
| let bytes = hex_bytes(h).ok()?; | |
| Self::from_bytes(bytes.as_slice()) | |
| hex_bytes(h).ok().as_deref().and_then(Self::from_bytes) |
There was a problem hiding this comment.
I'm a little conflicted about this kind of thing -- it's actually not obvious to me which of the above is simpler. Sometimes the functional chaining in option and result can be hard to decipher (especially because it often means that the None type survives to the end of the chain).
There was a problem hiding this comment.
I agree with @kantai , I find this kind of chaining to be less clear than the two line version.
There was a problem hiding this comment.
Am I the only one who finds chaining easier to read? It just seems like extra steps to take a value out of an Option/Result and then reconstruct another one, rather than using map()/and_then()
There was a problem hiding this comment.
For me, it depends on the situation. Sometimes I think it is much clearer to chain, other times less so. When there's multiple fallible steps, I think I usually prefer the non-chained version, because with chaining it can be hard to reason about how the error cases are being handled. Conversely, iteration is often much clearer to me when chained rather than manually looped (in particular things like finding an element or folding a sum).
kantai
force-pushed
the
chore/ed25519-dalek-update
branch
from
2438f65
to
bd3d240
Compare
kantai
force-pushed
the
chore/ed25519-dalek-update
branch
from
bd3d240
to
41df072
Compare
Description
As part of Brice working to unify the
TestSignersandSelfSignerstructs, etc., it became clear that having multiple versions of the various rand libraries was going to create headaches. This PR sets workspace versions forrand,rand_core, andrand_chacha, as well as updatesed25519-dalekto facilate some unification here.The biggest interface changes were:
gen_rangeaccepting a range argument rather than two low, high args, and changes to theed25519-dalekinterfaces. As part of this, I removed theDerefimpls forVRFPublicKeyandVRFPrivateKey-- those were only used to invoke theas_bytesandto_bytes, so I just added those explicitly. The problem with havingDerefimpls here is that it meant functions likefrom_byteswere overloaded, which I think can make it unclear which function is actually being invoked.