Skip to content
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

Remove a bajillion manual serde implementations #433

Merged
merged 15 commits into from
May 22, 2020
Merged

Conversation

timvermeulen
Copy link
Contributor

@timvermeulen timvermeulen commented May 20, 2020

This PR adds serde_tuple and serde_repr dependencies in order to remove by far most of our manual Serialize/Deserialize implementations.

BlockHeader is the main exception to this because for whatever reason serde_tuple and derive_builder don't seem to work well together.

Other minor related changes:

  • Rename types/src/sector/serde.rs to serde_tests.rs because after removing the Serialize/Deserialize implementations, the tests were all that was left
  • Add bitvec_serde::{serialize, deserialize} method in order to be used as #[serde(with = "bitvec_serde")] elsewhere
  • Turn the Randomness type alias into a proper wrapper struct that correctly handles serialization
  • Remove the Byte32De and BytesDe types as they're no longer needed
  • Add extern crate serde; in a couple places as workaround for a compiler bug

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's awesome that this library works now, my issue with this is what the code gets expanded to, I can barely read the expanded code after these changes, which is a huge disadvantage in itself and the expanded code is less concise. I do love how it makes the code more readable, but if it means we can't expand to view macro usage, it's a disadvantage.

Can other try cargo expanding themselves to see if they get same issue? I can't reproduce anywhere but here

blockchain/blocks/src/ticket.rs Show resolved Hide resolved
encoding/src/bytes.rs Outdated Show resolved Hide resolved
types/src/sector/post.rs Show resolved Hide resolved
utils/bigint/src/bigint_ser.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/cron/mod.rs Show resolved Hide resolved
vm/actor/src/builtin/cron/state.rs Show resolved Hide resolved
vm/src/randomness.rs Outdated Show resolved Hide resolved
@austinabell
Copy link
Contributor

So changed my mind on this (especially with cargo expand issue being resolved). If someone could also make a thorough pass to make sure of the field ordering and no bytes are serialized without tagging them to be serialized as byte string, that would be great!

Definitely with this now we have to be very careful of looking at field ordering to match go impl's but in the long run it should be easier. I also didn't know about the serde_repr derives, definitely much cleaner to use this and the tuple serialize macro (even if a bit more unreadable expanded code, that doesn't matter much)

Also closes #185 (very outdated and was created at very start of serde usage)

Copy link
Contributor

@dutterbutter dutterbutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥🔥🔥🔥🔥🔥 this is great!

Copy link
Member

@ec2 ec2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This is amazing.

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming you've decided to keep the randomness wrapper struct. I do believe it makes it much less ergonomic if you have to interact with the wrapper type when unnecessary because the wrapper is only used for deserialization, and no custom functionality is needed, but can always switch later if it becomes annoying I guess

@@ -10,7 +10,7 @@ use serde_bytes::ByteBuf;
pub struct BytesSer<'a>(#[serde(with = "serde_bytes")] pub &'a [u8]);

/// Wrapper for deserializing dynamic sized Bytes.
#[derive(Deserialize, Serialize)]
#[derive(Deserialize)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: to include Serialize on these deserialize types

It's nice to have it symmetric if you need to use one type for serializing and deserializing (no actual examples in code rn but can be useful to move values instead of serializing a reference to them).

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new changes lgtm

@timvermeulen timvermeulen merged commit acedcf0 into master May 22, 2020
@timvermeulen timvermeulen deleted the tim/serde_tuple branch May 22, 2020 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
encoding hashing and serialization Status: Merge Ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants