-
Notifications
You must be signed in to change notification settings - Fork 18
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
DA: Executor behaviour + swarm #768
Conversation
Ok(()) | ||
} | ||
|
||
pub fn local_peer_id(&self) -> &PeerId { |
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.
dial
and local_peer_id
are a wrapper functions for interacting with the libp2p swarm. I think it's a good decision because we might tweak this logic without changing the backend. In the same sense we could expose blob_sender
and open_stream_sender
channels, they'll be needed by the executor also.
self.swarm.local_peer_id() | ||
} | ||
|
||
pub fn protocol_swarm(&self) -> &Swarm<ExecutorBehaviour<Membership>> { |
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.
For executor swarm to hide libp2p completely we'd need to expose listen_on
and sampling_sender
(same applies to the validator swarm). From the looks of it, after we gather all these swam communication channels and related data, we move the whole swarm into a thread and run it, so ownership is lost, but we don't require it anymore either.
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.
For executor swarm to hide libp2p completely we'd need to expose
listen_on
andsampling_sender
(same applies to the validator swarm). From the looks of it, after we gather all these swam communication channels and related data, we move the whole swarm into a thread and run it, so ownership is lost, but we don't require it anymore either.
Yeah, makes sense. They can be exposed later when needed maybe.
Related to the communication channels. As we have to take ownership, it is difficult to wrap them in methods, I think they are not clone. But I can try.
a88d53b
to
8636e43
Compare
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.
Looks good, thanks! (errors not related and will be solved after https://github.com/status-im/infra-misc/issues/364)
Added executor composited behaviour and executor swarm.