Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Add ProtocolBehaviour#2922

Merged
gavofyork merged 8 commits into
paritytech:masterfrom
tomaka:protocol-behaviour
Jun 24, 2019
Merged

Add ProtocolBehaviour#2922
gavofyork merged 8 commits into
paritytech:masterfrom
tomaka:protocol-behaviour

Conversation

@tomaka

@tomaka tomaka commented Jun 21, 2019

Copy link
Copy Markdown
Contributor

The next step in the refactoring is to implement the libp2p NetworkBehaviour trait on Protocol. This was done by adding a ProtocolBehaviour struct.

In other words:
Before: CustomProto <-> Service <-> Protocol.
After: Service <-> (CustomProto <-> Protocol), and the thing in parentheses is ProtocolBehaviour.

This made it possible to move start_service in the constructor of NetworkWorker, which I did as well.

This unlocks the next step, which is to split Protocol in multiple NetworkBehaviours.

Because #2500 was "rejected", I've had to use some hacks for granting access to the gossiping and specialization. It is unclear how I will clean that up, but I'll try to find a solution.

@tomaka tomaka added the A0-please_review Pull request needs code review. label Jun 21, 2019
@tomaka tomaka requested a review from twittner June 21, 2019 09:47

@twittner twittner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (besides some weird formatting).

Comment thread core/network/src/protocol_behaviour.rs Outdated
}

impl<B: BlockT, S: NetworkSpecialization<B>, H: ExHashT> ProtocolBehaviour<B, S, H> {
/// Builds a new `ProtocolsBehaviour`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Builds a new `ProtocolsBehaviour`.
/// Builds a new `ProtocolBehaviour`.

Comment thread core/network/src/protocol_behaviour.rs Outdated
}

/// Returns the list of all the peers we have an open channel to.
pub fn open_peers<'a>(&'a self) -> impl Iterator<Item = &'a PeerId> + 'a {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn open_peers<'a>(&'a self) -> impl Iterator<Item = &'a PeerId> + 'a {
pub fn open_peers(&self) -> impl Iterator<Item = &PeerId> {

Ok(Async::NotReady) => {}
Err(err) => void::unreachable(err),
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These block delimiters are not necessary.

@gavofyork gavofyork added A7-looksgoodcantmerge and removed A0-please_review Pull request needs code review. labels Jun 21, 2019

@bkchr bkchr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nitpicks^^

Comment thread core/network/src/protocol_behaviour.rs Outdated
Comment thread core/network/src/protocol_behaviour.rs Outdated
);
}

// TODO: needed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every TODO should be an issue in github.

Comment thread core/network/src/protocol_behaviour.rs Outdated
Comment thread core/network/src/protocol_behaviour.rs Outdated
Comment thread core/network/src/protocol_behaviour.rs Outdated
Comment thread core/network/src/protocol_behaviour.rs Outdated
Comment thread core/network/src/protocol_behaviour.rs Outdated
Comment thread core/network/src/protocol_behaviour.rs Outdated
Comment thread core/network/src/protocol_behaviour.rs Outdated
Comment thread core/network/src/protocol_behaviour.rs Outdated
tomaka and others added 2 commits June 24, 2019 12:12
Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>
@tomaka

tomaka commented Jun 24, 2019

Copy link
Copy Markdown
Contributor Author

Fixed the nitpicks!

@gavofyork gavofyork merged commit 58f4581 into paritytech:master Jun 24, 2019
@tomaka tomaka deleted the protocol-behaviour branch June 24, 2019 11:24
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Jul 10, 2019
* Add ProtocolBehaviour

* Fix tests

* Line widths

* Address concerns

* Apply suggestions from code review

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

* Remove TODO
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants