-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Hardware-wallets Clean up things I missed in the latest PR
#8890
Conversation
ca2ba60
to
a160a2e
Compare
lol did I merge too fast? |
No, I didn’t want to introduce new changes after it got approved ;) |
Error::Usb(err) | ||
} | ||
} | ||
|
||
/// Specifies the direction of the `HardwareWallet` i.e, whether it arrived or left | ||
#[derive(Debug, Copy, Clone)] | ||
pub enum DeviceDirection { | ||
Arrived, | ||
Left, | ||
/// Device arrived |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in "user pulled out the wallet from the usb port"? Maybe be explicit here and say Device became available
/Device was removed from the system
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dvdplm do you want s/Arrived/Available/ s/Left/Unavailable
renamed completely including the enum patterns?
Happy to change, I have no strong opinions here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would make sense, yes, but my grumble was about the doc-comment. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I having second thoughts on this because DeviceDirection
doesn't actually indicate whether a device became available or unavailable. Essentially, it only tells we got a notification
that a USB device
was connected or disconnected with manufacturer ID (Ledger or Trezor). To actually say that a device become available/unavailable we need to do additional checks, for instance, request an Ethereum address and so on!
So, I will keep this! Ok?!
hw/src/ledger.rs
Outdated
@@ -363,7 +364,7 @@ impl Manager { | |||
} | |||
|
|||
// Try to connect to the device using polling in at most the time specified by the `timeout` | |||
fn try_connect_polling(ledger: Arc<Manager>, timeout: &Duration, device_direction: DeviceDirection) -> bool { | |||
fn try_connect_polling(ledger: &Arc<Manager>, timeout: &Duration, device_direction: DeviceDirection) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just &Manager
? &Arc<Manager>
will be automatically dereferenced to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goood catch thanks 🥇
Do you mind merging master into this or rebasing? Makes it easier to review (also avoids having repeated commits in the changelog). |
6f74865
to
c8f2037
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a small nit regarding &Arc<Manager>
.
hw/src/trezor.rs
Outdated
@@ -439,7 +433,7 @@ impl <'a>Wallet<'a> for Manager { | |||
} | |||
|
|||
// Try to connect to the device using polling in at most the time specified by the `timeout` | |||
fn try_connect_polling(trezor: Arc<Manager>, duration: &Duration, dir: DeviceDirection) -> bool { | |||
fn try_connect_polling(trezor: &Arc<Manager>, duration: &Duration, dir: DeviceDirection) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use &Manager
instead.
hw/src/ledger.rs
Outdated
@@ -15,19 +15,20 @@ | |||
// along with Parity. If not, see <http://www.gnu.org/licenses/>. | |||
|
|||
//! Ledger hardware wallet module. Supports Ledger Blue and Nano S. | |||
/// See https://github.com/LedgerHQ/blue-app-eth/blob/master/doc/ethapp.asc for protocol details. | |||
/// See <https://github.com/LedgerHQ/blue-app-eth/blob/master/doc/ethapp.asc> for protocol details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe replace "///" with "//!".
* &Arc<Manager> -> &Manager * Rustdoc -> Crate doc
c8f2037
to
a194156
Compare
…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Include node identity in the P2P advertised client version. (openethereum#8830) Allow disabling local-by-default for transactions with new config entry (openethereum#8882) Allow Poll Lifetime to be configured via CLI (openethereum#8885) cleanup nibbleslice (openethereum#8915) Hardware-wallets `Clean up things I missed in the latest PR` (openethereum#8890) Remove debian/.deb and centos/.rpm packaging scripts (openethereum#8887) Remove a weird emoji in new_social docs (openethereum#8913) Minor fix in chain supplier and light provider (openethereum#8906)
No description provided.