Skip to content

Conversation

@dcousens
Copy link
Contributor

This pull request updates HDWallet to use Node Buffers internally.
It is dependent on #133.

In short, bc05828 adds Buffer input parameter support for ECKey and ECPubKey, necessary to avoid jumping through compatibility hoops in HDWallet itself.

814aa9e does most of the heavy lifting, with some small changes to the tests in test/hdwallet.js.
Some key points for discussion are the changes in how hd.parentFingerprint is stored internally. I opted for the Number representation because as per BIP32 the fingerprint is guaranteed to be 32 bits; which fits well within Numbers integer precision.
This made some of the Buffer code nicer to deal with, but is not absolutely necessary.

Something of interest was that despite the previous documentation, all data used in the serialization format was in MSB byte order (that is, Big Endian) as per the test vectors in test/hdwallet.js.
It is not clear if this is intentional, and the serialization specification does not mention it at all.
If the test vectors were adapted from this implementation, this should be fixed as soon as possible, because it may derive completely different/wrong private keys.

Lastly, ddea4b1 takes some of the implicit mod math currently hidden in the ECKey constructor and puts it in the open as per the comments and specification.
This adheres to changes that will be made to the ECKey constructors soon, and removes unnecessary calculations and error checking that are not pertinent to what is needed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, we should do something better here.

kyledrake added a commit that referenced this pull request Apr 16, 2014
HDWallet now uses Buffers internally
@kyledrake kyledrake merged commit aaaf66f into bitcoinjs:master Apr 16, 2014
@dcousens dcousens deleted the hdwalletbuf branch April 16, 2014 21:25
Copy link
Contributor

Choose a reason for hiding this comment

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

Grrr the name netstr... you mean networkString?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, bit late now but I'll make a small amendment.

edit: 505e33c

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