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

refactor(wallet)!: make internal descriptor optional in constructors #1513

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Jul 13, 2024

Description

WIP! needs more testing to ensure everything works without a change descriptor.

If OK then can add new constructors for single descriptor wallets in a post 1.0 release.

Built on #1512 and should fix #1511.

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

Updated load_with_descriptors() requires a ChangeSet and descriptors, validates they match. If the given descriptors have private keys signers are added.

Also rename load_from_changeset() to load() and remove no longer needed NewOrLoadError.
@notmandatory notmandatory added module-wallet api A breaking API change labels Jul 13, 2024
@notmandatory notmandatory self-assigned this Jul 13, 2024
@notmandatory notmandatory changed the title Refactor wallet constructors to make change descriptor optional refactor(wallet)!: make internal descriptor optional in constructors Jul 13, 2024
@notmandatory notmandatory added this to the 1.1.0 milestone Jul 13, 2024
@notmandatory notmandatory force-pushed the refactor/wallet_single_desc branch 3 times, most recently from a653e94 to dd03db4 Compare July 13, 2024 16:46
@storopoli
Copy link
Contributor

Nice! I think it is a good compromise for now.

By the way, this still checks if the descriptors are unique right?
What we're actually doing is making the internal optional, but the checks that we've did in #1390 still apply, right?

@notmandatory
Copy link
Member Author

notmandatory commented Jul 14, 2024

By the way, this still checks if the descriptors are unique right? What we're actually doing is making the internal optional, but the checks that we've did in #1390 still apply, right?

Yes I kept the check that prevents duplicate descriptors, I just don't add a second descriptor and use the external one for change addresses. So for example if the user asked for addresses or utxos from the internal keychain they'll always get None. Correction this would break the API since we don't currently return an Option when requesting addresses 🙁. This could be a bigger problem.

To keep the current API the same (not add Option for returned AddressInfo) we'd need to either 1. allow it to panic if requesting address for non-existent internal keychain, 2. if internal doesn't exist pull address from external keychain instead. In ether case would have to explain careful what could happen.

@notmandatory notmandatory force-pushed the refactor/wallet_single_desc branch from dd03db4 to 9855998 Compare July 18, 2024 01:32
@notmandatory
Copy link
Member Author

closing in favor of #1533

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change module-wallet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Please unbreak single-key wallets (and potentially remove internal/external distinction entirely)
2 participants