Allow SharedSecret to be created from byte array #417
Conversation
|
Also can you make sure this compiles with rustc 1.29? Sorry, we'll eliminate this requirement in the next major version I think. |
src/ecdh.rs
Outdated
| fn from(arr: [u8; SHARED_SECRET_SIZE]) -> Self { | ||
| SharedSecret(arr) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm personally not keen on adding impl From<Inner> for Newtype in case there is struct Newtype(Inner); It encourages just calling into() which hides the newtype.
I suggest from_bytes(bytes: [u8; SHARED_SECRET_SIZE]) -> Self method instead.
There was a problem hiding this comment.
I don't have a strong preference here. I'm not sure what you mean by "hiding" .. a call to .into is pretty explicit.
There was a problem hiding this comment.
It's not clear which type is converted into which until you review a bigger chunk of code.
There was a problem hiding this comment.
Done. I'm not very familiar with #[inline] usage, let me know if this is needed here as well.
There was a problem hiding this comment.
Another data point: we have secret_bytes() -> [u8; SHARED_SECRET_SIZE] already which is slightly strangely named, the reason is that its the same method name used for various keys, especially KeyPair (for which it makes most sense). In light of this I'd find from_bytes the least surprising name so as to kind of match with secret_bytes.
There was a problem hiding this comment.
#[inline] could be useful/nice but not super-important. Especially if one uses LTO.
There was a problem hiding this comment.
I think it is a poor decision to avoid implementing traits from the standard library which are even part of the standard prelude. The From trait defines a standard interface that allows for better composability, e.g. you can define functions like this
fn process<T: Into<Foo>>(t: T)and it will compile for all types that can be converted into Foo. Also, you can always opt-in for doing the conversion explicitly:
let bar: Bar = foo.into()I really don't understand why we need to segue here out of the standard rust interface for doing conversions.
This was accidentally removed in 8b2edad. See also the discussion on rust-bitcoin#402
|
Looks like the tests have actually passed -- not sure why CI reporting is being weird. |
|
Holding off on publishing new version until #418. |
de65fb2 Implement de/serialization for SharedSecret (Tobin Harding) Pull request description: As we do for other keys implement serde de/serialization for the `SharedSecret`. Please note, this adds `from_slice` and `from_bytes` (borrowed and owner respectively) because I needed to use them. Doing so treads on @dspicher's toes because he is in the process of implementing an owned conversion method for `SharedSecret`. The fair thing to do would be let #417 resolve and merge before merging this one (I can rebase). ~Side note, its kind of rubbish that `BytesVisitor` deserializes into a buffer (presumably) then we reallocate and copy the buffer to use the borrowed conversion method due to the serde function signature `fn visit_bytes<E: de::Error>(self, v: &[u8]) -> Result<Self::Value, E>`~ (I was bumbling nonsense.) Closes: #416 ACKs for top commit: apoelstra: ACK de65fb2 Tree-SHA512: 3d484f160d8a459a867f645736713984bad982429236ac5351c20b6c21b42cec86e68009fe7adf062912037cf7e747e5b15357a5fd7900e52169f208a4e56710
This was accidentally removed in 8b2edad. See also the discussion
on #402
Closes #416.