-
Notifications
You must be signed in to change notification settings - Fork 458
Conversation
…peerHeaders are updated
…whenever the system info changes
…he current peer system
Integrate @liskhq/lisk-p2p module in chain module - Closes #2874
…eracting with peers 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.
LGTM only left a minor comment about a possible library change for random strings.
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.
LGTM
@@ -101,7 +103,7 @@ describe('Integration tests for P2P library', () => { | |||
// Set a different discoveryInterval for each node; that way they don't keep trying to discover each other at the same time. | |||
discoveryInterval: DISCOVERY_INTERVAL + index * 11, | |||
nodeInfo: { | |||
wsPort: NETWORK_START_PORT + index, | |||
wsPort: nodePort, |
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.
Possibly rename the variable to wsPort
for clarity (at line 94)?
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.
And other places as well, just minor that can be considered.
…skHQ/lisk-sdk into feature/create_network_module
a7a21e4
… feature/create_network_module
… feature/create_network_module
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.
Just a minor comment related to the naming of action and events
@@ -194,6 +174,25 @@ module.exports = class Chain { | |||
|
|||
self.logger.info('Modules ready and launched'); | |||
|
|||
this.channel.subscribe( | |||
'network:subscribe', |
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.
This name still doesn't seem quite right. We should raise another issue to adjust the naming.
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.
Actually we can use this existing issue: #3263
Description
Merging feature/create_network_module
In the new network module, internally it uses lisk-p2p library.
Any of the interaction directly to the peer is removed from chain, and it's handled within the network module.