Add selection of trezor device#1895
Conversation
OBorce
commented
Mar 20, 2025
OBorce
commented
- add a choice menu for the CLI wallet to select a trezor device if there are multiple devices available
- add optional parameters to the create and recover wallet commands to specify a trezor device name or device ID
- Fixed the leftover empty wallet when a create wallet command fails
OBorce
force-pushed
the
feature/trezor-device-selection
branch
from
1867f28 to
7c3dca3
Compare
ImplOfAnImpl
left a comment
ImplOfAnImpl
left a comment
There was a problem hiding this comment.
I didn't look at the UI part of the manual device selection yet, I only looked at the "automatic" selection logic.
The automatic selection didn't work for me out of the box - I connected 2 devices and opening a wallet immediately produced "No connected Trezor device found". Both in GUI and in CLI.
Turns out my wallet was created before I reset my device (wchich also resets the device id), so the device_id stored in the file was no longer relevant.
I guess we should always update the trezor data in the db once the wallet is successfully opened and keys are checked. This way the device id in the file will always be the latest one.
Anyway, as I've said in #1874, it's better to extract the automatic selection logic from this PR into 1874 and fix all the issues there as well.
| impl PartialEq<FoundDevice> for SelectedDevice { | ||
| fn eq(&self, other: &FoundDevice) -> bool { | ||
| self.name.as_ref().is_none_or(|name| name == &other.name) | ||
| && self.device_id.as_ref().is_none_or(|id| id == &other.device_id) | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think it's a good idea to compare labels when looking for a device. I think SelectedDevice should only contain device_id. Also, it should be non-optional, and Option<SelectedDevice> should be passed around instead.
| let selected = SelectedDevice::NONE; | ||
| let (client, data, session_id) = match find_trezor_device(selected) { | ||
| Ok(data) => (data.0, data.1, data.2), | ||
| Err(TrezorError::NoUniqueDeviceFound(_)) => { | ||
| if let Some(HardwareWalletData::Trezor(data)) = db_tx.get_hardware_wallet_data()? { | ||
| let selected = SelectedDevice { | ||
| device_id: Some(data.device_id), | ||
| name: Some(data.label), | ||
| }; | ||
|
|
||
| find_trezor_device(selected).map_err(SignerError::TrezorError)? | ||
| } else { | ||
| return Err( | ||
| SignerError::TrezorError(TrezorError::MissingHardwareWalletData).into(), | ||
| ); | ||
| } | ||
| } | ||
| Err(err) => return Err(SignerError::TrezorError(err).into()), | ||
| }; |
There was a problem hiding this comment.
I think the logic should be reversed - first we should look for the device with the device id stored in the db. And if it's not found, then we can try connecting to whatever device we have, provided that there is only one device.
Also, we shouldn't need 2 calls of find_trezor_device for that - find_trezor_device can itself choose the device.
| let selected = SelectedDevice::NONE; | ||
| let (mut new_client, data, session_id) = find_trezor_device(selected)?; |
There was a problem hiding this comment.
The device selection logic should work for reconnections too.
| pub struct FoundDevice { | ||
| pub name: String, | ||
| pub device_id: String, | ||
| } |
There was a problem hiding this comment.
The name will be empty if the label was never set (or if it was removed). I guess you have to obtain the model name too and then use it instead if the label is empty (it looks like Trezor Suite does exactly this).
There was a problem hiding this comment.
fixed to choose the label if not empty or use the model name
2 participants
