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
The transport layer should not know about events #2420
Conversation
e935890
to
bc50866
Compare
rebasing this |
This changes the transport class to receive a list of message instead of a list of events, which removes the necessity of encoding the event from the transport and moves it to the raiden service. fixes raiden-network#2419 [ci integration]
bc50866
to
3031a70
Compare
Rebased on master and fixed conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we should not be decoding the events in the transport layer but at one layer above it
@@ -207,7 +208,7 @@ def __init__(self, discovery, udpsocket, throttle_policy, config): | |||
def start( | |||
self, | |||
raiden: RaidenService, | |||
queueids_to_queues: typing.Dict[QueueIdentifier, typing.List[Event]], | |||
queueids_to_queues: typing.Dict[QueueIdentifier, typing.List[Message]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use QueueIdsToQueues here, and change the type there
for event in events: | ||
message = _event_to_message(event, node_address) | ||
self._raiden_service.sign(message) | ||
def _send_queued_messages(self, queueids_to_queues): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to adapt all uses of _queueids_to_queues
to iterate over Message
s instead of SendMessageEvent
s, or possibly just plain in
test:
_receive_delivered
_send_with_retry
send_async
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow, what are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The methods I enumerated above use _queueids_to_queues
as the values being List[SendMessageEvent]
, specifically iterating over the events and comparing the message_identifier
of the message with the attribute of the same name of the event. If now the QueueIdsToQueues
is an alias for Dict[QueueIdentifier, List[Message]]
, you need to update the type alias and ensure these places where this mapping is used comply with this new signature. If the Message
instance passed to transport is the same inside the various versions/instances of this map, or all Message
instances implements correctly equality comparison, you may want to optimize these lookups by using something like message in self._queueids_to_queues[queue_identifier]
, instead of the any
iterator.
Both transports currently basically do `send_async` on each message on these queues anyway. This was done once to allow batch send, but it was never implemented and matrix doesn't even support batch send messages, so it was useless and cluttered the interface (send_async receives a queue of events and a Message, and start received a queue of messages since #2420). The transports need to be aware of the events queues anyway, to eventually stop retrying the message once it's cleaned (TODO for UDP), so we can't avoid knowing about the events, but now the serialization always occurs either in RaidenEventHandler or in RaidenService when starting the transports, so consistent usage of `messages.message_from_sendevent` Followup of #2420 and replaces #2458 [ci integration]
This changes the transport class to receive a list of message instead of
a list of events, which removes the necessity of encoding the event from
the transport and moves it to the raiden service.
fixes #2419
[ci integration]