Skip to content

Conversation

@scgbckbone
Copy link
Contributor

This is more of a discussion PR, why do we keep clients for unused hww open?

Scenario:

  1. create sigle sig wallet with one of your hww devices and encrypt it
  2. connect other hww to PC
  3. cloose and re-open wallet from 1.
  4. choices dialog from base wizard is opened with hww choices
  5. choose one of your hww to decrypt wallet
  6. after this wallet 1. is opened and connected to target device, yet all other hww are connected too, even if unused. This will cause that no other application can use them as they are already opened in electrum (but for no reason)

@scgbckbone scgbckbone mentioned this pull request Mar 7, 2022
@SomberNight
Copy link
Member

Right, I guess when the wizard scans for hw devices, it creates clients for all connected devices, in BaseWizard._choose_hw_device:

device_infos = devmgr.unpaired_device_infos(None, plugin, devices=scanned_devices,

client = self.create_client(device, handler, plugin)

why do we keep clients for unused hww open?

No reason I guess; it's not clear where we should close them.
For example, what the current PR is doing looks weird... what if there is another main window open with some device connected? We should not close that client...

Maybe unpaired_device_infos could close the clients when it finishes? Not sure... I did not think through all the implications though.

@scgbckbone
Copy link
Contributor Author

if there is already another window using some devices I think you won't even see them in wizard as they are already used, right?

@SomberNight
Copy link
Member

Yes, I believe that is correct. However here you are iterating list(devmgr.clients.items()), which I expect would include them.

@scgbckbone
Copy link
Contributor Author

you're right - I'm accessing it incorrectly - let me dig deeper here

@SomberNight SomberNight removed a link to an issue Aug 6, 2022
Closed
@SomberNight
Copy link
Member

The wizard was completely refactored in #8560, so this PR is now too stale.

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.

2 participants