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

migrating to hwi 2.1.0 #1693

Merged
merged 19 commits into from
Jul 5, 2022
Merged

migrating to hwi 2.1.0 #1693

merged 19 commits into from
Jul 5, 2022

Conversation

k9ert
Copy link
Collaborator

@k9ert k9ert commented May 2, 2022

Jade is currently not fully supported, see #1635 and this get fixed via migrating to hwi 2.1.0.
This is WIP:

We need a bit more Manual testing for this. Help very much appreciated. If you can check one of these boxes, please post in the comments:

  • Manual Test with Trezor One
    • device-creation
    • signing a TX segwit
    • signing a multisig TX segwit
    • msg-signing
  • Manual Test with Keepkey
    • device-creation
    • signing a TX segwit
    • signing a multisig TX segwit
    • msg-signing
  • Manual Test with Bitbox02
    • device-creation
    • signing a TX segwit
    • msg-signing
  • Manual Test with Ledger
    • device-creation
    • signing a TX segwit
    • msg-signing
  • Manual Test with Jade
    • device-creation
    • signing a TX segwit
    • msg-signing
  • Manual Test with Specter-DIY (via USB obviously)
    • device-creation
    • signing a TX segwit
    • msg-signing

@netlify
Copy link

netlify bot commented May 2, 2022

Deploy Preview for specter-desktop-docs ready!

Name Link
🔨 Latest commit 3b4ae94
🔍 Latest deploy log https://app.netlify.com/sites/specter-desktop-docs/deploys/62c3131812ebbc0009bdc5c2
😎 Deploy Preview https://deploy-preview-1693--specter-desktop-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@relativisticelectron
Copy link
Collaborator

relativisticelectron commented May 6, 2022

I found in #1688 that the error message has changed and I need to replace


by 535100b

    assert {
            'error': {
                'code': -32000,
                'message': "Internal error: HWIBridge.enumerate() got an unexpected keyword argument 'non_existing_parameter'."
            },
            'id': 1,
            'jsonrpc': '2.0'
            } == json.loads(req.data)

Do I assue correctly that this error is due to the newer version of hwi?

relativisticelectron added a commit to relativisticelectron/specter-desktop that referenced this pull request May 6, 2022
@k9ert
Copy link
Collaborator Author

k9ert commented May 6, 2022

Do I assue correctly that this error is due to the newer version of hwi?

I would assume so.

@k9ert
Copy link
Collaborator Author

k9ert commented May 18, 2022

Ok, so i don't have a ledger and for some reason my old keepkey doesn't work anymore for testing. I would love to at least test the keepkey, though as i removed the keepkey specific code which is no longer needed.
Anyone with a keepkey can test that?

@k9ert
Copy link
Collaborator Author

k9ert commented May 25, 2022

I didn't get any feedback from keepkey folks. Also no feedback from their support in order to get my keepkey back to work. I'm a bit annoyed, i have to say.

@evliu
Copy link

evliu commented May 26, 2022

looks like hwi 2.1.1 has been released with small updates. might as well jump ahead
https://github.com/bitcoin-core/HWI/releases/tag/2.1.1

@k9ert
Copy link
Collaborator Author

k9ert commented Jun 7, 2022

Doesn't look like it get any better with Keepkey. Not sure about HWI 2.1.0. I'll definitely move this forward this week.

@k9ert k9ert marked this pull request as ready for review June 10, 2022 08:31
@k9ert
Copy link
Collaborator Author

k9ert commented Jun 10, 2022

Ok, i think we should merge this and maybe shortly after this is released upgrade to HWI 2.1.1. I've tested it with 2.1.0, it has some major changes on our side and it's beneficial to ask people if they have issues to switch versions in order to figure out where an issue is coming from.

I think i've discussed the details of this with @moneymanolis and it seems that @relativisticelectron looked into it but we don't have a formal approval yet. Can one of you approve this?

@relativisticelectron
Copy link
Collaborator

I am surprised you did not run into the seemingly changed error message:
#1693 (comment)

I think i've discussed the details of this with @moneymanolis and it seems that @relativisticelectron looked into it but we don't have a formal approval yet. Can one of you approve this?

Sorry, I did not test hwi 2.1.0. However I could test hwi 2.1.0 with Ledger, as soon as I have time.

@k9ert
Copy link
Collaborator Author

k9ert commented Jun 19, 2022

I am surprised you did not run into the seemingly changed error message:

I spent some time trying to understand why that might be fut failed. No idea. But i also can't see the relevance for merging this.

Sorry, I did not test hwi 2.1.0. However I could test hwi 2.1.0 with Ledger, as soon as I have time.

I don't think Ledger is that critical. This PR explicitely changed a lot for keepkey and Trezor and enabled the new jade's. So keepkey testing would be more important.

I'll merge that tomorrow and will create a ReleaseCandidate so that people from the chat can potentially test the ledger and you don't have to. There are some folks in there who might be able to help here (again).

@moneymanolis moneymanolis linked an issue Jul 4, 2022 that may be closed by this pull request
@k9ert k9ert merged commit 4d2abc7 into cryptoadvance:master Jul 5, 2022
ankur12-1610 pushed a commit to ankur12-1610/specter-desktop that referenced this pull request Jul 12, 2022
* migrating to hwi 2.1.0

* removing keepkey and trezor (Multisig is now in hwilib)

* some helper to understand hwi

* fixing the transport-issue

* package docstring

* understand trezor and hwi

* uncomment again

* mark test to be skipped

* fix inproper use of hwilib
k9ert added a commit that referenced this pull request Sep 13, 2022
* changed class name

* Working requirements.in

* Reordered lines to see changes better

* Working minimal upgrades

* Added hashes

* - Upgraded python to 3.10 and bitcoin to v23.0
- Added libzmq to build process, to enable future use cases such as #778
- added --without-gui to remove configure warning that gui will not be built.

* Added line-break

* - Updated the embit version to 0.4.13
- pushed python-bitcoind image

* - Build registry.gitlab.com/c8527/specter/cirrus-jammy:20220504
- updated docker references

* - added cypress-base-ubuntu-jammy

* - added cypress-python

* - fixed unchanged registry path

* Fixed wrong path

* readme change

* Fixed path

* Changed github-changelog

* changes registrar links

* Fixed bitcoin version

* Set reset bitcoin version to 22

* Set bitcoin version to 22.0

* Fixed black issue via psf/black#2964 (comment)

* Revert for changelog

* Upgraded pytest because otherwise I get the error  pytest-dev/pytest#9195

* Added temporary fix for upgraded hwi version. Should be fixed in #1693

* more verbosity for page generation

* doc about python 3.10

* docs: update TOC

* yet another library necessary

* black

* Corrected tag

* Fixed wrong python version introduced in the merge commit

* Use same babel version across requirements and test_requirements

* Created the reuqirements.txt with python 3.8 such, that that netlify can create the python docs in python 3.8, while everything elese works also with python 3.10

* testing https://peps.python.org/pep-0508/#environment-markers

* CHanged just the txt

* Added comments

* electron docker build successful

* rename image to cypress-python:v9.7.0

* doc

* docker file rename

* fix pyinstaller pip error by changing requirements.in

* reference jammy electron dockerfile

See electron-userland/electron-builder#6922 (comment)

* updated all references to dockerimages

* fix release-build

* fix the image-version to something else than latest. pin all the stuff!

* revert to old image to avoid glibc issues

Co-authored-by: Kim Neunert <[email protected]>
Co-authored-by: k9ert <[email protected]>
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.

blockstream jade want connect
3 participants