search

cryptoadvance / specter-desktop

A desktop GUI for Bitcoin Core optimised to work with hardware wallets
MIT License
793 stars 236 forks source link

Sign Over USB Option Missing #1681

Open dtrammell opened 2 years ago

dtrammell commented 2 years ago

I'm using a brand new Specter installation on a brand new Windows 11 laptop. I have verified that the computer can see my hardware USB signing device and so can Specter. When I go to sign a transaction, the Sign Over USB option is missing from the list of signing options.

Specter USB Configured Correctly Screenshot 2022-04-26 231722

Specter No USB Signing Option Screenshot 2022-04-26 231519

Specter Version: v1.9.2

Windows: Edition Windows 11 Home Version 21H2 OS build 22000.652 Experience Windows Feature Experience Pack 1000.22000.652.0

k9ert commented 2 years ago

This is dependent on the device you have created the wallet with. Can you check the type of the device? You can even change the type after the fact.

dtrammell commented 2 years ago

This wallet has 6 devices and it provides no USB method for any of them. They are Trezor T, Trezor One, Ledger, Coldcard, KeepKey, and Cabo Vault. I have used these devices with this wallet before, just on a different computer and instance of Specter Desktop. This is a new computer and new installation of Specter where I have imported the wallet using the backup JSON file.

k9ert commented 2 years ago

So, can you check the type of your devices? image If it's the wrong type, you should have a workaround and we seem to have a bug in the import mechanism. If it's not the wrong type, i'm still clueless why this happens.

dtrammell commented 2 years ago

Ah! I misunderstood what you were asking me to check. This indeed appears to be the problem. All of the imported devices are of type "Other". After updating the devices to the correct type, the problem has been resolved. Sounds like there is a bug in the import mechanism.

k9ert commented 2 years ago

I checked it and i agree that this is not so optimal:

relativisticelectron commented 2 years ago

I have been trying to fix this issue in https://github.com/relativisticelectron/specter-desktop/tree/add`device`type but I am running into inconsistent namings, that make it extremely confusing and error prone. I am however hesitant to make anything more consitent because it would break backwards-compatibility of imports:

What is the difference between label, name, alias and what would be a way to make this more consistent?

EDIT: I think the device/wallet export/import code needs cleanup.. e.g.

EDIT2

I would suggest to put a specter version in https://github.com/cryptoadvance/specter-desktop/blob/40357018e539ccd09089801fbb0fb574e1e334f8/src/cryptoadvance/specter/wallet.py#L882 and all other exports. That allows to write migration functions that kick in only if needed (this worked very well for me in the past).

k9ert commented 2 years ago

Thank you for your work here. It's difficult for me to dive in all the details you collected here.

What is the difference between label, name, alias and what would be a way to make this more consistent? I haven't been involved into the decisions about any of these conventions but i'm happy and definitely think we should make them more consistent. I don't think label was a good choice. name and alias made some sense for wallets (if a wallet already exists with that name on the core-node) but i have no idea why devices need an alias.

In general, i think we should tidy that up here. The specter-version inside the backupo-jsons is a good step for that. But we also need to tidy up the internal representation (although they are often quite identical). For that, i have a (pretty underused) Migration Framework which i did at some point in time. It's not very well documented yet but in short: