airgap-coin-lib icon indicating copy to clipboard operation
airgap-coin-lib copied to clipboard

Adding a new protocol to the library

Open vikramIde opened this issue 5 years ago • 18 comments

Hi guys,

What's the process of adding a new blockchain into this?, I wanted to add another staking network .

I felt this wallet is good as your security is awesome.

let me know the steps

Thanks

vikramIde avatar Jul 13 '20 14:07 vikramIde

Hi! Thanks for the interest in our project and thanks for the kind words regarding security. It's good to see there are people who care about this as much as we do :).

Before you start, I would like to say that we can't give you any guarantees that your PR will be merged in the end. Besides the effort it takes to review your code (which we will gladly do), adding a new protocol will add a lot of work in the future, such as keeping up to date with the latest blockchain changes, keeping the nodes running for the API, etc. So it's quite a commitment from our side.

There is no formal process for adding a new protocol to this library. Which currency would you like to add if I may ask?

The best place to start would be to take a look at some of the protocols we already support. Maybe take a look at a simpler one like AeternityProtocol.ts. The structure is pretty simple. There is an interface for preparing a transaction, signing and then broadcasting it. You basically just have to implement all the methods from that interface and it would automatically be compatible with both our Wallet and Vault.

We are very picky regarding including new libraries. So if possible, use some of the existing ones.

AndreasGassmann avatar Jul 13 '20 14:07 AndreasGassmann

Hi ,

Thanks for immediate response, I was thinking of including Harmony protocol, As they provide staking feature as well

I saw that you integrate with cosmos and Tezos , So thought Harmony could also be integrated .

I will review Aeternity's code now. And yes I can reuse the existing functions as well.

I run a validator node of Harmony. If you want I can become the api as well.

Regards

vikramIde avatar Jul 13 '20 16:07 vikramIde

I don't know much about Harmony, so right off the bat I can't tell you how well it fits into AirGap. But just give it a go and see how far you get, we'll discuss it internally in the team.

Do you have any contact to the people behind Harmony, like a foundation or something like that?

AndreasGassmann avatar Jul 13 '20 16:07 AndreasGassmann

Cool! I am starting from the library lik my own forked version, Then ill add it to the Vault and see things works fine.

Then Ill build a dummy API keeping your wallet spec same as push-backend.ts

If I am able to submit a transaction Ill ping you here :D, If you dont hear from me in a week I guess I would have failed

Yeah I had build some features in Harmony's delegation tool. So I know the core team And I am actually reviewing the Aeternity.ts file with them so that to understand what all we can reuse.

Thanks

vikramIde avatar Jul 13 '20 16:07 vikramIde

Ok let me know how it goes!

Just to reiterate: You should not need to do any code changes in the Wallet or Vault. You simply have to implement the ICoinProtocol interface, and because it's a delegateable coin, also the ICoinDelegateProtocol.

Then once you include this protocol in the Wallet / Vault, it should just work (the delegation part is the only one that probably needs some changes in the Wallet UI, because delegation flows are always a bit different.)

AndreasGassmann avatar Jul 14 '20 07:07 AndreasGassmann

@AndreasGassmann

Thanks for your steps, I am going to do this

  1. Make it work (take Harmony lib as it is and connect it to the ICoinProtocol)
  2. Convert harmony library to use existing packages from airgap core lib

Then ill submit for review here

vikramIde avatar Jul 15 '20 06:07 vikramIde

@AndreasGassmann when I ran test one of the test case failled is it something to be worried


vikrams-mbp:airgap-coin-lib vikrambhushan$ npm test

> [email protected] test /Users/vikrambhushan/Documents/Harmony/airgap-coin-lib
> nyc mocha --require ts-node/register --require source-map-support/register --full-trace --timeout 40000 ./test/**/**.spec.ts

sh: nyc: command not found
npm ERR! Test failed.  See above for more details.

After this I installed NYC npm i nyc tried to build the test again I got

vikrams-mbp:airgap-coin-lib vikrambhushan$ npm test

> [email protected] test /Users/vikrambhushan/Documents/Harmony/airgap-coin-lib
> nyc mocha --require ts-node/register --require source-map-support/register --full-trace --timeout 40000 ./test/**/**.spec.ts

Cannot find module 'ts-node/register'
Require stack:
- /Users/vikrambhushan/Documents/Harmony/airgap-coin-lib/node_modules/nyc/bin/nyc.js
npm ERR! Test failed.  See above for more details.

Then I did

vikrams-mbp:airgap-coin-lib vikrambhushan$ npm install ts-node --save-dev
+ [email protected]
added 8 packages from 40 contributors and audited 350 packages in 2.909s

7 packages are looking for funding
  run `npm fund` for details

