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

Add support for Satochip hardware wallet #8967

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Toporin
Copy link
Contributor

@Toporin Toporin commented Mar 21, 2024

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 #8453 #7690, #7518 and #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)

Include new wizard Qt desktop client spesmilo#8560
WIP
TODO: build binaries, adapt qrcodewidget
In satochip plugin, a user can setup a 2FA using a qrcode.
The cancel button allows the user to cancel this action.
flake8 . --count --select="$ELECTRUM_LINTERS" --ignore="$ELECTRUM_LINTERS_IGNORE" --show-source --statistics --exclude "*_pb2.py,electrum/_vendor/"
./electrum/plugins/satochip/__init__.py:6:23: W292 no newline at end of file
available_for = ['qt']                      ^
1     W292 no newline at end of file
bitcoin.py/transaction.py: API changes: rm most hex usage

See also spesmilo@2f10955
@accumulator
Copy link
Member

Some notes:

  • The pysatochip repo does not seem to use tags to identify releases, only branches?
  • there's quite a few unused imports
  • there's quite a few coding style warnings (newlines, whitespace)

Using:
$ ELECTRUM_LINTERS='E,F,W,C90,B'
Remove unused imports, variables & code
@Toporin
Copy link
Contributor Author

Toporin commented Jun 20, 2024

I have removed unused imports and improved code style.

Close message dialog correctly when using 2FA
@accumulator
Copy link
Member

accumulator commented Jul 4, 2024

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

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

Toporin commented Jul 4, 2024

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

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

Toporin commented Aug 26, 2024

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

@accumulator
Copy link
Member

accumulator commented Sep 13, 2024

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.

@spesmilo spesmilo deleted a comment from Ehsankaz Sep 24, 2024
@Toporin
Copy link
Contributor Author

Toporin commented Sep 26, 2024

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.

@accumulator accumulator added this to the 4.6.0 milestone Oct 14, 2024
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

Successfully merging this pull request may close these issues.

2 participants