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

Split NetworkEventListenerProvider into two #12861

Closed
mattsse opened this issue Nov 26, 2024 · 3 comments · Fixed by #12972
Closed

Split NetworkEventListenerProvider into two #12861

mattsse opened this issue Nov 26, 2024 · 3 comments · Fixed by #12972
Assignees
Labels
A-networking Related to networking in general C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started

Comments

@mattsse
Copy link
Collaborator

mattsse commented Nov 26, 2024

Describe the feature

currently

pub trait NetworkEventListenerProvider: Send + Sync {

returns a non generic:

pub enum NetworkEvent<R = PeerRequest> {
/// Closed the peer session.
SessionClosed {
/// The identifier of the peer to which a session was closed.
peer_id: PeerId,
/// Why the disconnect was triggered
reason: Option<DisconnectReason>,
},
/// Established a new session with the given peer.
SessionEstablished {
/// The identifier of the peer to which a session was established.
peer_id: PeerId,
/// The remote addr of the peer to which a session was established.
remote_addr: SocketAddr,
/// The client version of the peer to which a session was established.
client_version: Arc<str>,
/// Capabilities the peer announced
capabilities: Arc<Capabilities>,
/// A request channel to the session task.
messages: PeerRequestSender<R>,
/// The status of the peer to which a session was established.
status: Arc<Status>,
/// negotiated eth version of the session
version: EthVersion,
},
/// Event emitted when a new peer is added
PeerAdded(PeerId),
/// Event emitted when a new peer is removed
PeerRemoved(PeerId),
}

but we need this to be generic, but only in a few places.
we can make this more convenient if we convert these fields (minus messages) into a standalone type SessionInfo:

SessionEstablished {
/// The identifier of the peer to which a session was established.
peer_id: PeerId,
/// The remote addr of the peer to which a session was established.
remote_addr: SocketAddr,
/// The client version of the peer to which a session was established.
client_version: Arc<str>,
/// Capabilities the peer announced
capabilities: Arc<Capabilities>,
/// A request channel to the session task.
messages: PeerRequestSender<R>,
/// The status of the peer to which a session was established.
status: Arc<Status>,
/// negotiated eth version of the session
version: EthVersion,
},

and then introduce a new trait NetworkPeersEvents that only returns the non generic events while NetworkEventListenerProvider becomes NetworkEventListenerProvider: NetworkPeersEvents

TODO

    1. introduce SessionInfo
    1. introduce NetworkPeersEvents with stream and event type

Additional context

No response

@mattsse mattsse added C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Nov 26, 2024
@mattsse mattsse added D-good-first-issue Nice and easy! A great choice to get started A-networking Related to networking in general and removed S-needs-triage This issue needs to be labelled labels Nov 26, 2024
@lean-apple
Copy link
Contributor

Hello, can I work on it ?

@mattsse
Copy link
Collaborator Author

mattsse commented Nov 26, 2024

assigned

@mimisavage
Copy link

Could I take over this issue?

@github-project-automation github-project-automation bot moved this from Todo to Done in Reth Tracker Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants