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

Change TxBuilder.drain_to argument to Script instead of address String #279

Merged
merged 2 commits into from
Dec 11, 2022

Conversation

notmandatory
Copy link
Member

Description

Change TxBuilder.drain_to() argument to Script instead of address String. Also remove unneeded fn to_script_pubkey() fixes #261.

Notes to the reviewers

This change aligns bdk-ffi with bdk API which also takes a script instead of an address string for the TxBuilder.drain_to().

Changelog notice

  • APIs Changed
    • TxBuilder.drain_to() argument is now Script instead of address String.

Checklists

All Submissions:

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

@notmandatory notmandatory self-assigned this Dec 7, 2022
@notmandatory notmandatory added this to the Release 0.26.0 milestone Dec 7, 2022
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7d1a450.

How do you feel about fixing up the API docs as we go? Would you prefer we keep all the fixes for one big PR fix every release, or require people update as they open PRs that change the API?

@notmandatory
Copy link
Member Author

How do you feel about fixing up the API docs as we go? Would you prefer we keep all the fixes for one big PR fix every release, or require people update as they open PRs that change the API?

Yes good catch, we should keep the api-docs up to date as we make PR changes. I don't think for this one I only needed to change the method signature for Kotlin since the docs already match the rust docs. Doese it needs updating anywhere else?

@thunderbiscuit
Copy link
Member

Yep this is good to go. Ok will keep the docs in mind for future PRs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Investigate to_script_pubkey(address)
2 participants