Skip to content
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

Substitute lodash calls in bitcore-lib #3350

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

picnicbear
Copy link
Contributor

No description provided.

Copy link
Collaborator

@kajoseph kajoseph left a comment

Choose a reason for hiding this comment

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

Nice job! I left a few suggestions, mostly for relatively minor quality of life and consistency/cleanliness changes. Feel free to ping me to discuss anything!

@@ -69,10 +68,10 @@ BlockHeader._fromObject = function _fromObject(data) {
$.checkArgument(data, 'data is required');
var prevHash = data.prevHash;
var merkleRoot = data.merkleRoot;
if (_.isString(data.prevHash)) {
if (typeof data.prevHash === 'string') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is false if data.prevHash instanceof String, whereas _.isString(new String('')) == true

return new BN(str, base);
};

BN.fromBuffer = function(buf, opts) {
if (typeof opts !== 'undefined' && opts.endian === 'little') {
if (opts !== undefined && opts.endian === 'little') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably make opts !== undefined be opts != null to avoid unnecessary confusion for a user passing in a null variable. There should be no difference between

BN.fromBuffer(buf);

and

var opts = null;
BN.fromBuffer(buf, opts);

Same on line 89

Maybe consider changing all checks for undefined to checks for null as well (i.e. === undefined --> == null)

@@ -74,7 +60,7 @@ MultiSigInput.prototype.getSignatures = function(transaction, privateKey, index,

var self = this;
var results = [];
_.each(this.publicKeys, function(publicKey) {
this.publicKeys.forEach(function(publicKey) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why you did .forEach instead a a for..of loop here?

@@ -122,7 +116,7 @@ MultiSigScriptHashInput.prototype.getSignatures = function(transaction, privateK

var self = this;
var results = [];
_.each(this.publicKeys, function(publicKey) {
this.publicKeys.forEach(function(publicKey) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

.forEach instead of for..of?

@@ -16,13 +15,13 @@ function Output(args) {
if (!(this instanceof Output)) {
return new Output(args);
}
if (_.isObject(args)) {
if (typeof args === 'object') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

need to change all of these typeof ... === 'object' to use JSUtil.isObject

if (Array.isArray(privateKey)) {
privateKey.forEach(privateKey => this.sign(privateKey, sigtype, signingMethod));
} else {
this.getSignatures(privateKey, sigtype, signingMethod).forEach((signature) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

forEach here and above

@@ -1222,40 +1207,34 @@ Transaction.prototype.applySignature = function(signature, signingMethod) {
};

Transaction.prototype.isFullySigned = function() {
_.each(this.inputs, function(input) {
this.inputs.forEach(function(input) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

forEach

* @description sort an array in place based on toString values
* @param {*[]} array
*/
sort: array => array.sort((a, b) => a.toString() > b.toString() ? 1 : a.toString() < b.toString() ? -1 : 0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would probably be helpful to at least have a comment saying that this is an ascending sort. Might I suggest we have sortAsc and sortDesc functions? Just so we can avoid ambiguity.

* @param {*} obj
* @returns {Boolean}
*/
isObject: obj => typeof obj === 'object' && !!obj,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good. I think we should add similar functions for isNumber and isString and replace all the typeof <whatev>'s with them. What do you think? There are inconsistencies with checking typeof <xyz> === 'number' and typeof <xyz> === 'number' && !isNaN(<xyz>), plus the latter is a bit verbose.

Also, Number and String class instances would fail the typeof test, but would pass the lodash equivalents.

transaction.outputs.should.include(out1);
transaction.outputs.should.include(out2);
transaction.outputs.should.include(out3);
transaction.outputs.should.include(out4);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't test for randomness anymore

@kajoseph
Copy link
Collaborator

The problem referenced in #3358 still applies to the .every method on the Array prototype. We should fix it in here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants