Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

rlp_derive #6125

Merged
merged 7 commits into from
Aug 20, 2017
Merged

rlp_derive #6125

merged 7 commits into from
Aug 20, 2017

Conversation

debris
Copy link
Collaborator

@debris debris commented Jul 24, 2017

this pr introduces 4 new derives:

  • RlpEncodable - encodes a structure a list
  • RlpDecodable - decodes structure from a list
  • RlpEncodableWrapper - encodes structure with a single field, without declaring new list
  • RlpDecodableWrapper decodes structure with a single field

Possible improvements:
encoding and decoding enums

@debris debris requested a review from rphmeier July 24, 2017 08:52
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 24, 2017
@debris
Copy link
Collaborator Author

debris commented Jul 24, 2017

requires: rust 1.19 which has rust-lang/rfcs#1506

@rphmeier
Copy link
Contributor

@General-Beck ^

@debris What makes me worry about this is that we will know that the structs serialize and deserialize to RLP, but it's a lot harder to know if it's the correct RLP now. This is important for stuff like protocol structures, and existing structures we've written to old databases.

@debris
Copy link
Collaborator Author

debris commented Jul 24, 2017

@rphmeier I agree, that's why this pr needs careful review. Long term, I believe it's much needed, cause it simplifies our code and reduces amount of boilerplate, the same way serde_derive reduces it for serde.

@General-Beck
Copy link
Contributor

@debris @rphmeier After the release we will switch CI to version 1.19

@debris debris added the A4-awaitingci 🤖 Pull request is waiting for changes on the CI to complete tests before review/merge can begin. label Jul 30, 2017
@debris debris removed the A4-awaitingci 🤖 Pull request is waiting for changes on the CI to complete tests before review/merge can begin. label Jul 31, 2017
@debris debris added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 1, 2017
@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Aug 14, 2017
@@ -76,6 +76,8 @@ extern crate futures;
extern crate itertools;
extern crate rand;
extern crate rlp;
#[macro_use]
extern crate rlp_derive;
Copy link
Contributor

@rphmeier rphmeier Aug 16, 2017

Choose a reason for hiding this comment

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

could you toss this at the beginning of the imports with other macro_use guys?

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 16, 2017
@rphmeier
Copy link
Contributor

There are already tests for the light client which directly serialize the messages with manual RLP streaming (for this reason exactly) so I am not worried about PIP breaking.

Generally, having a nice suite import/regression tests (parity-import-tests repo) will protect against regressions here as well.

@gavofyork gavofyork merged commit 407c8c3 into master Aug 20, 2017
@gavofyork gavofyork deleted the rlp_derive branch August 20, 2017 04:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants