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.
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_coretobdk_walletand adjusted patterns for added fields. - Added default initializations for new fields in change-set conversion impls.
- Introduced error mapping for the new
InvalidOutputIndexvariant 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_seenandlast_evictedshould 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_cachefield 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::UnknownUtxoexpects a string representation of the outpoint; if it expects anOutPointtype, 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. |
ItoroD
left a comment
There was a problem hiding this comment.
LGTM!
Ran tests locally (kotlin) too.
| txouts, | ||
| anchors, | ||
| last_seen, | ||
| first_seen: Default::default(), |
There was a problem hiding this comment.
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.
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
Thanks for this. (I am taking notes for kotlin sample wallet 😁) |
Thanks for running those kotlin tests locally! |
No problem 👍 |
|
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 fmtandcargo clippybefore committingNew Features:
Bugfixes: