airgap-it / airgap-coin-lib

A library that offers a unified API to prepare, sign and broadcast multiple cryptocurrencies.
https://airgap-it.github.io/airgap-coin-lib/
MIT License
143 stars 39 forks source link

Feature/add coin xrp #3

Open okipol88 opened 5 years ago

okipol88 commented 5 years ago

Hi all,

I am working on adding XRP to airgap-coiin-lib and airgap wallet in general. Currently I belive I have finished the coin-lib part. Any suggestions welcome as I do not use TypeScript (at all up till now) and Javascript (ok I had some time with it once). Not to mention this cross app framework and npm ;).

codecov-io commented 5 years ago

Codecov Report

Merging #3 into master will decrease coverage by 0.32%. The diff coverage is 58.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #3      +/-   ##
==========================================
- Coverage   61.96%   61.64%   -0.33%     
==========================================
  Files          47       49       +2     
  Lines        2190     2401     +211     
  Branches      201      221      +20     
==========================================
+ Hits         1357     1480     +123     
- Misses        765      848      +83     
- Partials       68       73       +5
Impacted Files Coverage Δ
src/serializer/unsigned-transaction.serializer.ts 100% <ø> (ø) :arrow_up:
src/index.ts 100% <100%> (ø) :arrow_up:
src/utils/supportedProtocols.ts 81.25% <100%> (+1.25%) :arrow_up:
src/serializer/utils/toBuffer.ts 100% <100%> (ø) :arrow_up:
src/protocols/xrp/XrpProtocol.ts 55.26% <55.26%> (ø)
src/utils/xrp/xrpKeyPair.ts 83.33% <83.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 324c7aa...2df4b6f. Read the comment docs.

okipol88 commented 5 years ago

There is a problem I am facing with bignumber when used by airgap-coin-lib in the vault 👎.

I have a reproducable example: This works fine on its own https://github.com/okipol88/airgap-coin-lib/tree/feature/add-coin-xrp When it is linked to be used here - iit fails with the aforementioned error https://github.com/okipol88/airgap-vault/tree/feature/add-coin-xrp

I've seen a similar issue here (I guess):

hildjj/node-cbor#88

Originally posted by @okipol88 in https://github.com/ripple/ripple-lib/issues/925#issuecomment-501868562

AndreasGassmann commented 5 years ago

Hi, thank you for opening this pull request and investing your time into improving our library. It's much appreciated!

I didn't have much time to look at it, but the base already looks solid. A couple of issues:

Thanks again for taking your time to work on this.

okipol88 commented 5 years ago

Hi, thank you for opening this pull request and investing your time into improving our library. It's much appreciated!

I didn't have much time to look at it, but the base already looks solid. A couple of issues:

  • We are currently in the process of minimising the number of dependencies we use. You added quite a few new ones, one of which is your personal fork of is sjcl. Is it possible to reduce the number of dependencies and re-use existing packages or replace old packages by re-using one of the new ones? In either case, before merging we will have to review those dependencies.
  • We currently cannot upgrade the typescript version because of incompatibilities with our apps. Luckily, we are already in the process of upgrading both our apps so we can do that. We already have an internal branch open where we change the typescript version, together with some other changes. So we need to merge our changes to master before we can merge this.
  • We will also need to review and test your changes thoroughly. We already have a tight schedule for our next few sprints, so we will most likely not be able to do that for some time, just to let you know. We will also have to get familiar with the XRP protocol so we can understand the specifics of what you did.
  • Are you in any way affiliated with XRP or are you a community member who would simply like to see XRP support in AirGap?

Thanks again for taking your time to work on this.

ok so one by one:

So a big subnotes:

AndreasGassmann commented 5 years ago

So I finally managed to clean up the branch we've been working on internally and make it public on github. You can find it here: https://github.com/airgap-it/airgap-coin-lib/tree/release/v0.5.0

It's still a work in progress, but the overall structure shouldn't change much.

There are a lot of changes, but most of them are related to linting and the structure of the project:

There are 2 more things we have planned that are not yet started, but will happen soon: