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.

Sign up for GitHub

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

Jump to bottom

implement Zpub-s for multisig GetAddress #1415

Merged
merged 5 commits into from Jan 21, 2021
Merged

implement Zpub-s for multisig GetAddress #1415

merged 5 commits into from Jan 21, 2021

Conversation

prusnak
Copy link
Member

@prusnak prusnak commented Jan 13, 2021 •
edited

This builds on top of PR #1420 so that needs to be merged first!

Fixes #855 (first bug out of two, the second one has been fixed already) - reported here: https://twitter.com/_benma_/status/1227900614505566208

@prusnak prusnak mentioned this pull request Jan 13, 2021
Closed
2 tasks
@prusnak prusnak added this to the 2021-02 milestone Jan 13, 2021
@matejcik
Copy link
Contributor

what is the usecase for ignore_xpub_magic here?

matejcik
matejcik approved these changes Jan 14, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

tentative approve, code LGTM

@prusnak
Copy link
Member Author

prusnak commented Jan 14, 2021 •
edited

what is the usecase for ignore_xpub_magic here?

So client can select whether they want to show multisig xpubs with xpub (old default) or Ypub/Zpub (new default) prefix.

Essentially the same logic I introduced in GetPublicKey for descriptors usecase.

Maybe we might want to rename ignore_xpub_magic (for both GetAddress and GetPublicKey) to enforce_xpub_prefix? Is that easier to understand?

@prusnak prusnak force-pushed the multisig-xpubs branch 3 times, most recently from 330eebd to e9f884b Compare January 19, 2021 11:13
@tsusanka tsusanka requested review from matejcik and removed request for tsusanka January 19, 2021 13:39
@prusnak prusnak requested a review from mmilata January 20, 2021 13:12
mmilata
mmilata approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Tested with Electrum:

  • create 2of2 p2wsh m/48'/0'/0'/2' wallet using 2xTT
  • create payment request in Receive tab
  • clicking on the eye icon, TT displays address, QR code, and the public keys in Zpub format
  • public keys equal those in Wallet>Information window

Changes LGTM, dunno why the cardano tests fail.

@prusnak
Copy link
Member Author

prusnak commented Jan 21, 2021

Rebased and force pushed - if the tests will be green again, we can merge

@prusnak prusnak merged commit 2d4b91b into master Jan 21, 2021
@prusnak prusnak deleted the multisig-xpubs branch January 21, 2021 22:46
@IanMichaelHarper
Copy link

  • clicking on the eye icon, TT displays address, QR code, and the public keys in Zpub format

I just tried this in electrum with the Trezor One and the eye icon fails to display. I don't know if there's another way to get the Trezor One to display its Zpub.

@prusnak
Copy link
Member Author

prusnak commented Dec 27, 2022

  • clicking on the eye icon, TT displays address, QR code, and the public keys in Zpub format

I just tried this in electrum with the Trezor One and the eye icon fails to display. I don't know if there's another way to get the Trezor One to display its Zpub.

Are you using multisig or not?

@IanMichaelHarper
Copy link

  • clicking on the eye icon, TT displays address, QR code, and the public keys in Zpub format

I just tried this in electrum with the Trezor One and the eye icon fails to display. I don't know if there's another way to get the Trezor One to display its Zpub.

Are you using multisig or not?

I am

@prusnak
Copy link
Member Author

prusnak commented Feb 8, 2023

I am

Will you please open an issue in the Electrum repository - https://github.com/spesmilo/electrum? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmilata mmilata mmilata approved these changes

@matejcik matejcik matejcik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
21.02
Development

Successfully merging this pull request may close these issues.

Match the XPUBs on the device with the ones on the client
4 participants
@prusnak @matejcik @IanMichaelHarper @mmilata