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

Expose/get internal address #304

Merged

Conversation

xsats
Copy link
Contributor

@xsats xsats commented Jan 31, 2023

Description

Until this PR, bdk-ffi get_address did not support internal/change addresses. Any sufficiently advanced wallet will need this functionality, or be indistinguishable from an insufficiently-advanced wallet.

Notes to the reviewers

I created a separate mapping for get_internal_address instead of adding another field to get_address, which matches the existing bdk naming pattern and keeps the exposed get_{x_}address methods from being cluttered.

Changelog notice

Add wallet::get_internal_addresses API

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

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.

Approach ACK. Great addition to the API. It reminded me that I had in mind of taking all methods on the Wallet type one by one and asking myself "is there a reason to keep this out of the bindings?". In this case clearly there are none. Happy to see it added.

Similar to 305, you'll need to remove the to_string() calls in the test to pass the Clippy CI, and I'd like to have the Kotlin API docs fixed before merging. As mentioned in #305, I'm happy to do it, just let me know.

@xsats
Copy link
Contributor Author

xsats commented Feb 1, 2023

Thanks again, happy to fix this too. Can also help with adding other absent APIs.

From my standpoint this and #305 are definite candidates, and simple to implement so I jumped straight to PRs. But will open issues for any 'contentious' additions in terms of whether they're needed, or if there're multiple clear implementation variants etc.

@xsats
Copy link
Contributor Author

xsats commented Feb 1, 2023

and I'd like to have the Kotlin API docs fixed before merging. As mentioned in #305, I'm happy to do it, just let me know.

Similarly, if you don't mind adding for now that would be great thanks.

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.

One small fix to the tests is required.

bdk-ffi/src/wallet.rs Outdated Show resolved Hide resolved
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 aa842c3.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK aa842c3

Concept and code looks good!

@notmandatory notmandatory added the enhancement New feature or request label Feb 1, 2023
@thunderbiscuit thunderbiscuit merged commit aa842c3 into bitcoindevkit:master Feb 2, 2023
@xsats xsats deleted the expose/get-internal-address branch February 2, 2023 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants