search

satoshipay / solar

🌞 Stellar wallet. Secure and user-friendly.
https://solarwallet.io
MIT License
191 stars 59 forks source link

Support Ledger hardware wallets #686

Open andywer opened 5 years ago

andywer commented 5 years ago

Ledger wallets come with a pretty convenient SDK: https://github.com/LedgerHQ/ledgerjs.

Open questions: Check out the SDK and see what needs to be changed technically.

andywer commented 4 years ago

Preliminary test result (#1062)

I'm afraid we have to change our approach here altogether… libusb and node-hid and its version conflicts with node.js and electron don't seem to be worth it.

I think our best bet is to use @ledgerhq/hw-transport-webusb and @ledgerhq/hw-transport-web-ble and run it in the web context. It should hardly be any less secure. The problem is rather that we might need to re-think the API now.

If the key store code is platform specific, but the hardware wallet code is running in the web context, then we cannot extend the key store interface by some hardware wallet - specific methods. We could, however, have a second virtual key store that is a facade for the Ledger SDK, though. Just an idea.

@ebma

ebma commented 4 years ago

Could you be a little more specific to what exactly is not working? If you only have trouble running/building the electron build try adding 

"postinstall": "electron-builder install-app-deps"

to the scripts in package.json (and run npm install afterwards). This should resolve the node-module-version conflict.

If you actually got the app running is the ledger device not recognised or what exactly is the problem? Just trying to understand because I can't test it myself.

andywer commented 4 years ago

It's multiple issues, actually. One is the building in CI due to libusb – would probably need to customize a docker image. Another one is the Ledger SDK transport driver with USD events crashing the process. The transport without USB events doesn't crash, but doesn't seem to recognize the device, possibly because of the node-hid vs node.js version mismatch.

I haven't tried electron-builder install-app-deps yet, but the overall point is that it feels very flimsy. QA-ing this will be tough and chances are that users might run into issues on their machines, potentially even if they do not use a hardware wallet.

That's why I propose using the Web USB and Web Bluetooth transports – that should be much less prone to errors and I know that this works as I can use my Ledger Nano S with https://accountviewer.stellar.org/. We can have a call about it another day if you like.

ebma commented 4 years ago

Hmm I see. Having a call about this sounds good.

andywer commented 4 years ago

@ebma Let's try https://www.npmjs.com/package/@ledgerhq/hw-transport-webauthn, too. Should be supported by Electron.

ebma commented 4 years ago

Necessary steps for bluetooth integration: