Skip to content
This repository has been archived by the owner on Jan 25, 2024. It is now read-only.

Add Groestlcoin support #22

Closed
wants to merge 13 commits into from
Closed

Add Groestlcoin support #22

wants to merge 13 commits into from

Conversation

gruve-p
Copy link

@gruve-p gruve-p commented Jan 19, 2019

No description provided.

@prusnak
Copy link

prusnak commented Jan 20, 2019

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

@gruve-p
Copy link
Author

gruve-p commented Jan 20, 2019

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

@gruve-p
Copy link
Author

gruve-p commented Feb 16, 2019

Implementation has been tested as working on Zelcore:
TheTrunk#2

@cooncesean
Copy link

@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....

  1. One approach would be to use your fork and keep up to date with upstream changes.
  2. 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
Copy link
Author

gruve-p commented Feb 16, 2019

@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
Copy link

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
Copy link
Author

gruve-p commented Oct 17, 2019

@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
Copy link
Author

gruve-p commented Oct 17, 2019

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

@OttoAllmendinger
Copy link
Contributor

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

@gruve-p
Copy link
Author

gruve-p commented Jan 3, 2020

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

@OttoAllmendinger
Copy link
Contributor

I will use #45 to track the progress

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants