hw_device: support for multiple devices added [for review] #4299
Conversation
src/device/device.cpp
Outdated
| for( const auto& sm_pair : devices.registry ) { | ||
| auto device = registry.find(device_descriptor_lookup); | ||
| if (device == registry.end()) { | ||
| MERROR("device not found in registry: '" << device_descriptor << "'\n" << "known devices:"); |
There was a problem hiding this comment.
That \n sucks because for grep etc it adds lines without the right format.
src/device/device.hpp
Outdated
| device& get_device(const std::string & device_descriptor); | ||
| }; | ||
|
|
||
| static std::unique_ptr<device_registry> registry; |
There was a problem hiding this comment.
It's a bad idea as you may end up with several versions of that. Put it in a cpp file.
src/simplewallet/simplewallet.cpp
Outdated
| } | ||
|
|
||
| } catch(std::exception & e){ | ||
| LOG_PRINT_L2(std::string("Device reconnect failed: ") + e.what()); |
There was a problem hiding this comment.
Prefer using the severity based macros in new code, not the old ones. Moreover, you don't have to cast anything: MERROR("Device reconnect failed: " << e.what()); Also the previous error is shown to the user, but not this one ?
src/simplewallet/simplewallet.cpp
Outdated
| const auto pwd_container = get_and_verify_password(); | ||
| if (pwd_container) | ||
| { | ||
| m_wallet->device_name(args[0]); |
There was a problem hiding this comment.
Doesn't seem to check there is an argument
src/simplewallet/simplewallet.cpp
Outdated
| try { | ||
| r = m_wallet->reconnect_device(); | ||
| if (!r){ | ||
| fail_msg_writer() << "Device reconnect failed"; |
There was a problem hiding this comment.
tr() for user visible output.
src/simplewallet/simplewallet.cpp
Outdated
| @@ -2188,6 +2189,28 @@ bool simple_wallet::set_ignore_fractional_outputs(const std::vector<std::string> | |||
| return true; | |||
| } | |||
|
|
|||
| bool simple_wallet::set_device_name(const std::vector<std::string> &args/* = std::vector<std::string>()*/) | |||
| { | |||
| const auto pwd_container = get_and_verify_password(); | |||
There was a problem hiding this comment.
If you can change that on the fly, which password are you verifying here, since the password is on the hw AFAIK ? No other place in simplewallet expects you can load another wallet on the fly. Is this actually needed ?
There was a problem hiding this comment.
Hmm you are right, the on the fly change of the device is not really needed.
The original idea is to address the situation where the device descriptor changes (device path) so the user can fix the device name / path, otherwise wallet won't connect / work.
This solution does not fix it well as the wallet cannot be started if the device is not found. I will just let to specify the device with the command line argument, that should do the job. Thanks!
src/simplewallet/simplewallet.cpp
Outdated
| { | ||
| bool r = m_wallet->reconnect_device(); | ||
| if (!r){ | ||
| fail_msg_writer() << "Failed to reconnect device"; |
src/simplewallet/simplewallet.cpp
Outdated
| } | ||
| catch (const std::exception &e) | ||
| { | ||
| fail_msg_writer() << "Failed to reconnect device: " << e.what(); |
src/wallet/wallet2.h
Outdated
| @@ -940,6 +941,8 @@ namespace tools | |||
| void ignore_fractional_outputs(bool value) { m_ignore_fractional_outputs = value; } | |||
| bool confirm_non_default_ring_size() const { return m_confirm_non_default_ring_size; } | |||
| void confirm_non_default_ring_size(bool always) { m_confirm_non_default_ring_size = always; } | |||
| std::string device_name() const { return m_device_name; } | |||
There was a problem hiding this comment.
could be const std::string &
|
@moneromooo-monero thanks for review! I will fix it soon |
ph4r05
force-pushed
the
multi-device
branch
3 times, most recently
from
12f140d to
135015f
Compare
| auto device_descriptor_lookup = device_descriptor; | ||
| if (delim != std::string::npos) { | ||
| device_descriptor_lookup = device_descriptor.substr(0, delim); | ||
| } |
There was a problem hiding this comment.
I wonder if adding a mutex for acessing the global would be better. It seems safe reading this anyway.
- device name is a new wallet property - full device name is now a bit more structured so we can address particular device vendor + device path. Example: 'Ledger', 'Trezor:udp', 'Trezor:udp:127.0.0.1:21324', 'Trezor:bridge:usb01'. The part before ':' identifies HW device implementation, the optional part after ':' is device path to look for. - new --hw-device parameter added to the wallet, can name the hardware device - device reconnect added
ph4r05
force-pushed
the
multi-device
branch
from
135015f to
f9b22a7
Compare
f9b22a7 hw_device: support for multiple devices added [for review] (Dusan Klinec)
Ledger,Trezor:udp,Trezor:udp:127.0.0.1:21324,Trezor:bridge:usb01. The part before:identifies HW device implementation, the optional part after:is device path to look for.