-
Notifications
You must be signed in to change notification settings - Fork 38
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
Descriptor urtype account #426
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #426 +/- ##
===========================================
- Coverage 94.63% 94.62% -0.02%
===========================================
Files 57 57
Lines 7078 7083 +5
===========================================
+ Hits 6698 6702 +4
- Misses 380 381 +1 ☔ View full report in Codecov by Sentry. |
If the modification enables Krux to accurately load a descriptor from SS and display the correct receive and change addresses, then we could certainly implement it. |
Nice addition Jean! Plz add a simple test to cover this use case and I think it is good to merge! |
Yes, that is the intention of this pr... but it will only work for single-sig and mainnet (because seedsigner is currently exporting for mainnet, and for multisig, like krux, will only export a fraction of the cosigners at a time.) |
Thank you for reviewing. Tests added to check for UR:Output and UR:Account at the least. Tests for UR:Bytes would also be nice (but I don't know of a coordinator or signer, yet, for that use case... even though the code does exist). |
I know Specter-DIY uses UR:Bytes somewhere, maybe for PSBT |
It looks like the Specter Desktop wallet-descriptors are being qr-encoded as an alphanumeric dictionary, rather than UR:Bytes. Whenever I stumble on a coordinator that is exporting UR:Bytes as their wallet backup, then I'll add tests for that. We already have quite thorough UR:Bytes testing for PSBTs, it's in the ~/krux/tests/test_qr.py tests. |
Thank you! |
Description
I'm not sure if this belongs, but I found this while trying to scan an xpub exported by seedsigner for sparrow, while testing/inspecting the seedsigner release candidate for v0.8.0.
Without this snippet of code which adds support for urtype Account to existing support for urtype Output, krux raises and catches the following error (while loading a wallet descriptor):
Example) The following qrcode is decoded by SeedQReader as:
Conveniently, all 4 of the coordinators supported by both seedsigner/krux deal well with the xpubs exported for sparrow by seedsigner, and they're often scanned with better success, especially with a bad webcam, than the xpubs exported specifically for that coordinator. (For singlesig types, they're full descriptors with script wrapper and checksum, always with the mainnet "xpub" version bytes). In coordinators that support testnet mode... they force the keys to testnet and the resulting import is a "tpub" (this is not the case for krux, because krux respects the xpub exactly in watch-only mode, and would show a key mismatch if a wallet were loaded for testnet. I have a strong opinion that this is the most correct thing to do since krux is a signer that should favor being strict, while coordinators also have reason to favor being tolerant in order to create a usable wallet).
My reasoning for this code, is simply to verify between my only two signers... but it's more realistic that bitcoiners will be verifying descriptors from their coordinator (which works flawlessly whether setup by krux or seedsigner)
What is the purpose of this pull request?