Add Dash network

[UdjinM6]

Please consider adding Dash network.
https://github.com/dashpay/dash
https://www.dash.org/

Thanks.

[dcousens]

@jprichardson I'm semi OK with this. Its been a top-5 contender for a while now.

Did we end up with a solution that wasn't using bitcoinjs-lib as a library for all the possible currency constants?

[dcousens]

@UdjinM6 ugh, tests are all failing because dash conflicts with bitcoin testnet.

[dcousens]

@UdjinM6 due to Dash conflicting with bitcoin testnet, it won't be able to be bundled with the default networks, even though it is within the top-5 market caps.

I think @jprichardson had a repository for these constants, if not, we'll make one such that a user is not constrained to what we bundle by default.

[UdjinM6]

@dcousens Ah, I see. I knew wif was the same but I didn't though it would make tests to fail.. sorry..
Thank you for you time!

[dcousens]

@jprichardson technically the tests don't need to fail anymore since there is no 'automatic' default. In fact, its only failing because our tests include all the networks in network.json, so I guess, we could still merge.

Re-opening for discussion.

[UdjinM6]

Thanks for re-opening. While failing tests like that was unexpected I actually think that code like the one in that test could be useful. Also changing Dash testnet wif would make things cleaner and simpler for everyone imo. I started discussion in our dev channel about changing this value and restarting testnet. I'll post here asap once I have other devs opinions.

[UdjinM6]

It seems like it's better to split this PR into 2 separate ones - for Dash mainnet and testnet respectively. I pulled latest commits from this repo and reworked commit in current PR to add only mainnet part of Dash network. Will create a new PR to add Dash testnet when we are ready.
Strange thing: all tests (npm test) passed fine on my local machine after that however Travis here doesn't like smth... 😕

[jprichardson]

Yeah, my repo is https://github.com/cryptocoinjs/coininfo. I'd be fine with redoing it / renaming it / whatever if someone felt there was a better way. It does however support Dash if it matters: https://github.com/cryptocoinjs/coininfo/blob/master/lib/coins/doge.js

[UdjinM6]

@jprichardson I was looking for some lib like this also! 👍 Starred, forked and submitted cryptocoinjs/coininfo#13 to update Dash info there too.

[dcousens]

@jprichardson it'd be nice to have the network format be consistent between the libraries so users can simply just use ECPair.fromWIF(..., networks.dash)

[UdjinM6]

Well, I pulled latest commits and rebased and still tests are failing (randomly actually): sometimes it's

1) bitcoinjs-lib (advanced) can create transactions using OP_CHECKLOCKTIMEVERIFY "before each" hook for "where Alice can redeem after the expiry is past":
     Error: timeout of 10000ms exceeded. Ensure the done() callback is being called in this test.

sometimes it's

1) bitcoinjs-lib (multisig) can spend from a 2-of-4 multsig P2SH address:
     Error: [object Object]
      at node_modules/cb-http-client/index.js:30:25
      at Request._callback (node_modules/httpify/index.js:27:7)
      at Request.self.callback (node_modules/httpify/node_modules/request/request.js:198:22)
      at Request.<anonymous> (node_modules/httpify/node_modules/request/request.js:1035:10)
      at IncomingMessage.<anonymous> (node_modules/httpify/node_modules/request/request.js:962:12)
      at _stream_readable.js:944:16

I guess that's because they rely on some external service and this has nothing to do with current PR.
Can you check this pls?

[dcousens]

@UdjinM6 you are correct, the faucet API provided by BlockTrail is really sketchy and seems to fail 1/2 the time, if the tests weren't so worthwhile I'd of removed them by now.
If you know of a API for a testnet faucet that works consistently, let me know 👍 .

These are just part of the integration tests, not the unit tests. For a change like this, what is most important is that the unit tests do not fail.

Sorry for the inconvenience.

[losh11]

I wouldn't add DASH until it matures with better and trusted third party APIs.

[dcousens]

Closing in favour of #518 (comment)

I wouldn't add DASH until it matures with better and trusted third party APIs.

I don't want these decisions to be political, hence why I'm in favour of the above.

[UdjinM6]

Ok, we'll just keep using my fork for now I guess.

Btw any examples of what kind of

better and trusted third party APIs

are needed for Dash to be included in the original lib?
Some examples that worked for DOGE maybe? That would be really helpful - I guess we could find something similar and provide them as a proof that we are ready (or once we are ready if we aren't yet).

Thanks!

pinging @losh11 as well

[dcousens]

@UdjinM6 I want to make it 100% clear that isn't against Dash in any way. The only reason litecoin and dogecoin are still included by default is so that we don't break SEMVER.
They will likely be removed in the next version too in favour of the solution provided by @jprichardson.

[UdjinM6]

@dcousens yep, thanks, no worries!
I'm just trying to follow recommendations and find out if there anything we are missing or anything else we can put our energy into to be/look more mature. Litecoin is old enough and must have more 3rd party APIs using it while doge is relatively young. Basically that's why I picked doge for examples - to find kind of a minimal requirements or smth like that.

Anyway, as I already mentioned we are completely fine using a fork (imo) and I'm surely looking forward to have the solution by @jprichardson too :)