spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.38k stars 3.07k forks source link

Add support for Satochip hardware wallet #8967

Open Toporin opened 6 months ago

Toporin commented 6 months ago

This is a pull request to add support for the Satochip hardware wallet.

The Satochip hardware wallet is based on a jacavard smartcard and is fully open-source. The wallet is composed of a javacard applet that is to be loaded on the smartcard, and an Electrum client plugin that acts as the interface between the card and the network. An optional 2FA setting allows to confirm transaction requests on a second android device before signing them (iOS version coming soon).

This pull request updates and supersedes https://github.com/spesmilo/electrum/pull/8453 https://github.com/spesmilo/electrum/pull/7690, https://github.com/spesmilo/electrum/pull/7518 and https://github.com/spesmilo/electrum/pull/6520.

Changes compared to previous PR: Based on latest release Electrum v4.5.4 Use pysatochip v0.12.6 (using minimum functionalities and dependencies) *Suport for Satochip applet v0.12

New functionalities in Satochip applet v0.12: Card authenticity verification based on device certificate & PKI Reset to factory option (e.g. if user forget his PIN code or 2FA device) *Support for importing encrypted seed from a SeedKeeper

More info: https://github.com/Toporin/ (official repository) https://pypi.org/project/pysatochip/ (pysatochip library) https://prezi.com/p/mpq-xhh3mxjl/satochip-gent-meetup/ (Slides from previous meetups in Belgium) https://t.me/Satochip (Telegram support)

accumulator commented 3 months ago

Some notes:

Toporin commented 3 months ago

I have removed unused imports and improved code style.

accumulator commented 2 months ago

I noticed one typical use-case is not supported yet. The user can't generate a new seed and store it on the card, the user must already have a (BIP39) seed available..

accumulator commented 2 months ago

one remark w.r.t coding style; there's many java style parenthesis used in if statements :)

so e.g.

if (condition):

instead of

if condition:

Toporin commented 2 months ago

I noticed one typical use-case is not supported yet. The user can't generate a new seed and store it on the card, the user must already have a (BIP39) seed available..

Yes, that's because electrum does not allow BIP39 generation (by design choice) because it is considered insecure (as there is no seed versioning). Only importing existing BIP39 seed is allowed for compatibility. Also, importing an electrum seed would break compatibility with other hardware wallets, as the (de-facto) standard for hardware wallet is to import a BIP39.

We provide another tool for generating (and importing) BIP39 seeds and other utilities (like reset, change PIN...): Satochip-Bridge which will be soon replaced with Satochip-Utils.

accumulator commented 2 months ago

Also, importing an electrum seed would break compatibility with other hardware wallets, as the (de-facto) standard for hardware wallet is to import a BIP39.

Correct, we don't generate BIP39. I noticed that an initialized card can be used to create a new wallet, where the user can select derivation path and script type, so electrum seeds would indeed be problematic here, potentially leading to hard to recover funds.

Toporin commented 1 month ago

Hi, is there anything we can do to help with this PR?

accumulator commented 2 weeks ago

Hi, is there anything we can do to help with this PR?

IMO it looks pretty good already, but as I described above, there's still some java-isms w.r.t parenthesis

Also, there' still no release tags on the pysatochip repo.

Toporin commented 4 days ago

I have removed the unnecessary parenthesis from the Satochip plugin code. Also the release tags were added on the pysatochip repo: https://github.com/Toporin/pysatochip/tags. Finally, I upgraded the Satochip plugin from PyQt5 to PyQt6.