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

Add network list tests for ECPair/HDNode #550

Merged
merged 3 commits into from
Feb 25, 2016
Merged

Add network list tests for ECPair/HDNode #550

merged 3 commits into from
Feb 25, 2016

Conversation

dcousens
Copy link
Contributor

This actually fixes a bug bip32 is undefined when a networks list was passed and no valid version was found in HDNode.

@dcousens dcousens self-assigned this Feb 25, 2016
@dcousens dcousens added this to the 2.3.0 milestone Feb 25, 2016
@dcousens
Copy link
Contributor Author

@fanatid please review 👍.

Thoughts on aae5db6? On the 1 hand, easier to verify the tests are actually following the path they should be (not ambiguous with the other other case).
On the other hand, its two cases a user has to be aware of, despite the fact only 1 should occur depending on whether they pass in a list or singular network parameter.

@fanatid
Copy link
Member

fanatid commented Feb 25, 2016

@dcousens I think throwing 2 different errors is OK, first says that not one of network from networks don't match, second says that version for given|selected network is invalid.
LGTM

@@ -72,7 +74,7 @@ HDNode.fromBase58 = function (string, networks) {
}

if (version !== network.bip32.private &&
version !== network.bip32.public) throw new Error('Invalid network')
version !== network.bip32.public) throw new Error('Invalid network version')
Copy link
Member

Choose a reason for hiding this comment

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

just curious, you don't use align in if blocks?

if (version !== network.bip32.private &&
    version !== network.bip32.public) throw new Error('Invalid network version')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, alignment isn't that big a deal IMHO.
Its also very hard to enforce semantically so I don't bother.

dcousens added a commit that referenced this pull request Feb 25, 2016
Add network list tests for ECPair/HDNode
@dcousens dcousens merged commit 7d2b2de into master Feb 25, 2016
@dcousens dcousens deleted the testnet branch February 25, 2016 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants