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

mobile: add toString & rlp/json encoding for protocol types #14454

Merged
merged 1 commit into from May 11, 2017

Conversation

Projects
None yet
3 participants
@karalabe
Member

karalabe commented May 10, 2017

Fixes #14443

This PR adds support for marshaling base consensus types (headers, blocks, transactions and receipts) to/from RLP and JSON. These are useful if a mobile client wants to interact with some third party service requiring these data structures without needing to write the encoders in Java/Swift (which will go stale with any data structure modification).

The PR also makes all base types implement the stringer interface so the consensus types can just be printed to console for debugging purposes.

@ligi PTAL

@karalabe karalabe added this to the 1.6.2 milestone May 10, 2017

@ligi

This comment has been minimized.

Show comment
Hide comment
@ligi

ligi May 10, 2017

Member

YAY LGTM 👍 Thanks!

Member

ligi commented May 10, 2017

YAY LGTM 👍 Thanks!

@karalabe karalabe changed the title from mobile: allow encoding transactions to rlp (raw form) to mobile: add toString & rlp/json encoding for protocol types May 11, 2017

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe May 11, 2017

Member

@ligi Souped up the PR. It can encode/decode all consensus types to/from RLP/JSON.

Tiny white lie, encoding/decoding JSON for entire blocks doesn't work as it needs a follow up fix for the Go types package to support that since currently it does not directly. Will do that eventually.

Member

karalabe commented May 11, 2017

@ligi Souped up the PR. It can encode/decode all consensus types to/from RLP/JSON.

Tiny white lie, encoding/decoding JSON for entire blocks doesn't work as it needs a follow up fix for the Go types package to support that since currently it does not directly. Will do that eventually.

@ligi

This comment has been minimized.

Show comment
Hide comment
@ligi

ligi May 11, 2017

Member

@karalabe Very cool - can't wait to try it out - will do once it hits maven-central. Hope the increased complexity does not delay the merge too much - my main use-case seemed already covered by the single digit lines change before ;-)
That said - the json im/export looks handy and I can drop some code for converting between geth and walleth types - thanks!

Member

ligi commented May 11, 2017

@karalabe Very cool - can't wait to try it out - will do once it hits maven-central. Hope the increased complexity does not delay the merge too much - my main use-case seemed already covered by the single digit lines change before ;-)
That said - the json im/export looks handy and I can drop some code for converting between geth and walleth types - thanks!

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe May 11, 2017

Member

Well, the code is quite trivial so it shouldnt really matter compared to the original PR. It just calls a few encode/decode methods. That's why I didn't fix up the core type, which would have needed a more thorough view.

Member

karalabe commented May 11, 2017

Well, the code is quite trivial so it shouldnt really matter compared to the original PR. It just calls a few encode/decode methods. That's why I didn't fix up the core type, which would have needed a more thorough view.

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe
Member

karalabe commented May 11, 2017

@ligi 💥

@karalabe karalabe merged commit 7c707d1 into ethereum:master May 11, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ligi

This comment has been minimized.

Show comment
Hide comment
@ligi

ligi May 11, 2017

Member

@karalabe @Arachnid Thanks - awesome speed - love it - big up!

Member

ligi commented May 11, 2017

@karalabe @Arachnid Thanks - awesome speed - love it - big up!

@ligi

This comment has been minimized.

Show comment
Hide comment
@ligi

ligi May 12, 2017

Member

Just tried it out in walleth - works great!

Member

ligi commented May 12, 2017

Just tried it out in walleth - works great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment