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 wallet apis #601

Merged

Conversation

thunderbiscuit
Copy link
Member

@thunderbiscuit thunderbiscuit commented Oct 7, 2024

This PR brings in more Wallet APIs listed in #596. Draft for now, I'll see about adding as much as I can today/tomorrow.

Changelog notice

Added:
  - Wallet::cancel_tx method [#601]
  - Wallet::get_utxo [#601]
  - Wallet::derivation_of_spk [#601]

[#601]: https://github.com/bitcoindevkit/bdk-ffi/pull/601

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

@thunderbiscuit thunderbiscuit force-pushed the feature/add-wallet-apis branch from 79dcf13 to c8a8983 Compare October 9, 2024 22:04
@thunderbiscuit
Copy link
Member Author

The two remaining methods we talked about exposing (Wallet::policies and Wallet::public_descriptor) require returning a simplified version of the Rust return type or adding new types we don't currently have.

I think both of those might be fine just returning strings (a policy string/json and a descriptor) but at the same time if we change our mind later we'll create a breaking change by switching the return type of the methods.

I'm thinking we can merge this PR as is and punt on those 2 methods which are not super requested and simply add them in a 1.1 release (or one of the later betas if we go to 6 and 7), potentially with their full types or simply strings after more discussion with the team. This way these methods can make it into the beta 5 release and we can move on to the TxBuilder methods we want to add (see #597).

What do you think @reez @notmandatory @rustaceanrob?

@rustaceanrob
Copy link
Contributor

My main fixation on public_descriptor is to allow 1.0 users to export their wallet somehow as of the first stable release. I agree that it seems to warrant a new type. I could be persuaded either way, but I felt that some wallet export support should be available from day one.

@rustaceanrob
Copy link
Contributor

Pull request LGTM

@thunderbiscuit
Copy link
Member Author

thunderbiscuit commented Oct 10, 2024

@rustaceanrob yeah true that we 100% need to make sure people can export/save. The reason I think we could get away with it is that right now the Wallet constructor requires a Descriptor (one of our sort of combined/merge type for ffi), which has a to_string()/Display method on it, which prints the same public descriptor that will come out of the Wallet::public_descriptor method.

Mind you we could just decide to return the public descriptor string too. We have a bit of time to think about it before the final 1.0 on the Rust side I think. Eventually I do think that a more complete exposition of the Descriptor types and their inner parts might be nice to have for us, because right now at the ffi layer you get much less than you do in Rust for those types, including things like rust-bitcoin keys stuff (PublicKey, ExtendedKey, etc.). One of the hiccups for them is that they're absolutely chuck-full of generics, so they need a bit of massaging. Maybe a job for bitcoin-ffi?

@reez
Copy link
Collaborator

reez commented Oct 10, 2024

ACK c8a8983

Can merge, and I think those final two things still to expose I'm interested in discussing more per #605 and bitcoindevkit/BDKSwiftExampleWallet#230

@thunderbiscuit thunderbiscuit merged commit c8a8983 into bitcoindevkit:master Oct 10, 2024
25 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.

3 participants