[Alternative] Allow deserializing from owned types #218
Conversation
|
It's definitely less hacky than my #217, but introduces quite some code duplication. I'd like to propose to unify the visitors as implemented in sgeisler@6356ba4. |
Thanks! cherry-picked it modulo some formatting and small changes for readability :) |
|
@elichai Can you rebase this so it is mergable again? @apoelstra Any chance we can get this merged? Not being able to deserialize from owned types is rather inconvenient in some situations. |
| serde_impl!(SecretKey, constants::SECRET_KEY_SIZE); | ||
|
|
||
| #[cfg(feature = "serde")] | ||
| impl ::serde::Serialize for SecretKey { |
There was a problem hiding this comment.
Seems like this is encouraging user to serialize their secrets. Do we really want to be doing that?
There was a problem hiding this comment.
As an alternative, we could provide a module so users have to explicitly opt-in to this by saying #[serde(with = "secp256k1::serde::secret_key")]. This has the downside that we need to provide a module for Option and Vec as well if we want it to work out of the box with those containers.
However, I am not sure if this makes things actually better? Some situations require secret keys to be serialized (a backup functionality for example). Additionally, SecretKey also implements Debug and Display which are basically the same.
I am not convinced that we should be introducing a policy on this abstraction level. Whether or not serializing a secret key is dangerous depends completely on the application.
There was a problem hiding this comment.
I'll just note that this is already the case: https://docs.rs/secp256k1/0.20.0/secp256k1/key/struct.SecretKey.html#impl-Serialize
There was a problem hiding this comment.
(At least as long as we don't have HD derivation natively in this library), serializing secret keys is essential. Keys need to be stored.
In one case, the caller has the secret key in serialized form already and just passes it to the library. Then letting the user serialize keys won't hurt either.
In the other case, the caller relies on the library to generate (or tweak) keys. Then it's essential to serialize them for storage.
There was a problem hiding this comment.
Yeah, concept ACK having serialization. You need to store secret key material somehow.
This PR needs a rebase though.
|
This PR requires some heavier lifting by now. Back then we chose to remove the |
|
Yeah after schnorr and other changes this PR doesn't have a clean path for a rebase and requires some re-thinking and balance between simplicity and readability and code duplication(adding back macros). If anyone wants to take this feel free, otherwise I'll try to find some time for it this weekend / next week |
|
Looks good |
This is an alternative to #217
I removed the macro because only SecretKey used it, trying to remerge all of the implementations into a macro has some problems because some of them use specific functions(from_der() / serialize() etc.) we could add it back it a complicated way (or implement a private serialize function for all the types that use whatever is needed underneath and use that in the macro)
I used the implementation for PublicKey to replace the one in SecretKey and Signature (implemented a similar one, just to realize
PublicKeyhas it's own one that does the same)And added tests that check that (they should fail before this change and pass after)
Anyone that actually uses it please test that there's no difference in the actual input/output in the serializer you're using (it shouldn't, but worth testing against multiple common serializers)