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

Expose underlying implementation #1567

Closed
wemeetagain opened this issue Jan 20, 2023 · 5 comments · Fixed by #1586
Closed

Expose underlying implementation #1567

wemeetagain opened this issue Jan 20, 2023 · 5 comments · Fixed by #1586
Assignees
Labels
need/triage Needs initial labeling and prioritization

Comments

@wemeetagain
Copy link
Member

In Lodestar, we're forced to do make some little wrapper functions to get the types of the default implementations. (And we're just lucky that the types are exported, despite your intention 😂) These default implementations are often necessary to fine-tune or inspect data not available in the generic interface types.

Maybe there's a better way to do this?

See:
https://github.com/ChainSafe/lodestar/blob/unstable/packages/beacon-node/src/network/util.ts#L71-L90

@wemeetagain wemeetagain added the need/triage Needs initial labeling and prioritization label Jan 20, 2023
@wemeetagain
Copy link
Member Author

I'd almost say there's no need to have an interface-connection-manager, interface-registrar, etc if there's not even a way to swap in alternative implementations.

@p-shahi p-shahi moved this to 🥞Weekly Candidates/Discuss🎙 in js-libp2p Jan 24, 2023
@maschad
Copy link
Member

maschad commented Jan 26, 2023

I'd almost say there's no need to have an interface-connection-manager, interface-registrar, etc if there's not even a way to swap in alternative implementations.

Agreed. My understanding is that the Connection Manager and Registrar in particular are apart of the core libp2p stack and are intended to have concrete implementations according to the spec. Looking at Go's Base connection manager , it seems to have been implemented that way (as a struct as opposed to an interface).

I would say should there arise a need to have added functionality then a consumer could extend the ConnectionManager or Registrar classes.

@maschad
Copy link
Member

maschad commented Feb 2, 2023

#1574 should solve some of the issues we where facing in lodestar, but for this particular method which gets all connections and their associated peer ids , I don't see why we couldn't modify the connection manager interface to expose that connections map.

@achingbrain
Copy link
Member

To get all connections, call libp2p.getConnections(). Each connection has a remotePeer property that is the PeerId of the remote node.

@maschad
Copy link
Member

maschad commented Feb 8, 2023

To get all connections, call libp2p.getConnections(). Each connection has a remotePeer property that is the PeerId of the remote node.

This would add additional runtime logic which in the case of lodestar negatively impacts performance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/triage Needs initial labeling and prioritization
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants