Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Hardware-wallets Clean up things I missed in the latest PR #8890

Merged
merged 2 commits into from Jun 18, 2018
Merged

Hardware-wallets Clean up things I missed in the latest PR #8890

merged 2 commits into from Jun 18, 2018

Conversation

niklasad1
Copy link
Collaborator

No description provided.

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 13, 2018
@niklasad1 niklasad1 force-pushed the na-clean-up-hardware-wallets branch from ca2ba60 to a160a2e Compare June 13, 2018 15:43
@5chdn 5chdn added this to the 1.12 milestone Jun 13, 2018
@5chdn
Copy link
Contributor

5chdn commented Jun 13, 2018

lol did I merge too fast?

@niklasad1
Copy link
Collaborator Author

No, I didn’t want to introduce new changes after it got approved ;)

dvdplm
dvdplm approved these changes Jun 13, 2018
View reviewed changes
hw/src/lib.rs
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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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!

Copy link
Collaborator

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. :)

Copy link
Collaborator Author

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?!

debris
debris approved these changes Jun 14, 2018
View reviewed changes
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 {
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goood catch thanks 🥇

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 14, 2018
@andresilva
Copy link
Contributor

Do you mind merging master into this or rebasing? Makes it easier to review (also avoids having repeated commits in the changelog).

@niklasad1 niklasad1 force-pushed the na-clean-up-hardware-wallets branch 2 times, most recently from 6f74865 to c8f2037 Compare June 14, 2018 10:25
andresilva
andresilva previously requested changes Jun 14, 2018
View reviewed changes
Copy link
Contributor

@andresilva andresilva left a 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 {
Copy link
Contributor

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe replace "///" with "//!".

@andresilva andresilva added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jun 14, 2018
@niklasad1
Clean up things that I missed
251aaad
@niklasad1
Address grumbles
a194156 
* &Arc<Manager> -> &Manager
* Rustdoc -> Crate doc
@niklasad1 niklasad1 force-pushed the na-clean-up-hardware-wallets branch from c8f2037 to a194156 Compare June 14, 2018 14:19
@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 14, 2018
@5chdn 5chdn dismissed andresilva’s stale review June 18, 2018 09:55

Review addressed

@5chdn 5chdn merged commit b472185 into master Jun 18, 2018
@5chdn 5chdn deleted the na-clean-up-hardware-wallets branch June 18, 2018 09:55
dvdplm added a commit that referenced this pull request Jun 18, 2018
@dvdplm
Merge branch 'master' into refactor/hashdb-generic
1d7dc83 
* master:
  Hardware-wallets `Clean up things I missed in the latest PR` (#8890)
  Remove debian/.deb and centos/.rpm packaging scripts (#8887)
  Remove a weird emoji in new_social docs (#8913)
ordian added a commit to ordian/parity that referenced this pull request Jun 20, 2018
@ordian
Merge branch 'master' of https://github.com/paritytech/parity into wa…
dc3ec66 
…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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dvdplm dvdplm dvdplm approved these changes

@debris debris debris approved these changes

@andresilva andresilva andresilva left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.0 Ethereum
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@niklasad1 @5chdn @andresilva @dvdplm @debris