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

Do not expose new or tried peers from P2P library - Close #4044 #4125

Merged
merged 9 commits into from
Aug 26, 2019
Merged

Do not expose new or tried peers from P2P library - Close #4044 #4125

merged 9 commits into from
Aug 26, 2019

Conversation

diego-G
Copy link

@diego-G diego-G commented Aug 22, 2019

Review checklist

@diego-G diego-G self-assigned this Aug 22, 2019
@shuse2 shuse2 requested review from mitsuaki-u and removed request for shuse2 August 22, 2019 13:38
jondubois
jondubois previously requested changes Aug 22, 2019
View reviewed changes
elements/lisk-p2p/src/peer_pool.ts Outdated Show resolved Hide resolved
ishantiw
ishantiw suggested changes Aug 23, 2019
View reviewed changes
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.

I feel that we can get rid of checking in new/tried peers list instead of that we can simply use getAllPeers method of PeerBook and make the expectation of the test simpler

elements/lisk-p2p/test/integration/p2p.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/test/integration/p2p.ts Show resolved Hide resolved
@diego-G diego-G requested a review from ishantiw August 23, 2019 11:34
ishantiw
ishantiw approved these changes Aug 23, 2019
View reviewed changes
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.

Looks good

mitsuaki-u
mitsuaki-u approved these changes Aug 23, 2019
View reviewed changes
Copy link
Contributor

@mitsuaki-u mitsuaki-u left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just some very minor comments. Nice job cleaning up tests!

elements/lisk-p2p/test/integration/p2p.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/test/integration/p2p.ts Outdated Show resolved Hide resolved
@diego-G diego-G requested a review from shuse2 August 26, 2019 09:50
@shuse2 shuse2 merged commit 1685b78 into hotfix/2.3.1 Aug 26, 2019
@shuse2 shuse2 deleted the 4044-do_not_expose_new_tried_peers branch August 26, 2019 10:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mitsuaki-u mitsuaki-u mitsuaki-u approved these changes

@ishantiw ishantiw ishantiw approved these changes

@shuse2 shuse2 shuse2 approved these changes

@jondubois jondubois Awaiting requested review from jondubois

Assignees

@diego-G diego-G

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@diego-G @jondubois @shuse2 @ishantiw @mitsuaki-u