Closed mrfelton closed 5 years ago
This breaks compatibility with previous versions.
please make sure to run tests before submitting.
if you break an existing test you are likely breaking someone's app if we merge.
Then add a new test that checks your changes.
Yes, sorry. I submitted the PR before the code was really ready to share.
Just to get it out there incase others wanted it and as a reminder to myself to finish it off and get it merge ready - which I'll do at some point soon.
@junderw I've updated this PR to ensure that all of the tests pass. I've also updated the documentation with notes as to the new coin types.
I've reworked so that the API is exactly the same as before - just the addition of two new coin types litecoin
and litecoin_testnet
.
Any specific reason why coininfo is a git reference?
Can we use npm?
Other than that LGTM.
Yes, because they added support for litecoin recently, but have not made a new release yet.
See https://github.com/cryptocoinjs/coininfo/commits/master
4.3.0 was the last release, but there are several commits since then (inc the one we need)
Could you make the dependency explicitly state github?
iirc it implicitly gets the domain from this project's repository setting.
Explicit is better imo
@junderw I've updated the dependency to reference the full uri.
Published as 1.2.1
Adds basic support for litecoin.