-
Notifications
You must be signed in to change notification settings - Fork 8
Description
I've come across a few issues while trying to use this crate, which are described below.
But seeing that there is an old PR from 2023 that hasn't been merged yet, I'm wondering whether this crate is still maintained. If yes, I could try to come up with a PR. If not, I guess I'll have to fork it.
-
USB interface selection.
Ledger devices can have multiple USB interfaces, e.g. on my Nano S+ I can see two - №0 is the "normal" APDU interface and №1 is the FIDO/U2F one, which doesn't accept APDUs. The problem is that
UsbTransport::listreturns all the interfaces and it's impossible to tell one from another.In ledger-live they filter devices like this, i.e. they look for specific "usage page", if it is available (i.e. on Windows or Mac) or just take the interface №0 (on Linux). Neither
usage_pagenorinterface_numberare exposed by this crate, so it's impossible to do the filtering in the user code. But ideally, the crate should itself do the filtering. I suppose the correct way would be to rely onusage_pagewhere possible (Windows, Mac, Linux/hidraw) and resort tointerface_numberonly if it's not (Linux/libusb).Edit: it seems that the problem of multiple interfaces being returned is specific to Linux, because on Windows I was always getting only the APDU interface. But I guess it's still better to do the filtering regardless of the OS.
-
Bluetooth device selection.
This crate selects Bluetooth devices based on their names, which doesn't look very reliable. Also, it seems like only the NanoX and Stax models are currently supported. In ledger-live, on the other hand, they have this collection, whose
bluetoothSpec'sserviceUuidare then used for device selection.
Perhaps this crate should have a similar collection hardcoded? -
Not that this is super important, but TcpTransport uses port 1237, which is not the default apdu port for Speculos.
As I can see, the default apdu port for Speculos is 9999, so I'm wondering where the 1237 came from. Perhaps it's better to use 9999 instead?
-
Also I have a question about the statement (in the comments) that UsbTransport/UsbDevice are not really Send (or Sync), but are still marked as such as a workaround. Namely:
#[cfg(feature = "unstable_async_trait")] impl !Send for UsbDevice {} #[cfg(feature = "unstable_async_trait")] impl !Sync for UsbDevice {} #[cfg(feature = "unstable_async_trait")] impl !Send for UsbTransport {} #[cfg(feature = "unstable_async_trait")] impl !Sync for UsbTransport {} /// WARNING: THIS IS A LIE TO APPEASE `async_trait` #[cfg(not(feature = "unstable_async_trait"))] unsafe impl Send for UsbTransport {}This looks scary, but I'm not sure if it's needed.
As I understand, the original issue was that
hidapi::HidApirequired that only one instance of it could exist at at time. However, first of all, it doesn't seem to be the case in later versions of hidapi, as the documentation forhidapi::HidApinow says "The hidapi C library is lazily initialized when creating the first instance, and never deinitialized. Therefore, it is allowed to create multiple HidApi instances.". Secondly, even if it still behaved the old way, I'm not sure how marking it as non-Send would prevent somebody from accidentally constructing it several times. Also, a) I don't see how this could affect UsbDevice; b) in any caseunsafe impl Send for UsbTransportandimpl !Sync for UsbDeviceare redundant becaseUsbTransportis alreadySendandUsbDeviceis already!Sync.I.e. as I've said, this all looks redundant to me, but I'm wondering whether I could be missing something. If not, then perhaps this code can be removed.