-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #258 +/- ##
===========================================
+ Coverage 75.67% 75.79% +0.11%
===========================================
Files 103 105 +2
Lines 4991 5073 +82
===========================================
+ Hits 3777 3845 +68
- Misses 1214 1228 +14
Continue to review full report at Codecov.
|
src/worker/EnigmaNode.js
Outdated
connectToBootstrap(peerInfo) { | ||
connectToBootstrap(peerInfo, callback) { | ||
this.dial(peerInfo, (connectionErr, connection) => { | ||
if (connectionErr) { | ||
this._logger.error(`Failed connecting to a bootstrap node= ${connectionErr}`); | ||
return false; | ||
} else { | ||
this.notify(peerInfo); | ||
return true; | ||
} | ||
callback(connectionErr); | ||
}); | ||
} |
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.
Please use promisify:
const { promisify } = require("util");
// ...
connectToBootstrap(peerInfo) {
const dial = promisify(dial).bind(this);
try {
const connection = await dial(peerInfo); // Notice that connection is unused
} catch (connectionErr) {
this._logger.error(`Failed connecting to a bootstrap node= ${connectionErr}`);
return false;
}
this.notify(peerInfo);
return true;
}
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.
Also Notice that dial
can be implemented with promisify
on dialProtocol
and node.dialProtocol
.
This way you won't have to do const dial = promisify(dial).bind(this);
.
|
||
// core | ||
try { | ||
let regParams = await this.asyncGetRegistrationParams(); |
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.
let -> const
// ethereum | ||
if (this.hasEthereum()) { | ||
try { | ||
let eth = await this.ethereum().healthCheck(); |
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.
let -> const
healthCheckResult.core.status = healthCheckResult.core.registrationParams.signKey != null; | ||
if (this._controller.hasEthereum()) { | ||
try { | ||
let eth = await this._controller.ethereum().healthCheck(); |
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.
const
this._controller.engNode().connectToBootstrap(otherPeer, err => { | ||
let success = false; | ||
if (!err) { | ||
success = true; | ||
} | ||
this._controller.logger().debug(`connection to bootstrap succeeded=${success}`); | ||
}); |
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.
Please use promisify
with try/catch
.
@lenak25 You can ignore the "let -> const" comments, but please use promisify, it will greatly improve code readability. 🙏 |
@assafmo , code review comments were integrated.. please review again :) |
Also "Merging is blocked" due to "The base branch requires all commits to be signed." 🤷♂️ Try squashing this entire PR or resign every commit since branching out: git rebase --exec 'git commit --amend --no-edit -n -S' -i develop |
No description provided.