Skip to content

Conversation

@dcousens
Copy link
Contributor

@dcousens dcousens commented Jun 4, 2014

This pull request changes the base58check API, removing the version parameter completely in favour of simply performing the checksum comparison and encoding/decoding the Base58 only.

This has been chosen in favour of some formats such as the export format in BIP32 using 4-byte version constants instead of the traditional 1-byte. This means there is less code duplication in HDNode and overall the remainder of the code stays relatively similar.
It also makes testing remarkably simpler for base58check.

Finally, an additional change that enforces consistent use of the network objects throughout the code base has been applied.

ECPubKey.getAddress() and ECKey.toWIF() now take a network object. This actually simplified the Message signing code a little bit as well.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling 28dc390 on dcousens:base58rework into 63e6cf9 on bitcoinjs:master.

@dcousens
Copy link
Contributor Author

dcousens commented Jun 4, 2014

Travis exploding again on helloblock. Will hopefully be resolved soon.

@weilu
Copy link
Contributor

weilu commented Jun 4, 2014

@dcousens it's not helloblock this time. https://github.com/dcousens/bitcoinjs-lib/blob/base58rework/test/integration/p2sh.js#L41 instead of getAddress(networks.testnet.pubKeyHash) it should be getAddress(networks.testnet)

@dcousens
Copy link
Contributor Author

dcousens commented Jun 4, 2014

Fixed and good to go.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) when pulling 4c6385e on dcousens:base58rework into 2c5b9cf on bitcoinjs:master.

@kyledrake
Copy link
Contributor

+1

kyledrake added a commit that referenced this pull request Jun 4, 2014
@kyledrake kyledrake merged commit 9b5dfbd into bitcoinjs:master Jun 4, 2014
@dcousens dcousens deleted the base58rework branch June 5, 2014 02:57
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.

4 participants