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

v2.9.4 - creating new Nano S wallet fails - QT4 #269

Closed
Danconnolly opened this issue Nov 18, 2017 · 7 comments
Closed

v2.9.4 - creating new Nano S wallet fails - QT4 #269

Danconnolly opened this issue Nov 18, 2017 · 7 comments

Comments

@Danconnolly
Copy link

The btchip library provided by ledger uses QT4, electron cash has switched to QT5.

We make use of dialogs provided by btchip to create a wallet linked to a Nano S.

Issue originally reported in the thread #149 but the cause is different from the first report of #149, so ive created this one.

@Danconnolly
Copy link
Author

One comment from #149, but note that there is other discussion in that thread.

Traceback (most recent call last):
  File "/home/user/Projects/electroncash/lib/plugins.py", line 150, in get_hardware_support
    p = self.get_plugin(name)
  File "/home/user/Projects/electroncash/lib/plugins.py", line 178, in get_plugin
    self.load_plugin(name)
  File "/home/user/Projects/electroncash/lib/plugins.py", line 102, in load_plugin
    p = loader.load_module(full_name)
  File "/usr/lib64/python2.7/pkgutil.py", line 246, in load_module
    mod = imp.load_module(fullname, self.file, self.filename, self.etc)
  File "/home/user/Projects/electroncash/plugins/ledger/qt.py", line 12, in <module>
    from btchip.btchipPersoWizard import StartBTChipPersoDialog
  File "/usr/lib/python2.7/site-packages/btchip/btchipPersoWizard.py", line 23, in <module>
    from PyQt4.QtGui import QDialog, QMessageBox
ImportError: cannot import name QDialog

Looks like the python2 btchip depends on QT4, we've moved to QT5. This problem will be present on Mac and Linux, not Windows.

@Danconnolly
Copy link
Author

Assuming we go forward with QT5, I think the "correct" thing to do, from an open source perspective, is to fork btchip, update for QT5, and submit the changes back to the btchip owners for inclusion in their library. However, this does not help us immediately, so we can do this but also adjust Electron Cash to use the forked btchip, until btchip updates for QT5 (whether they use our suggestion or something else).

@Danconnolly
Copy link
Author

PR submitted to btchip for QT5, works for me but complex to set up manually. Next step is to include the modified btchip in Electron Cash, until btchip with QT5 support is released.

@willyloman
Copy link

Thanks for opening this new ticket and submitting the patch to btchip. If you have any steps to follow to use the patched branch, it’d be good to post here for others, too.

Seeing as the PR in btchip to upgrade to Qt5 may also result in breaking changes for downstream dependencies (like Electrum), I’m not sure how quickly we can expect them to be merged. In the meantime, we should try to unblock users who have BCH funds in Ledger hardware wallets.

I’m still curious why we upgraded to Qt5. It seems like at this stage we should try to follow Electrum closely so we can incorporate changes wholesale and vice-versa. With that in mind, it seems like a simpler process change to simply rollback the upgrade to Qt5 because that is something we control and brings us into closer alignment with Electrum. What do you think?

@Danconnolly
Copy link
Author

Some clarification:

  • we are upgrading to Qt5 in the development version (cash branch)
  • the 2.9.4 release still uses Qt4 (pyqt4 branch)
  • Electrum has upgraded to Qt5 and Python 3 - the code of Electrum and Electron Cash has diverged significantly

This issue affects the development version only.

There is a bigger issue with support for the ledger devices #153 which is not addressed by this issue.

@willyloman
Copy link

@Danconnolly: Looks like this issue is fixed in v3.0 for Linux.

@marceloneil
Copy link
Collaborator

marceloneil commented Jan 7, 2018

@fyookball this should be fixed

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

No branches or pull requests

4 participants