Add Groestlcoin support

prusnak commented 5 years ago

@gruve-p It seems your changes do not pass the test suite. Can you work on these failures?

gruve-p commented 5 years ago

@prusnak Tests are not yet updated because we want a review if these changes are acceptable to get merged.

gruve-p commented 5 years ago

Implementation has been tested as working on Zelcore: https://github.com/TheTrunk/bitgo-utxo-lib/pull/2

cooncesean commented 5 years ago

@gruve-p thank you for contributing to this open source library. I'm guessing that none of the maintainers from BitGo have taken a look at this yet because:

They're currently swamped with their own work.
BitGo doesn't currently support Groestlcoin and doesn't plan to in the near future.
We haven't yet figured out how to respond to pull requests like this; ones that add bloat (I know you wouldn't consider it bloat, but from our perspective, it adds weight + complexity to a codebase in terms of new functionality that will likely only be used by a small pct of developers.

.......

So, what are our options here....

One approach would be to use your fork and keep up to date with upstream changes.
Another approach would be to think about how we can change the architecture of this codebase such that it can be used as a very light-weight core library and specific coins protocols can dynamically hook it into to implement their own nuanced behavior. ie: instead of adding BTC/BCH/Groestlcoin logic to this lib, this lib performs very bare bone/core logic and other libraries can hook into that logic to perform extended + custom functionality in their own repos.

In it's current form, I'm doubtful this will get merged :/

gruve-p commented 5 years ago

@cooncesean thank you for your honest and transparant answer. We had low expectations that this would be merged so no heart feelings there.

We think we will go with option 1 as Trezor is planning to use their own fork and keep up to date with upstream.

Should I keep this PR open or close it?

FWIW This PR has been tested by @TheTrunk on https://github.com/TheTrunk/bitgo-utxo-lib and is being used for Zelcore wallet: https://zeltrez.io/ It is rebased from https://github.com/wlc-/bitcoinjs-lib mady by @wlc- and is being used for CoinID wallet: https://coinid.org/

cooncesean commented 5 years ago

Should I keep this PR open or close it?

Your call. Leaving it open would be a nagging reminder for us to re-think the architecture. Closing it would clean up a request that likely won't be merged.

Thanks again for the work 👍 -- if you have thoughts for changing the architecture to be able to support future requests like this, by all means open up a proposal.

gruve-p commented 4 years ago

@cooncesean We have changed the methodology and fixed some unit tests. With the new methodology other coins that use different algo for base58 (For example Decred uses blake2b for base58) can follow our way to get supported without making the architecture too complex. What do you think of the new methodology as any feedback would be welcome.

gruve-p commented 4 years ago

@prusnak We will fix the test suite shortly so it passes.

OttoAllmendinger commented 4 years ago

we are restructuring bitgo-utxo-lib a bit to be more compatible with upstream and be more modular

gruve-p commented 4 years ago

Should we resubmit a new PR after the restructuring? If so when should we resubmit?

OttoAllmendinger commented 4 years ago

I will use https://github.com/BitGo/bitgo-utxo-lib/issues/45 to track the progress

BitGo / bitgo-utxo-lib

Add Groestlcoin support #22