search

komputing / KEthereum

Kotlin library for Ethereum
MIT License
350 stars 73 forks source link

Add testnet version bytes to extended keys, close #67 #68

Closed mahdi13 closed 5 years ago

ligi commented 5 years ago

thanks! LGTM - kindly asking @mirceanis to also have a look as he was doing the original BIP32 PR

mirceanis commented 5 years ago

I can take a look tomorrow

ligi commented 5 years ago

That would be much appreciated!

mirceanis commented 5 years ago

Code looks ok, there is a breaking change for people who are directly instantiating ExtendedKey or using equality tests on preexisting ones and that should be mentioned in release notes when released.

Also, what is the use of this? It would be nice to see a description on the PR.

The reason that testnet values weren't added from the beginning is that the "testnet" is a bitcoin one and had no apparent relation to ethereum. It's also possible I missed it back then. Is there a use-case now, @mahdi13 ?

ligi commented 5 years ago

Thanks for the review. I think it is good to also support bitcoin and respect the standard. Also think to extract the code to a new repo into Komputing like KBip32 to decouple from Ethereum

mirceanis commented 5 years ago

I think it is good to also support bitcoin and respect the standard.

right, my question is out of curiosity, not a critique of the PR.

Also think to extract the code to a new repo into Komputing like KBip32 to decouple from Ethereum

sounds like a very good idea. Based on past experience, this should also be correlated with a release in KEthereum where the bip32 module is removed, to avoid getting a "duplicated classes" error when compiling for android.

mahdi13 commented 5 years ago

@mirceanis I agree, I don't see any relationship between bitcoin's testnet and ethereum. I only suggested it because it was part of the official bip-0032 proposal. Thanks for your great toolchain, I've been using this repository in many of my organization's project. Not only about ethereum, but I've also chosen this library for implementing bitcoin, ripple, and other cryptocurrencies tools. I see great potential on this code to be expanded to cover more cryptocurrencies.