Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tame overeager wallet manager #9262

Merged

Conversation

CriesofCarrots
Copy link
Contributor

Problem

The cli's wallet manager is indexing usb devices anytime it detects one, even if a command doesn't require a remote wallet. This is particularly problematic if a hardware wallet is connected but has timed out, because the cli may return a remote-wallet error instead of processing the command normally.

Summary of Changes

Add helper function to check for KeypairUrl::Usb args, and only initialize the wallet manager if one or more is present.

Fixes #8768

garious
garious previously approved these changes Apr 2, 2020
Copy link
Contributor

@garious garious left a comment

Choose a reason for hiding this comment

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

A test for check_for_usb would be nice. Also, check out the new matches! macro in Rust 1.42.

@CriesofCarrots
Copy link
Contributor Author

@garious , do you need this in 1.0?

@garious
Copy link
Contributor

garious commented Apr 2, 2020

No, just 1.1

@codecov
Copy link

codecov bot commented Apr 2, 2020

Codecov Report

Merging #9262 into master will decrease coverage by <.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #9262     +/-   ##
========================================
- Coverage      81%     81%   -0.1%     
========================================
  Files         276     276             
  Lines       61103   61103             
========================================
- Hits        49511   49498     -13     
- Misses      11592   11605     +13

@mergify mergify bot dismissed garious’s stale review April 2, 2020 19:38

Pull request has been modified.

@CriesofCarrots CriesofCarrots merged commit ec4745d into solana-labs:master Apr 2, 2020
mergify bot pushed a commit that referenced this pull request Apr 2, 2020
* Add helper fn to check for usb cli args

* Use helper fn to prevent wallet_manager connecting unnecessarily

* Review improvements

(cherry picked from commit ec4745d)
solana-grimes pushed a commit that referenced this pull request Apr 2, 2020
@CriesofCarrots CriesofCarrots deleted the tame-wallet-manager branch April 16, 2020 17:40
CriesofCarrots added a commit to CriesofCarrots/solana that referenced this pull request Apr 18, 2020
CriesofCarrots added a commit to CriesofCarrots/solana that referenced this pull request Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI pinging USB
2 participants