Change selectPeers function to only handle detailed PeerInfo objects; not live Peer objects Closes#1059 #1067
Conversation
| @@ -365,6 +365,10 @@ export class Peer extends EventEmitter { | |||
| inboundSocket.off(REMOTE_EVENT_MESSAGE, this._handleRawMessage); | |||
| } | |||
|
|
|||
| public static constructPeerIdFromPeerInfo(peerInfo: P2PPeerInfo): string { | |||
There was a problem hiding this comment.
maybe just export as a function?
There was a problem hiding this comment.
If we export as a function then we should do the same for the Peer.constructPeerId(address, port) which is now a static method.
Or we can do this in a separate PR because I also rely on the same static method right now for my next PR.
ishantiw
force-pushed
the
1059-change_selectpeers_to_handle_peerinfo
branch
from
eded4d5
to
ff555a0
Compare
packages/lisk-p2p/src/p2p.ts
Outdated
| ); | ||
| } | ||
| const response: P2PResponsePacket = await selectedPeer[0].request(packet); | ||
| const response = await this._peerPool.requestPeer(packet, this._nodeInfo); |
There was a problem hiding this comment.
We shouldn't pass this._nodeInfo as second argument. We should only support a single way of updating the nodeInfo; using applyNodeInfo.
Because the PeerPool now holds a reference to the latest _nodeInfo state internally, you can just use this. _nodeInfo from inside the PeerPool instead.
packages/lisk-p2p/src/p2p.ts
Outdated
| selectedPeers.forEach((peer: Peer) => { | ||
| peer.send(message); | ||
| }); | ||
| this._peerPool.sendToPeers(message, this._nodeInfo); |
| @@ -365,6 +365,10 @@ export class Peer extends EventEmitter { | |||
| inboundSocket.off(REMOTE_EVENT_MESSAGE, this._handleRawMessage); | |||
| } | |||
|
|
|||
| public static constructPeerIdFromPeerInfo(peerInfo: P2PPeerInfo): string { | |||
There was a problem hiding this comment.
If we export as a function then we should do the same for the Peer.constructPeerId(address, port) which is now a static method.
Or we can do this in a separate PR because I also rely on the same static method right now for my next PR.
packages/lisk-p2p/src/peer_pool.ts
Outdated
| nodeInfo: P2PNodeInfo, | ||
| ): Promise<P2PResponsePacket> { | ||
| const peerSelectionParams: PeerOptions = { | ||
| lastBlockHeight: nodeInfo.height, |
There was a problem hiding this comment.
As mentioned in previous comment, you can just use this._nodeInfo instead of getting the nodeInfo as a second argument to this function.
packages/lisk-p2p/src/peer_pool.ts
Outdated
| return response; | ||
| } | ||
|
|
||
| public sendToPeers(message: P2PMessagePacket, nodeInfo: P2PNodeInfo): void { |
There was a problem hiding this comment.
Same as previous comment about nodeInfo.
|
comments are addressed |
ishantiw
force-pushed
the
1059-change_selectpeers_to_handle_peerinfo
branch
from
2cb92e1
to
07dc5d0
Compare
Description
Currently, the
selectPeersfunction accepts and returns a list ofPeerobjects (ReadonlyArray<Peer>). We should make theselectPeersfunction inpeer_selection.tswork with detailed peer info objects instead of live Peer objects.Review checklist