-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: use the lowest latency peer for protocols #1540
Conversation
size-limit report 📦
|
8858e45
to
42eb3cb
Compare
b132ffe
to
08f36ff
Compare
@@ -340,7 +340,11 @@ export class ConnectionManager | |||
void (async () => { | |||
const peerId = evt.detail; | |||
|
|||
this.keepAliveManager.start(peerId, this.libp2p.services.ping); | |||
this.keepAliveManager.start( | |||
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.
nit: we can simplify and pass ‘libp2p’ directly
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 suggest passing ping
and peerStore
individually to KeepAliveManager
instead of the entire libp2p
object because:
- by passing only what's needed, it's clear what
KeepAliveManager
depends on. This makes our code more readable and maintainable. - this approach reduces the dependency on the entire libp2p structure. If libp2p evolves, it minimizes the ripple effect on KeepAliveManager.
- it follows best practices like the Single Responsibility and Interface Segregation Principles
Let me know your thoughts!
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.
SOLID is not applied here because it is more about how entity/object should be designed and not what it should be provided with. You might need thousand dependencies to do single thing and it is up to devs how easier/cleaner they should be provided.
My rule of thumb for dependencies - it's easier to grow the number of entities than methods.
And again, it is a nit, so feel free to keep the way you think reasonable.
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.
Understanding it's a nit, glad we're discussing to be on similar pages around these philosophies if we're maintaining big codebases :)
I understand that SOLID primarily focuses on the design and structure of classes. However, the Dependency Inversion Principle directly addresses how dependencies should be managed -- I interpret it as our classes are decoupled and that we're injecting only the necessary dependencies for this use case
Perhaps you have a different interpretation?
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.
No, it's exactly about necessary dependencies, and the necessary dependency here is libp2p
and not its methods.
Problem
During selection of peers to be used for light protocols, the selection currently happens randomly. This is not ideal, as we might potentially be communicating with a higher latency node, while an option to use a faster node might exist.
Ref: #1465 #1497
Solution