search

symbol / symbol-cli

Command-line tool for Symbol
https://docs.symbolplatform.com/cli.html
Apache License 2.0
36 stars 24 forks source link

feat: Ledger integration!!! #380

Closed fboucquez closed 3 years ago

fboucquez commented 3 years ago

fix: generic profile types fix: option resolvers with generic types.

Hi team, I have integrated symbol-cli to Ledger. The user would be possible to sign and cosign transactions using the ledger device.

To try this, connect and open the symbol app in your device, then create/import a new profile and select ledger. Once you have the ledger profile, you can create, sign and cosign any transaction with ledger

The PR uses this dependency https://github.com/fboucquez/symbol-ledger-typescript. I need to open a conversation about the naming and where to put this repo. I'll start one on slack. This repo will probably be transferred to nemgrouplimited

In order to achieve this, I need to do a small refactoring. A ledger profile doesn't need or know a private key or a password to decrypt it. I had to change the hierarchy a little bit so the abstract profile doesn't have these properties. Private key, password, mnemonic phrase are part of the PrivateKey and HD profiles. A ledger profile doesn't ask for passwords as there is nothing to encrypt.

Another change is the signing operations, they are now async as there is a connection to the device and the user needs to click buttons to accept or reject a transaction. I have created the abstractions for async signing. This could potentially be moved to the ts SDK, at least one of the submodules once we split it.

cryptoBeliever commented 3 years ago

Below report from issues found during tests.

  1. When I'm creating a profile or I'm sending transactions all is fine but on the end, I got a message "Segmentation fault (core dumped)". Except for error, all work but it's worth checking.
  2. It's not possible to create a mosaic (below information what I chose)

Looking for a Ledger device... Symbol App version is v1.0.2 :heavy_check_mark: Do you want a non-expiring mosaic? … yes :heavy_check_mark: Enter the mosaic divisibility: … 6 :heavy_check_mark: Do you want this mosaic to have a mutable supply? … yes :heavy_check_mark: Do you want this mosaic to be transferable? … yes :heavy_check_mark: Do you want this mosaic to be restrictable? … no :heavy_check_mark: Amount of mosaics units to create: … 500000 :heavy_check_mark: Enter the maximum fee (absolute amount): … 1000000 :heavy_check_mark: Select the transaction announce mode: › normal

After that I got error:

TypeError: Cannot write to closed device
    at TransportNodeHidNoEvents.writeHID (/home/pmielcar/Developer/git/symbol-cli/node_modules/@ledgerhq/hw-transport-node-hid-noevents/lib/TransportNodeHid.js:91:21)
  1. When I'm sending secretproof, secretlock, mosaicglobalrestriction I got error 
Error
    at new TransportStatusError (/home/pmielcar/Developer/git/symbol-cli/node_modules/@ledgerhq/errors/dist/index.cjs.js:289:18)
    at TransportNodeHid.Transport.send (/home/pmielcar/Developer/git/symbol-cli/node_modules/@ledgerhq/hw-transport/lib/Transport.js:59:15)
Probably because Ledger app don't support this tx types. Should I ask Mohamad to add it?
  1. When I'm sending multisigmodification, aggregatekeylinks. I got error (same as for mosaic creation) 
    TypeError: Cannot write to closed device
at TransportNodeHidNoEvents.writeHID (/home/pmielcar/Developer/git/symbol-cli/node_modules/@ledgerhq/hw-transport-node-hid-noevents/lib/TransportNodeHid.js:91:21)
fboucquez commented 3 years ago

Thanks @cryptoBeliever

  1. I couldn't figure out the error and reported https://ledger-dev.slack.com/archives/C2D012G7Q/p1623335513121300 to the Leader team. If moving forward with this, we can spend more time trying to solve it. Does it happen in the wallet? I guess it's harder to see if in desktop mode.
  2. I'll have a look
  3. I've reported https://github.com/symbol/app-symbol/issues/2
  4. I'll have a look
fboucquez commented 3 years ago

Fixed 2 and 4 @cryptoBeliever .

Question, I have an option to connect symbol-cli to a speclus simulator. I think it's useful for testing, especially if you don't have a device or if you want e2e tests. Would you like that? Probably in another PR

cryptoBeliever commented 3 years ago
  1. I don't see it in desktop wallet console but as you told it doesn't matter that it does not happen. It's not a big issue because all to seem to work fine.
  2. Works fine now :+1:
  3. Thanks for reporting
  4. Works fine now :+1:

From my perspective, speculos simulator maybe not be so helpful since I'm using a physical device but sure if this is not big problem it may be helpful for other testers and developers.

fboucquez commented 3 years ago

@cryptoBeliever @Momo10101 should we merge this and try a Trazor implementation?

cryptoBeliever commented 3 years ago

@fboucquez from the functional part looks ok. But isn't it waiting for code review?

fboucquez commented 3 years ago

@fboucquez from the functional part looks ok. But isn't it waiting for code review?

@bassemmagdy @rg911 @Momo10101 ? cool for review?

Momo10101 commented 3 years ago

@cryptoBeliever @Momo10101 should we merge this and try a Trazor implementation?

Yes please! : )