-
Notifications
You must be signed in to change notification settings - Fork 56
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
Conversation
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?) |
There was a problem hiding this 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
tobdk_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
andlast_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 anOutPoint
type, convert appropriately instead of usingoutpoint.to_string()
.
BuildFeeBumpError::InvalidOutputIndex(outpoint) => CreateTxError::UnknownUtxo {
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. |
There was a problem hiding this 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(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Understood! 👍
Thanks for this. (I am taking notes for kotlin sample wallet 😁) |
Thanks for running those kotlin tests locally! |
No problem 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1a5545b
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. |
Description
Bump to wallet-2.0.0-beta.0
Notes to the reviewers
Changelog notice
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
Bugfixes: