cryptocoinjs / hdkey

JavaScript component for Bitcoin hierarchical deterministic keys (BIP32)
MIT License
201 stars 74 forks source link

fix: derive will error when a path does not start with "m/" #20

Closed mattgstevens closed 6 years ago

mattgstevens commented 6 years ago

Was working through some tests while using this library and noticed that the assert statement for the first section of path was incorrect.

This lead to a closer look and I believe that the only valid first character is m.

Noticed another PR that would have cleared this up but it seems stuck / forgotten https://github.com/cryptocoinjs/hdkey/pull/6.

What do you think @jprichardson @axic ?

dcousens commented 6 years ago

NACK, m is only valid for a master key?

mattgstevens commented 6 years ago

@dcousens ok have updated.

curious to read more about the usage of m', M and M'

when reading through the BIP32 and related I don't see a reference to this.

dcousens commented 6 years ago

@mattgstevens never seen it either, but as @RyanZim pointed out, it could break someones code otherwise.

mattgstevens commented 6 years ago

@dcousens understood.

if / when this library goes to the next major version this would be a good change to include IMO.

mattgstevens commented 6 years ago

@dcousens is there any further changes you would like? otherwise I think this is ready for a merge.

RyanZim commented 6 years ago

@dcousens I'm of the opinion that we should make the change of requiring m only, and just release a major release.

dcousens commented 6 years ago

@RyanZim go for it :+1: - but what about the 12,970 weekly downloads that might appreciate this patch?

RyanZim commented 6 years ago

Well, they can upgrade if they want, otherwise semver to the rescue. While we're at it, could drop old Node versions, currently supports 0.10+

RyanZim commented 6 years ago

Just realized this is in 0.x; should we release v1 since it's basically stable?

jprichardson commented 6 years ago

Yep, good idea to release a 1.0.