-
Notifications
You must be signed in to change notification settings - Fork 43
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: discover nodes from a specific peer #920
Conversation
lib/service/Service.ts
Outdated
@@ -374,6 +377,14 @@ class Service extends EventEmitter { | |||
return this.orderBook.removePair(pairId); | |||
} | |||
|
|||
/** Removes a trading pair. */ |
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.
Comment doesn't match up with the method.
proto/xudrpc.proto
Outdated
string peer_pub_key = 1 [json_name = "peer_pub_key"]; | ||
} | ||
message DiscoverNodesResponse { | ||
repeated NodeConnectionInfo nodes = 1; |
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.
Conceptually I'm wondering whether we want to actually return all the connection info we get from the peer. I am not sure whether that would be useful.
I'm thinking this call would be used to tell xud to find new peers, but the caller can't actually act on the nodepubkeys and all the addresses we discover, that's all handled internally. Instead we could just return the number of nodes that we discovered, or maybe just a list of pub keys (which can then be used to call getnodeinfo if we want more details). Thoughts @kilrau?
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 think "Discovered X new nodes" as response would be good. If the delta of "new nodes" is hard to calculate just go with "Discovered X nodes".
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.
The first option would require some infra work, so I went for the latter, since the use-case is quite limited.
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, just needs a rebase to resolve conflicts. Thanks.
Discover nodes from a specific peer, apply new connections, and return the list to the client.