-
Notifications
You must be signed in to change notification settings - Fork 178
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
Enable importing joinmarket wallet as watch-only to mobile wallets #1267
base: master
Are you sure you want to change the base?
Enable importing joinmarket wallet as watch-only to mobile wallets #1267
Conversation
Concept ACK. Will review / test this and related PRs shortly. |
I agree, could be a very useful addition. I guess descriptors could also be seen as a big priority. (To be clear I don't have any plans to work on any of this, short term) |
I already have some code for generating output descriptors in one unfinished PR. That's relatively simple compared to descriptor parsing. So we could add that too. I think here we want to support both simple xpub and descriptors, as different mobile wallets will support different things, with only descriptors this functionality will be limited currently. |
Needs rebase.
Now that #1270 is merged, we can use output descriptors here, but not sure about wallet support, likely we need to support multiple ways for now - xpub, zpub/ypub and descriptors. |
64eb2a3
to
18be935
Compare
@AdamISZ @kristapsk , all comments addressed, PTAL, thanks! |
18be935
to
3703857
Compare
804c7dd
to
f58e74f
Compare
jmclient/jmclient/cryptoengine.py
Outdated
|
||
# Convert xpub to ypub or zpub for display | ||
if is_segwit_mode(): | ||
if is_native_segwit_mode(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong, should look at the wallet type not joinmarket.cfg
settings. Noticed by opening native segwit JM wallet with native = false
in joinmarket.cfg
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
# non-native segwit wallet. | ||
# If set to false, the extended public key will always display in the xpub | ||
# format. | ||
qt_display_ypub_zpub = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure default should be true, but probably. What do others think? @AdamISZ ? @chris-belcher ?
f58e74f
to
3900038
Compare
Co-authored-by: Kristaps Kaupe <[email protected]>
3900038
to
deb38ab
Compare
I've converted this PR back to Draft. I'm going to enable both options instead: xpub and zpub. So that the user can have the option to either one. |
Rethinking this - what is bad from UX perspective is multiple choices for GUI user, when he just wants to import pubkey in watchonly wallet. BlueWallet's wallet import screen says "Please enter your seed words, public key, WIF, or anything else you've got", nothing about xpub vs zpub. When provided with xpub, BlueWallet will assume BIP44 and user sees no balance. Maybe just displaying single option and having zpub as default is the right thing to do? Anyone has experinece with importing in various different popular wallets? |
My personal experience is that most mobile wallet does the same thing, and that's my motivation of this PR initially.
This is my preferred option, but it seems that not everyone agrees with this approach though. |
Any chance this will find its way into rpc api? There seems to be "problems" with the xpubs contained in the |
Based on #1266 and #1265.
What changed in this PR
Added a config
POLICY.display_ypub_zpub
to allow showing xpub inypub
orzpub
format.Both
ypub
andzpub
are BIP standards and are used by many mobile wallets: BlueWallet, Sentinel, etc. We have to show the pub key inypub
orzpub
format when we import joinmarket wallet to these mobile wallets in order for them to figure out the correct script to use: p2sh-p2wpkh and p2wpkh respectively.https://en.bitcoin.it/wiki/BIP_0049
https://en.bitcoin.it/wiki/BIP_0084
Motivation
With the combination of #1265 #1266 and this PR, we can now easily import one mixdepth of the joinmarket wallet as a watch-only wallet to our mobile wallets. (I tested the flow using Sentinel https://play.google.com/store/apps/details?id=com.samourai.sentinel&gl=US)
This way, whenever we want to receive a payment while traveling, we can generate a new wallet address using our mobile wallet. Later on, when we get back to our laptop, we can then start a tumbler process directly without worrying about transferring Bitcoin from other wallets to the joinmakert wallet.
(Eventually, we're going to replace xpub/ypub/zpub with output descriptors, but we're not quite there yet.)