[r2r] Hardware Wallet enhancements #1633

sergeyboyko0791 · 2023-01-26T12:03:29Z

Remove mm2_rmd160 property from the HD account table. This is required so either Iguana or an HD account share the same HD account records. Fixes Hardware Wallet storage should be shared between Iguana and all HD accounts #1621
Added new task::get_new_address::* RPCs that should replace the legacy get_new_address RPC. Fixes Possible to generate address on Trezor after disconnecting #1583
Added trezor_connection_status RPC to allow the GUI to poll the Trezor connection status Add trezor_connection_status RPC to allow the GUI to poll the Trezor connection status #1642

* Add `MmCtx::shared_db_id` * Add `get_shared_db_id` RPC

* Add `MmCtx::shared_sqlite_conn` * Remove `mm2_rmd160` property from HD wallet storage * Refactor SQL queries by using named params

* TODO implement `task::get_new_address::cancel` * Add `MockableConfirmAddress` for tests

shamardy

Great enhancement 🚀 ! Only non-blockers :)

mm2src/coins/hd_wallet_storage/mod.rs

borngraced

Great work! few suggestions and a question

mm2src/coins/hd_confirm_address.rs

mm2src/crypto/src/shared_db_id.rs

mm2src/coins/utxo/utxo_common.rs

* Add `trezor_connection_status` RPC * `HwCtx::trezor` should return `TrezorSession<'a>`

* Fix comments * Use `EnumFromInner`, `EnumFromStringify` derive macros

* Rename `NullStringPassphrase` to `EmptyPassphrase`

sergeyboyko0791 · 2023-02-06T15:31:57Z

@shamardy @borngraced the PR is ready for the next review iteration!
Please note that I also implemented #1642 issue since the last review.
Hope it won't make the review process complicated

borngraced · 2023-02-06T21:04:24Z

mm2src/trezor/src/transport/webusb.rs

