Skip to content

deps: bdk_wallet 2.0.0-beta #777

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

Merged
merged 1 commit into from
Jun 9, 2025
Merged

Conversation

reez
Copy link
Collaborator

@reez reez commented Jun 2, 2025

Description

Bump to wallet-2.0.0-beta.0

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

@reez
Copy link
Collaborator Author

reez commented Jun 3, 2025

Walking thru iOS bitcoindevkit/BDKSwiftExampleWallet#297 checking out all the breaks updating from 1.2 -> 2.0.0-beta, and adding comments on the expected/unexpected items that broke (and why?)

@reez reez changed the title (draft) deps: bdk_wallet 2.0.0-beta deps: bdk_wallet 2.0.0-beta Jun 4, 2025
@reez reez requested a review from Copilot June 4, 2025 14:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Bump FFI bindings to use bdk_wallet 2.0.0-beta.0 and align code with its updated APIs.

  • Updated imports and path references from bdk_core to bdk_wallet and adjusted patterns for added fields.
  • Added default initializations for new fields in change-set conversion impls.
  • Introduced error mapping for the new InvalidOutputIndex variant and bumped dependency versions.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

File Description
bdk-ffi/src/types.rs Switched imports to bdk_wallet paths, added .. patterns and default initializations for new fields.
bdk-ffi/src/error.rs Removed old PushBytesError import, added import from bdk_wallet, and mapped InvalidOutputIndex to UnknownUtxo.
bdk-ffi/src/electrum.rs Updated spk_client imports to bdk_wallet, and adjusted blockhash parse to use the new crate.
bdk-ffi/Cargo.toml Bumped bdk_wallet, bdk_esplora, bdk_electrum, and bdk_kyoto versions.
Comments suppressed due to low confidence (3)

bdk-ffi/src/types.rs:904

  • [nitpick] Default initialization of new fields first_seen and last_evicted should be covered by unit tests to ensure the conversion logic sets the expected default state.
first_seen: Default::default(),

bdk-ffi/src/types.rs:787

  • [nitpick] The new spk_cache field is initialized with the default value; consider adding tests to verify that the default cache behavior matches expectations.
spk_cache: Default::default(),

bdk-ffi/src/error.rs:1056

  • Verify that CreateTxError::UnknownUtxo expects a string representation of the outpoint; if it expects an OutPoint type, convert appropriately instead of using outpoint.to_string().
BuildFeeBumpError::InvalidOutputIndex(outpoint) => CreateTxError::UnknownUtxo {

@reez
Copy link
Collaborator Author

reez commented Jun 4, 2025

Ready for review.

Will probably do some follow ups based on my iOS app changes that were spurred from this update, but for this PR I'm keeping it focused on straight update to the beta.

@reez reez force-pushed the v2.0.0-beta-bump branch from 24986db to 08be0d4 Compare June 5, 2025 15:15
@reez reez requested a review from ItoroD June 6, 2025 13:32
Copy link
Collaborator

@ItoroD ItoroD left a comment

Choose a reason for hiding this comment

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

LGTM!

Ran tests locally (kotlin) too.

@@ -900,6 +901,8 @@ impl From<TxGraphChangeSet> for bdk_wallet::chain::tx_graph::ChangeSet<BdkConfir
txouts,
anchors,
last_seen,
first_seen: Default::default(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a comment here: Would we want to update TxGraphChangeSet to also have first_seen and last_evicted ? I see they are present for ChangeSet in bdk_wallet. Or maybe that can wait for later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea! I think we should do that. Let's do it in a separate PR? I'm thinking of this PR as a straight update of the dependency, and then doing follow up PR for something like exposing the new TxDetails, so if you want to do a follow up PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. Understood! 👍

@ItoroD
Copy link
Collaborator

ItoroD commented Jun 9, 2025

Walking thru iOS bitcoindevkit/BDKSwiftExampleWallet#297 checking out all the breaks updating from 1.2 -> 2.0.0-beta, and adding comments on the expected/unexpected items that broke (and why?)

Thanks for this. (I am taking notes for kotlin sample wallet 😁)

@reez reez force-pushed the v2.0.0-beta-bump branch from 08be0d4 to 1a5545b Compare June 9, 2025 15:48
@reez
Copy link
Collaborator Author

reez commented Jun 9, 2025

LGTM!

Ran tests locally (kotlin) too.

Thanks for running those kotlin tests locally!

@reez
Copy link
Collaborator Author

reez commented Jun 9, 2025

Walking thru iOS bitcoindevkit/BDKSwiftExampleWallet#297 checking out all the breaks updating from 1.2 -> 2.0.0-beta, and adding comments on the expected/unexpected items that broke (and why?)

Thanks for this. (I am taking notes for kotlin sample wallet 😁)

No problem 👍

Copy link
Collaborator

@ItoroD ItoroD left a comment

Choose a reason for hiding this comment

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

ACK 1a5545b

@reez
Copy link
Collaborator Author

reez commented Jun 9, 2025

Rebased, the only change I had to make was adding https://github.com/bitcoindevkit/bdk-ffi/pull/777/files#diff-d0e1303d15701dcbb56b78672717b2e6b21b60f59d120c0c400bf41ad4785befR11 which had to do with 30675df adding the from_merge constructor method to the ChangeSet struct.

After rebase I also retested locally built swift bindings again with BDK iOS app, built and ran, no changes needed.

@reez reez merged commit 1a5545b into bitcoindevkit:master Jun 9, 2025
26 checks passed
@reez reez deleted the v2.0.0-beta-bump branch June 9, 2025 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants