Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Merge Feature/create network module #3465

Merged
merged 167 commits into from
Apr 29, 2019
Merged

Conversation

shuse2
Copy link
Collaborator

@shuse2 shuse2 commented Apr 28, 2019

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.

jondubois and others added 30 commits March 1, 2019 10:47
Integrate @liskhq/lisk-p2p module in chain module - Closes #2874
pablitovicente
pablitovicente previously approved these changes Apr 29, 2019
Copy link
Contributor

@pablitovicente pablitovicente left a 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.

michielmulders
michielmulders previously approved these changes Apr 29, 2019
Copy link
Contributor

@michielmulders michielmulders left a 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,
Copy link
Contributor

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)?

Copy link
Contributor

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.

@shuse2 shuse2 dismissed stale reviews from michielmulders and pablitovicente via a7a21e4 April 29, 2019 10:55
ishantiw
ishantiw previously approved these changes Apr 29, 2019
pablitovicente
pablitovicente previously approved these changes Apr 29, 2019
@shuse2 shuse2 dismissed stale reviews from pablitovicente and ishantiw via f337657 April 29, 2019 12:59
nazarhussain
nazarhussain previously approved these changes Apr 29, 2019
Copy link
Contributor

@ishantiw ishantiw left a 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',
Copy link
Contributor

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.

Copy link
Contributor

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

@shuse2 shuse2 merged commit 009f1d4 into development Apr 29, 2019
@shuse2 shuse2 deleted the feature/create_network_module branch April 29, 2019 16:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants