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

Refactor raiden messages as dataclasses #3253

Closed
4 tasks
andrevmatos opened this issue Jan 10, 2019 · 6 comments
Closed
4 tasks

Refactor raiden messages as dataclasses #3253

andrevmatos opened this issue Jan 10, 2019 · 6 comments

Comments

@andrevmatos
Copy link
Contributor

andrevmatos commented Jan 10, 2019

(Discussion deadline: 2019-01-25)

Abstract

Currently, raiden transport messages gathered a lot of unneeded complexity. They were initially developed for python2, targeting binary serialization (udp transport), and then adapted to be JSON-serialized for Matrix, and with current tools, implentation could be way smaller, easier to understand, expand and version.

Motivation

The binary serialization is still depended upon as certain parts, as additional_hash is the hash of the binary representation of the whole message, and the _data_to_sign, the binary packing of the relevant members for the smartcontracts.
Nonetheless, these classes contain very little logic, and mostly carry the message data, making it very suitable for dataclasses, with some introspection making way for binary serialization as well.

Specification

My suggestion is to use attrs, which is already pulled as sub-dependency, possibly with help of cattrs for [de]serialization. These tools play very well with type annotations, making for a good declarative syntax of the structure and data included in the messages.
I prefer attrs over python 3.7's dataclasses mainly because of improved support for converters and validators. If we use sized data types (#3051), introspection can greatly simplify [de]serialization algorithms, as well as allowing strong data integrity/consistency guarantees.

Suggested approach:

Some inspiration can be drawn from this old branch/experiment: https://github.com/andrevmatos/raiden/blob/refactor_messages/raiden/messages2.py

Backwards Compatibility

Proposal above should be fully compatible with current implementations (same binary serialization, hashing of additional_data, data to be signed, and JSON encoding as well), while simplifying codebase on these matters and helping understand, expand and enforce types and limits.

@andrevmatos
Copy link
Contributor Author

This approach could be extended in the future for other dataclasses in core as well, specifically in states, state changes and internal events (TBD)

@ulope
Copy link
Collaborator

ulope commented Jan 16, 2019

In Monday's transport teem meeting we decided on a deadline for discussion on this issue no later than 2019-01-25.

@hackaugusto
Copy link
Contributor

Please, don't go about refactoring the message without thinking about the rest of the code base. We should avoid using different approaches for the messages/state machines/data transfer object/etc.

@ulope
Copy link
Collaborator

ulope commented Jan 28, 2019

(This has been talked about before, but just to document it in writing as well)
Related: #3302
When we're taking this on it would be a good opportunity to use better datastructures.

@andrevmatos
Copy link
Contributor Author

As @hackaugusto said, we shouldn't do it independently of the rest of dataclasses on Raiden, but as transport's messages has some additional requirements to them (mainly, ability to binary-and-ordered-fields [de]serialize), and also have way more limited scope (they're accessed, serialized and deserialized in very specific places inside transport and messages handlers), I'd like to push for scoping this issue only for transport's messages and using it as a playground for this dataclass implementation, to be expanded to the rest of the dataclasses after it.
Implementation wise, I also liked pyrsistent, and would like to push doing this limited experimentation with it, which if mid-way shown as not enough, would not lose too much work and allow for simpler refactoring into any other solution may arise in the meantime.

@karlb
Copy link
Contributor

karlb commented Mar 21, 2019

To throw in one more option: dataclasses could be used with https://pypi.org/project/marshmallow-dataclass/ for (de)serialization

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

No branches or pull requests

5 participants