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

Update bdk dependency to 0.22 #193

Merged

Conversation

thunderbiscuit
Copy link
Member

The bindings do not build when attempting this upgrade because get_balance() now returns a Balance struct (this was merged in bitcoindevkit/bdk#640)

error[E0308]: mismatched types
   --> src/lib.rs:433:9
    |
432 |     fn get_balance(&self) -> Result<u64, Error> {
    |                              ------------------ expected `Result<u64, bdk::Error>` because of return type
433 |         self.get_wallet().get_balance()
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found struct `Balance`
    |
    = note: expected enum `Result<u64, _>`
               found enum `Result<Balance, _>`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `bdk-ffi` due to previous error

When we upgrade to 0.22.0 we could decide to add the Balance struct to the bindings, or simply return the total by calling get_total(), which returns a u64 (same as we have now).

@thunderbiscuit
Copy link
Member Author

thunderbiscuit commented Sep 1, 2022

Because Balance is very simple (4 u64 fields) I decided to take a stab at bringing it in the FFI layer and test it in my test suite.

As is, the Balance struct works well, but the methods on it are not ported to the FFI layer. I'm not sure how to enable them without rewriting the struct in the FFI... @notmandatory any ideas? Would this be a case of using the typedef defined here?

I tried just for fun to add

[External="bdk"]
typedef extern Balance;

to the UDL file but unfortunatly it throws this error:

error[E0432]: unresolved import `bdk::FfiConverterTypeBalance`
    --> /Users/user/repos/bdk-ffi/target/release/build/bdk-ffi-28f1e274c858a20d/out/bdk.uniffi.rs:2621:5
     |
2621 | use bdk::FfiConverterTypeBalance;
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `FfiConverterTypeBalance` in the root

For more information about this error, try `rustc --explain E0432`.
error: could not compile `bdk-ffi` due to previous error

@notmandatory
Copy link
Member

notmandatory commented Sep 5, 2022

@thunderbiscuit I think we should make a bdk-ffi version of the Balance struct that also has fields for spendable and total. I can push a patch to this PR branch with the needed changes. This way we can leave Balance as a simple dictionary. Ok?

@thunderbiscuit
Copy link
Member Author

thunderbiscuit commented Sep 5, 2022

ACK a6af35c. Ready for merging when the new BDK version comes out.

@notmandatory notmandatory marked this pull request as draft September 5, 2022 21:30
@notmandatory notmandatory changed the title Test new BDK release candidate 0.22.0-rc.1 Update bdk dependency to 0.22 Sep 5, 2022
@notmandatory
Copy link
Member

notmandatory commented Sep 5, 2022

I rebased and updated the CHANGELOG. I also updated the PR title and changed this to a draft, once bdk 0.22 is published this PR can then be updated to replace the bdk 0.22.0-rc.1 version with the final 0.22 version.

@notmandatory notmandatory added this to the Release 0.9.0 milestone Sep 5, 2022
@notmandatory notmandatory mentioned this pull request Sep 5, 2022
21 tasks
@notmandatory
Copy link
Member

Updated PR branch with final bdk release version 0.22. This should be ready to merge.

@notmandatory notmandatory marked this pull request as ready for review September 8, 2022 13:38
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 3c6075a

@notmandatory notmandatory merged commit dfb350e into bitcoindevkit:master Sep 8, 2022
@thunderbiscuit thunderbiscuit deleted the test/release-candidate-0.22 branch November 14, 2023 15:47
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.

2 participants