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

descriptors w/o fingerprints show "@ unknown" #493

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

jdlcdl
Copy link
Collaborator

@jdlcdl jdlcdl commented Dec 11, 2024

What is this PR for?

This PR will show "@ unknown" when loading a descriptor w/o origin/master fingerprint information, instead of showing the hd-key's own fingerprint.

Background:

All bip32 extended keys have their own fingerprint, and most have a parent fingerprint. These fingerprints are contained within or can be derived from the base58 xprv/xpub. But both of these are NOT what users think of "fingerprint" except for the root level bip32 master key... which is not available via xprv/xpub except for the master's depth=1 children. For this reason, the "origin info", containing "[fingerprint/derivation/path]" is included in many descriptors.

But since not all descriptors include the optional origin information, namely BlueWallet single-sig wallet backups, it is incorrect (or at least confusing to the user) to show a key's own fingerprint when they're actually expecting to see the origin/master fingerprint instead. If there is no origin information, it is better for the user to see "@ unknown" instead of a fingerprint they don't recognize.

Thank you Tadeubas for pounding this concept into my head, and for being tolerant enough till I understood. Because of this, I'm going to do better by using the terms "bip32 master fingerprint" or "origin fingerprint" instead of the more vague term "fingerprint" in future discussions/explanations.

What is the purpose of this pull request?

  • [*] Bug fix
  • New feature
  • Docs update
  • Other

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.31%. Comparing base (aaaa067) to head (12401c7).
Report is 15 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #493      +/-   ##
===========================================
- Coverage    94.85%   94.31%   -0.55%     
===========================================
  Files           74       74              
  Lines         7948     8086     +138     
===========================================
+ Hits          7539     7626      +87     
- Misses         409      460      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odudex
Copy link
Member

odudex commented Dec 11, 2024

Thank you @jdlcdl and @tadeubas !

@odudex odudex merged commit 97f50ba into selfcustody:develop Dec 11, 2024
7 checks passed
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.

2 participants