@@ -44,14 +45,14 @@ struct WebUsbLink {
 impl Link for WebUsbLink {
    async fn write_chunk(&mut self, chunk: Vec<u8>) -> TrezorResult<()> {
        if !self.device.is_open().await? {
-            self.reconnect().await?;
+            return MmError::err(TrezorError::DeviceDisconnected);


does this mean device can no longer try to reconnect?

Yes, exactly. The main reason is this issue https://github.com/KomodoPlatform/WebDEX/issues/672.
Once the device is disconnected, we no longer need to try to reconnect as the GUI should notify the user to reinitialize Hardware Wallet via a Connect Hardware Wallet button.

Ok that makes sense thanks.

borngraced

Looks very good 🔥 💯 thank you for the great work!

shamardy

Thanks for the additional fixes! Only a question and minor changes.

shamardy · 2023-02-13T12:05:34Z

deny.toml

+    "RUSTSEC-2022-0084",
+    "RUSTSEC-2023-0004",


Can you please add small comments about these vulnerabilities like it's done for others?

This will no longer be needed once I merge dev into my branch. Thank you!

shamardy · 2023-02-13T13:10:52Z

mm2src/crypto/src/hw_ctx.rs

+            None => return HwConnectionStatus::Connected,
+        };
+
+        #[cfg(target_arch = "wasm32")]


I believe this is not needed anymore

You're right, will remove this compilation flag

shamardy · 2023-02-13T13:17:35Z

mm2src/trezor/src/client.rs

+            Some(inner) => TrezorSession { inner },
+            None => return Ok(None),
+        };
+        let features = session.initialize_device().await?;


Just a question to better understand what is happening, try_init_new_session_if_not_occupied is used only in trezor_connection_status, why do we have to initialize the device when we are getting the status?

There are three ways to check if the device is still connected:

Using webusb or libusb APIs that can provide such info. webusb provides an easy way to check the status, but libusb requires to sanitize much more actions to use its hotplug feature. It's also can be unimplemented on some OS.

Trying to ping the device and receive a pong. The problem with this approach is that if the HW device is locked (but still connected), it will require the user to enter a PIN. Even if we cancel the PIN request immediately once the device asked us about it, the device will draw the PIN matrix for a few milliseconds. This looks unfriendly.

Trying to initialize a new session by sending Initialize packet. It has little overhead since it returns the big Features response. But on the other hand, this request doesn't require the user to enter the PIN, so there is no any interaction on it.

I've chosen the last option as 1) requires more refactoring to implement the hotplug feature, and 2) looks unfriendly.
Btw, I just realized that is_connected is no more needed, will remove it.
https://github.com/KomodoPlatform/atomicDEX-API/blob/d28c11fb541dae04d1dcfb0200e9172676f309a6/mm2src/crypto/src/hw_ctx.rs#L100-L108

Also I'll add a comment that points to this discussion for better understanding.

Thanks a lot for the detailed explanation

Btw, I just realized that is_connected is no more needed, will remove it.

I was actually asking this because I thought that part of this code can be removed/refactored :)

anarkiVan · 2023-02-21T14:19:48Z

When the trezor security's option passphrase is enabled and i try to withdraw coin from WebDEX i got an error:
Internal error: Unexpected interaction request: PassphraseRequest
Also on each asset activation it asks to enter seed
ps: During connection to trezor i skip sending passphrase and our old logic just send empty string

sergei-boiko · 2023-02-21T21:41:45Z

All PR issues have been solved in #1672

shamardy · 2023-02-22T11:53:46Z

Superseded by #1672

sergeyboyko0791 added 3 commits January 25, 2023 15:02

Use a Shared DB in WASM for HD account storage

6f8d35c

* Add `MmCtx::shared_db_id` * Add `get_shared_db_id` RPC

Merge branch 'dev' into shared-db

872a138

Use a Shared DB for HD account sqlite storage

59e5c78

* Add `MmCtx::shared_sqlite_conn` * Remove `mm2_rmd160` property from HD wallet storage * Refactor SQL queries by using named params

sergeyboyko0791 linked an issue Jan 26, 2023 that may be closed by this pull request

Hardware Wallet storage should be shared between Iguana and all HD accounts #1621

Open

sergeyboyko0791 added 3 commits January 27, 2023 13:20

Add HDWalletCoinOps::generate_and_confirm_new_address

173f8e3

Add task::get_new_address::* RPCs

6b4ec0f

* TODO implement `task::get_new_address::cancel` * Add `MockableConfirmAddress` for tests

Minor changes

a3706d7

sergeyboyko0791 linked an issue Jan 31, 2023 that may be closed by this pull request

Possible to generate address on Trezor after disconnecting #1583

Open

sergeyboyko0791 changed the title ~~[wip] Implement a DB shared between Iguana and each HD accounts~~ [r2r] Hardware Wallet enhancements Jan 31, 2023

sergeyboyko0791 changed the title ~~[r2r] Hardware Wallet enhancements~~ [wip] Hardware Wallet enhancements Feb 1, 2023

Minor fixes and changes

66c43bb

sergeyboyko0791 changed the title ~~[wip] Hardware Wallet enhancements~~ [r2r] Hardware Wallet enhancements Feb 1, 2023

sergeyboyko0791 requested review from artemii235, shamardy and borngraced February 1, 2023 18:29

shamardy previously approved these changes Feb 3, 2023

View reviewed changes

mm2src/coins/hd_wallet_storage/mod.rs Outdated Show resolved Hide resolved

borngraced reviewed Feb 3, 2023

View reviewed changes

mm2src/coins/hd_confirm_address.rs Outdated Show resolved Hide resolved

mm2src/crypto/src/shared_db_id.rs Outdated Show resolved Hide resolved

mm2src/crypto/src/shared_db_id.rs Outdated Show resolved Hide resolved

mm2src/coins/utxo/utxo_common.rs Show resolved Hide resolved

Refactor Trezor so it cannot be used after being disconnected

d28c11f

* Add `trezor_connection_status` RPC * `HwCtx::trezor` should return `TrezorSession<'a>`

sergeyboyko0791 dismissed shamardy’s stale review via d28c11f February 6, 2023 14:34

sergeyboyko0791 added 2 commits February 6, 2023 15:48

Fix PR issues

3a2fd3d

* Fix comments * Use `EnumFromInner`, `EnumFromStringify` derive macros

Fix PR issues

12e7bee

* Rename `NullStringPassphrase` to `EmptyPassphrase`

sergeyboyko0791 linked an issue Feb 6, 2023 that may be closed by this pull request

Add trezor_connection_status RPC to allow the GUI to poll the Trezor connection status #1642

Closed

Ignore cargo deny issues for a while

bb0da39

borngraced reviewed Feb 6, 2023

View reviewed changes

borngraced approved these changes Feb 8, 2023

View reviewed changes

ca333 self-requested a review February 10, 2023 12:26

ca333 added the under review label Feb 10, 2023

shamardy requested changes Feb 13, 2023

View reviewed changes

sergeyboyko0791 mentioned this pull request Feb 19, 2023

[r2r] Complete MetaMask #1591

Closed

8 tasks

shamardy closed this Feb 22, 2023

shamardy deleted the shared-db branch February 22, 2023 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r2r] Hardware Wallet enhancements #1633

[r2r] Hardware Wallet enhancements #1633

sergeyboyko0791 commented Jan 26, 2023 •

edited

Loading

shamardy left a comment

borngraced left a comment

sergeyboyko0791 commented Feb 6, 2023

borngraced Feb 6, 2023

sergeyboyko0791 Feb 7, 2023

borngraced Feb 8, 2023

borngraced left a comment •

edited

Loading

shamardy left a comment

shamardy Feb 13, 2023

sergeyboyko0791 Feb 13, 2023

shamardy Feb 13, 2023

sergeyboyko0791 Feb 13, 2023

shamardy Feb 13, 2023

sergeyboyko0791 Feb 13, 2023

shamardy Feb 14, 2023

anarkiVan commented Feb 21, 2023 •

edited

Loading

sergei-boiko commented Feb 21, 2023

shamardy commented Feb 22, 2023

[r2r] Hardware Wallet enhancements #1633

[r2r] Hardware Wallet enhancements #1633

Conversation

sergeyboyko0791 commented Jan 26, 2023 • edited Loading

shamardy left a comment

Choose a reason for hiding this comment

borngraced left a comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced left a comment • edited Loading

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anarkiVan commented Feb 21, 2023 • edited Loading

sergei-boiko commented Feb 21, 2023

shamardy commented Feb 22, 2023

sergeyboyko0791 commented Jan 26, 2023 •

edited

Loading

borngraced left a comment •

edited

Loading

anarkiVan commented Feb 21, 2023 •

edited

Loading