-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Derive Serialize/Deserialize for certain simple structs #1565
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there are a number of types here that already have canonical serialization, either to protobuf
or string representations. Please reuse those serialized forms instead.
@@ -67,7 +68,7 @@ const ZFUTURE_TX_VERSION: u32 = 0x0000FFFF; | |||
/// that have been mined. | |||
/// - For v5 transactions onwards, this identifier is derived only from "effecting" data, | |||
/// and is non-malleable in all contexts. | |||
#[derive(Clone, Copy, PartialOrd, Ord, PartialEq, Eq, Hash)] | |||
#[derive(Clone, Copy, PartialOrd, Ord, PartialEq, Eq, Hash, Serialize, Deserialize)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use hand-written Serialize
and Deserialize
impls that serialize to the canonical txid text string representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Also I dont see the definition for the canonical string rep - could you point me to it? I do see Display
implemented so maybe that is it? Is there a FromStr
for TxId
anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't needed to implement parsing, but yes the string representation used in the Display
impl is canonical.
@nuttycom w.r.t. Proposal and associated structs, we need it to be serializable for |
As per @daira 's suggestion, we will try using the protobuf encoding to do the passing back and forth. |
This PR introduces "non-controversial" derives of
Serialize
andDeserialize
for some structs that we persist to storage in our memory wallet implementation. This is by no means all the structs that we want to be able to serialize, but they are the ones that should be stable or are simple internal representations.