Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a new protocol to the library #7

Open
vikramIde opened this issue Jul 13, 2020 · 18 comments
Open

Adding a new protocol to the library #7

vikramIde opened this issue Jul 13, 2020 · 18 comments

Comments

@vikramIde
Copy link

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

@AndreasGassmann
Copy link
Member

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.

@vikramIde
Copy link
Author

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

@AndreasGassmann
Copy link
Member

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?

@vikramIde
Copy link
Author

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

@AndreasGassmann
Copy link
Member

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

@vikramIde
Copy link
Author

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

vikramIde commented Jul 15, 2020

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

@AndreasGassmann
Copy link
Member

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 :).

@vikramIde
Copy link
Author

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

@AndreasGassmann
Copy link
Member

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

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

@vikramIde
Copy link
Author

@AndreasGassmann I have added a new issue
airgap-it/airgap-wallet#31

@AndreasGassmann
Copy link
Member

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

@vikramIde
Copy link
Author

Not yet ,

Working on it

@vikramIde
Copy link
Author

@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

@AndreasGassmann
Copy link
Member

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!

@vikramIde
Copy link
Author

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

Okay nevermind sorry for that comment

@vikramIde
Copy link
Author

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 

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

No branches or pull requests

2 participants