Decouple Transport from RaidenService

[andrevmatos]

(Discussion deadline: 2019-01-25)

Abstract

Even though tranport is logically a child of RaidenService, it receives RaidenService in start and uses it in a couple of places, tightly coupling both bi-directionally and making it harder to use transports for anything outside RaidenService (like MS and PFS).

Motivation

Transport role can be described as: given an own address and some configuration, it can listen to messages sent to its address, and send messages to a target address. This role doesn't strictly require raiden service, but it's used all over the place for specific things that aren't specific to RaidenService. We need to decouple this interface, and make transport not depend directly on RaidenService.

Specification

These are some of the uses by MatrixTransport and suggestions on how they could be abstracted away from service:

• RaidenService.address: can be passed as parameter to transport.start
• RaidenService.handle_state_change two uses, one for start-time's ActionUpdateTransportAuthData and other for ActionChangeNodeNetworkState aka update partners presence. Both can be replaced by registering callbacks and firing the actions from functions/methods on the callbacks, like done for AlarmTask
• RaidenService.chain.network_id: can come from config on __init__ or start
• state's queueids_to_queues: specifically used to check when a message is removed from the queue and stop retrying. This bottom-up check should be replaced by an explicit method transport.stop_retry(queue, message) (or other meaningful name) and called by user when transport should give up retrying a message (e.g. on Processed or channel closed)
• RaidenService.on_message: similar to handle_state_change, should be turned into a callback.
• RaidenService.sign, private_key: should be replaced by a small signer helper service/object, passed as parameter to init or start, able to sign string/bytes and Messages with key from address. This abstraction will also allow easily implementing different signing backends/methods, like remote signing (Get rid of local signing #1402) (as we already implemented eth_sign/EIP191)

Backwards Compatibility

These changes should be completely backwards compatible, and not impact much the rest of the Raiden's architecture outside of transports.

[LefterisJP]

I like the idea of decoupling transport from Raiden Service. Thank you for the detailed issue on how this can be achieved.

So apart from the easy ones like the address change it seems you suggest to achieve this by changing composition to callback based relationship.

[andrevmatos]

Yes, at least for the "events" coming from the transport. Another similar (callback-based) approach is some pattern like EventEmitter, but as the normal callback pattern is already present in AlarmTask and MatrixClient, I think it'd be better for now

[ulope]

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

[ulope]

Thanks @andrevmatos for the detailed writeup. I agree that this is something we should do. It will also help us get rid of the circular dependency.

Things that come to mind (in no particular order):

• MatrixTransport._handle_message uses helpers from raiden.messages to turn the incoming bytes / json into Message instances. Ideally this should also be decoupled and put into its own helper.
• If we want to re-use the transport for MS / PFS we will need to think about where the transport should live. Currently it's deep inside the raiden package structure and also heavily imports from within raiden. I don't know if PFS / MS currently have raiden as a dependency. Given the existence of raiden_libs I'd guess not. (@palango or @karlb can you comment on that?)
• Once the above is solved somehow it should be a lot simpler to use the transport from within the PFS and MS (MS needs a public Matrix room to receive balance proofs / monitoring requests #3243)

[palango]

I don't know if PFS / MS currently have raiden as a dependency. Given the existence of raiden_libs I'd guess not.

No, not currently. But I guess at some point we'll need this anyways.