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

Add support for XpubOnly keys #586

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sanket1729
Copy link
Member

This allows users to directly operate on extended keys without always having to match on DescriptorPublicKey enum. This is quick hack PR, just looking for superficial feedback if this addresses issues by @quad.

All the APis that were supported on DescirptorPublicKeyare now also exported via XPubOnlyDescriptor (I am not happy names I chose in the PR).

Overall,

  • [] There is some code duplication in the last commit that I want to address later.
  • [] I am not happy with the names for exported descriiptors in lib.rs.

sanket1729 and others added 4 commits July 31, 2023 00:46
There are more specialized key types coming
Unlike descriptor publickey which supports multi xkeys as well as single
keys, this only supports extended keys. Makes the ergonimics much
simpler for most common wallet implementations

co-authored-by: Scott Robinson <ssr@squareup.com>
I am not happy with the names or the code duplication, but looking for
feedback on whether this solves the desired issue
@quad
Copy link
Contributor

quad commented Aug 14, 2023

… I guess this works? It adds yet another type. It'll mean that for now and into the future the methods need to be manually copied across. 🤷

@sanket1729
Copy link
Member Author

It adds yet another type.

My understanding is that most wallet developers will only use this type MsDescriptorXPubOnly. That way they get to use all the descriptor APIs (not only DescriptorPublicKey ones, but all MiniscriptKey ones) without having to deal with unwrap/err cases for Multi or SinglePub.

… I guess this works? It adds yet another type. It'll mean that for now and into the future the methods need to be manually copied across. shrug

I understand that replicating methods instead of implementing a unifying trait might seem contrived. We've succumbed to the allure of grouping similar-looking elements within traits. However, amalgamating two distinct entities to alter the trait signature into a Result would compel users to manage errors in methods that consistently yield Ok(). Alternatively, this approach might result in methods exclusively returning errors.

W.r.t to forgetting things, the DescriptorPublicKey enum contains the XPub, so we won't forget to add methods if add methods to DescriptorPublicKey type.

Perhaps, @danielabrozzoni also has some comments for this one.

@danielabrozzoni
Copy link
Contributor

I'm not confident enough with the project structure to give suggestions about how to implement this, but I think that having a xpub only descriptor would be really useful for most users - but I'm not sure how useful this would be for bdk, as I don't think we'll ever drop support for single key descriptors.
We actually have a quite similar situation in bdk with multixpub descriptors, as at the moment we just deny them (bitcoindevkit/bdk@792b39f), and in the future we plan to support multixpub by just translating them to a vector of xpub descriptors, as it seems easier to implement on our side, and seems to fit better the bdk architecture.

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.

3 participants