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
fix: check status and version to find peers #30
Conversation
Codecov Report
@@ Coverage Diff @@
## master #30 +/- ##
==========================================
+ Coverage 99.54% 99.55% +<.01%
==========================================
Files 23 23
Lines 221 224 +3
Branches 19 20 +1
==========================================
+ Hits 220 223 +3
Misses 1 1
Continue to review full report at Codecov.
|
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've done some simple tests and I didn't have any problem.
@@ -19,6 +20,10 @@ const mergePeerArrays = (oldArray, newArray) => { | |||
return peers | |||
} | |||
|
|||
const getPeerVersion = (peer) => { | |||
return !peer.version || /^1\./.test(peer.version) ? 1 : 2 |
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'd use something like:
return (peer.version && peer.version.startsWith('1.')) ? 1 : 2
@@ -69,7 +71,7 @@ module.exports = class ApiClient { | |||
if (responsePeers.length) { | |||
// Ignore local and unavailable peers |
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.
// Ignore local and unavailable peers, and those with wrong version
@@ -69,7 +71,7 @@ module.exports = class ApiClient { | |||
if (responsePeers.length) { | |||
// Ignore local and unavailable peers | |||
responsePeers = responsePeers.filter(peer => { | |||
return selfIps.indexOf(peer.ip) === -1 && peer.status === versionStatus[version] | |||
return selfIps.indexOf(peer.ip) === -1 && versionStatus.includes(peer.status) && getPeerVersion(peer) === version |
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'd use includes
on both checks:
return !selfIps.includes(peer.ip) && versionStatus.includes(peer.status) && getPeerVersion(peer) === version
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.
@alexbarnsley Approved in case you want to release it today without my suggestions
Can do the refactoring at a later time, thanks 👌 |
This PR includes a way to deal wtih ArkEcosystem/core#2026