Update Ed25519Signature ser/de logic #756
Conversation
|
I figured out the reason of |
| let bytes = if deserializer.is_human_readable() { | ||
| let s = String::deserialize(deserializer)?; | ||
| base64ct::Base64::decode_vec(&s).map_err(|e| de::Error::custom(e.to_string()))? | ||
| struct Ed25519SignatureVisitor { |
There was a problem hiding this comment.
If we just put #[derive(Deserialize, Serialize)] on top of Ed25519Signature and #[serde(skip)] on top of bytes, we can save the custom ser/de logic here. The downsides are:
- Human readable data is not base64, but something less compact.
- We will not be using the
bytesonce_cell "cache".
There was a problem hiding this comment.
This is excellent!
I think that the only thing that is missing is a few lines of english in a comment of
impl (De)Serializesummarizing what invariants the programmer should maintain when editing the inner representation of our types:
- we use this disjunction between human-readable and non-human readable formats since crypto: Add Sui + Aggregation support #460,
- at the same time we aim to have serde_reflection processable types, which works poorly with aliases and nameless types.
Here's why I think a comment about this is necessary: you're making specific choices here (e.g. the marker fields for disjunction between b64 and raw formats) that we may debate in the future. If someone was to make different ones, what shouldn't they break?
Thanks for the feedback. Added a comment with the information outlined above.
There was a problem hiding this comment.
This is excellent!
I think that the only thing that is missing is a few lines of english in a comment of impl (De)Serialize summarizing what invariants the programmer should maintain when editing the inner representation of our types:
- we use this disjunction between human-readable and non-human readable formats since crypto: Add Sui + Aggregation support #460,
- at the same time we aim to have serde_reflection processable types, which works poorly with aliases and nameless types.
Here's why I think a comment about this is necessary: you're making specific choices here (e.g. the marker fields for disjunction between b64 and raw formats) that we may debate in the future. If someone was to make different ones, what shouldn't they break?
| @@ -51,6 +52,38 @@ fn serialize_deserialize() { | |||
| assert!(bincode::deserialize::<Ed25519PublicKey>(&bytes).is_err()); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn custom_serde_reflection() { | |||
| @@ -126,18 +126,6 @@ fn get_registry() -> Result<Registry> { | |||
| tracer.trace_type::<HeaderDigest>(&samples)?; | |||
| tracer.trace_type::<CertificateDigest>(&samples)?; | |||
|
|
|||
| // Caveat: the following (trace_type) won't work, but not because of generics. | |||
There was a problem hiding this comment.
Indeed, this is quite an antiquated comment.
| ser::SerializeStruct, | ||
| Deserialize, Serialize, | ||
| }; | ||
| use serde_bytes::{ByteBuf, Bytes}; |
There was a problem hiding this comment.
Nit (that you can ignore): we already use serde_with elsewhere, and it has analogous byte types. But that may be a bit too much OCD on my part.
https://docs.rs/serde_with/latest/serde_with/
There was a problem hiding this comment.
Thanks for the suggestion. serde_with::Bytes seems to be missing some features compared to serde_bytes::Bytes, e.g. wrapping &[u8]. It is a bit hard to use it in the situation here.
|
lgtm! should we add similar tracer test to other serializer for Secp256k1 and BLS signatures (in a follow up PR)? or, port over the format tests from sui to narwhal as well? |
Good idea, Narwhal does have a similar |
* Update signature serde * fixup! Update signature serde * fix Ed25519Signature ser/de * add comment
Implement struct ser/de logic for
Ed25519Signature. The current ser/de logic ofEd25519Signaturejust returns a&[u8], making it being treated as[u8]byserde_reflectionand causing ser/de failures.I investigated if we can rely on
serdeannotations and avoid manual implementation of struct ser/de. But it seems there is no straightforward way unless we want to compromise onbytesfor ser/de.