found 3 vulnerabilities (2 low, 1 high)
  run `npm audit fix` to fix them, or `npm audit` for details
vikrams-mbp:airgap-coin-lib vikrambhushan$ npm test

> [email protected] test /Users/vikrambhushan/Documents/Harmony/airgap-coin-lib
> nyc mocha --require ts-node/register --require source-map-support/register --full-trace --timeout 40000 ./test/**/**.spec.ts

Error: spawn mocha ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:267:19)
    at onErrorNT (internal/child_process.js:467:16)
    at processTicksAndRejections (internal/process/task_queues.js:84:21)
npm ERR! Test failed.  See above for more details.

vikramIde avatar Jul 15 '20 08:07 vikramIde

Hi, I think we did not update the readme of how you have to setup and work with the project.

In order to prevent develop/test dependencies from being added to the package.json, we moved them out of the devDependencies. You'll have to do:

npm run install-test-dependencies

After that you can run the tests with npm run test.

To go back to the previous state (eg. to commit changes in the package.json), you have to do

npm run install-build-dependencies

BTW, can you please work with the develop branch if you don't already? There are some changes there that will give some conflicts later on if you continue working on master :).

AndreasGassmann avatar Jul 15 '20 14:07 AndreasGassmann

Hi,

Thanks for the reply . I should get a brownie point for this ;)

Okay I can start working on the dev branch.

Also i saw some bug while using the wallet, it get's stuck sometime when trying to use sign the tx from Vault.

Thhis happens when valut and wallet is on the same device

vikramIde avatar Jul 15 '20 16:07 vikramIde

Yeah we'll give you some points for it ;).

Could you describe the wallet bug further? I think I've never seen that one.

AndreasGassmann avatar Jul 16 '20 12:07 AndreasGassmann

@AndreasGassmann I have added a new issue https://github.com/airgap-it/airgap-wallet/issues/31

vikramIde avatar Jul 17 '20 16:07 vikramIde

Thanks. Did you make any progress on the implementation of the protocol?

AndreasGassmann avatar Jul 17 '20 16:07 AndreasGassmann

Not yet ,

Working on it

vikramIde avatar Jul 17 '20 18:07 vikramIde

@AndreasGassmann Here you can track my update.

https://github.com/vikramIde/airgap-coin-lib/blob/bobo-wallet/src/protocols/harmony/HarmonyProtocol.ts

Major issue I am facing is Like trying to use the existing library's from the airgap-coin-lib. So literally I have to match each and every function from harmony SDK and then use similar one's from airgap

vikramIde avatar Aug 03 '20 07:08 vikramIde

Yeah it's usually hard to find exact matches between the method because in our library some things are deliberately separated (eg. preparation and signing).

But good to see you are making progress!

AndreasGassmann avatar Aug 03 '20 08:08 AndreasGassmann

I see that you use two different types of identitfication for commits.

Can you please explain so I will follow the same method.

Fix(), Chore(), Feat()

vikramIde avatar Aug 03 '20 11:08 vikramIde

Okay nevermind sorry for that comment

vikramIde avatar Aug 03 '20 11:08 vikramIde

So I was able to pass all the testcases in the protocol.spec.ts

    ✓ should getPublicKeyFromMnemonic - should be able to create a public key from a corresponding mnemonic (101ms)
    ✓ getPrivateKeyFromMnemonic - should be able to create a private key from a mnemonic
    - getExtendedPrivateKeyFromMnemonic - should be able to create ext private key from mnemonic
    ✓ getAddressFromPublicKey - should be able to create a valid address from a supplied publicKey
    - getAddressFromExtendedPublicKey - should be able to create a valid address from ext publicKey
    ✓ getTransactionsFromAddresses - Is able to get list of tx using its address key (1016ms)
    ✓ prepareTransactionFromPublicKey - Is able to prepare a tx using its public key (1317ms)
    - prepareTransactionFromExtendedPublicKey - Is able to prepare a tx using its extended public key
    ✓ prepareTransactionFromPublicKey - Is able to prepare a transaction with amount 0 (2091ms)
    ✓ signWithPrivateKey - Is able to sign a transaction using a PrivateKey (1393ms)
    - signWithExtendedPrivateKey - Is able to sign a transaction using a PrivateKey
    ✓ getTransactionDetails - Is able to extract all necessary properties from a TX
    ✓ getTransactionDetailsFromSigned - Is able to extract all necessary properties from a TX
    ✓ should match all valid addresses
    ✓ getTransactionStatus - Is able to get transaction status
    ✓ signMessage - Is able to sign a message using a PrivateKey (47ms)
    ✓ verifyMessage - Is able to verify a message using a PublicKey```

But my code is still shit and non usable at the moment. I needed confidence hence just made it work using their existing library 

vikramIde avatar Aug 11 '20 03:08 vikramIde