add multibyte public version support (zcash support)

D-Nice commented 6 years ago

adds support for coins, such as zcash, that use two or more bytes for versioning of public addresses. This was done by converting all versions passed as Buffers to coinstring, which the library insists on for multibyte support.
added zcash into test cases, required adding a conditional whereby versions are passed into fromWif in the case the coin doesn't follow the previous convention of priv - pub = 0x80. Can be easily done by also leveraging coininfo.

Should result in all coins currently listed on coininfo being supported, except for Decred, which uses multibytes for the private key. Had an implementation for that too, but in general it seems that private keys are not intended to be supported with Decred for whatever reason.

D-Nice commented 6 years ago

pinging @jprichardson @fanatid

fanatid commented 6 years ago

Problem with multibyte version that we do not know, will big endian used or little endian...

D-Nice commented 6 years ago

Yes, this is not something I've looked at, I've presumed big-endian as that's what Zcash appears to use (EDIT: or at least worked with). Do you know of any that may use LE? However, can the library not just expect all to be BE, and even if LE, for it to be pre-converted before being passed, into an integer that in BE form will correspond correctly?

D-Nice commented 6 years ago

As a note, I've had a look at various coins, and the only one other than zcash copypastas I've seen using multibytes is decred, which I've been able to find no real deep documentation/protocol info on, and has an additional complication of having the private version be multibyte as well (which I do have working but didn't find useful to push).

This is a note on the endianness specified in the Zcash Protocol Specification: All integers in Zcash-specific encodings are unsigned, have a fixed bit length, and are encoded in little-endian byte order unless otherwise specified. However, the version is apparently specified in the paper as being 2 distinct 8-bit values (from my understanding), rather than a single 16-bit uint, in which case the endianness does not matter. Meaning the current bufferizeVersion method is the way to go, as I'd expect the hex bytes as specified to be passed, and it works seamlessly with the current definitions in coininfo.

If you can recommend what would be needed to get this merged, let me know. My other option is just using a forked version, as this is a dependency for one of my own projects, but this being upstream is preferable and beneficial to others, if you guys deem it sufficient.

D-Nice commented 6 years ago

New push.

Fixes aforementioned length issue.

Handles LE or leading zero instances by requiring a hex string sequence instead of passing a uint for the version.

Please let me know if this is an acceptable change, and/or of other issues.

fanatid commented 6 years ago

LGTM ping @jprichardson

RyanZim commented 6 years ago

This is semver-minor, right?

fanatid commented 6 years ago

Yes, I think this is semver-minor.

RyanZim commented 6 years ago

Didn't realize this package was so out of date; published as 3.0.0 along with dropping old Node version support.

cryptocoinjs / coinkey

add multibyte public version support (zcash support) #9