Add Wallet list_unspent method #158

zoedberg · 2022-06-09T10:21:01Z

Here is my attempt at implementing the list_unspent method.

In order to test this I've updated the bdk-ffi submodule in the bdk-kotlin project, ran commands to build and publish the android library to a local maven repository and finally imported the library on an Android project written in Kotlin.

The list seems to work fine, except for a strange behaviour I've noticed when calling the method multiple times.

So, in order to exclude bugs from my side, I've wrote a minimal Rust project that directly uses bdk 0.18 to list the unspents of an existing wallet.

From this test I've understood that creating a wallet using a bdk::database::SqliteDatabase database and calling the wallet sync method leads to each UTXO getting re-inserted in the database even if already present.

Here's a minimal piece of code that shows how to reproduce the bug:

let blockchain = ElectrumBlockchain::from(Client::new("ssl://electrum.blockstream.info:60002")?);
let database = bdk::database::SqliteDatabase::new("./testdb.sqlite".to_string());
let wallet = Wallet::new(
    "wpkh(tprv8ZgxMBicQKsPdqrqwQmJsdw7Sbt36hJPJyoCQ5BhFwfLcfVHQEwiwPykQPLT9AxUYS26BPQ4yrqTYYGG4HPg4UwJzLJCnK5L8YC6VAUb8nZ/84'/1'/0'/0/*)",
    Some("wpkh(tprv8ZgxMBicQKsPdqrqwQmJsdw7Sbt36hJPJyoCQ5BhFwfLcfVHQEwiwPykQPLT9AxUYS26BPQ4yrqTYYGG4HPg4UwJzLJCnK5L8YC6VAUb8nZ/84'/1'/0'/1/*)"),
    Network::Testnet,
    database,
)?;

wallet.sync(&blockchain, SyncOptions::default())?;

println!("Num unspents: {}", wallet.list_unspent()?.len());

>> Num unspents: 1

wallet.sync(&blockchain, SyncOptions::default())?;

println!("Num unspents: {}", wallet.list_unspent()?.len());

>> Num unspents: 2

println!("Num unspents: {}", wallet.list_unspent()?.len());

>> Num unspents: 2

wallet.sync(&blockchain, SyncOptions::default())?;

println!("Num unspents: {}", wallet.list_unspent()?.len());

>> Num unspents: 3

println!("Num unspents: {}", wallet.list_unspent()?.len());

>> Num unspents: 3

Inspecting the sqlite database shows that the UTXO actually gets re-inserted at each sync.

The error does not arise when the database is defined as MemoryDatabase::default().

thunderbiscuit · 2022-06-09T12:53:30Z

Hey thanks for your contribution!

I think the bug you're uncovering is described in #155 and will be fixed once we use bdk 0.19.0 (should be released in the next few days!). So that should be taken care of at least.

Once that's done we'll try and release a new version of bdk-ffi right away. Not sure if we'll try and add new features in or just bump to bdk 0.19.0, which is a fairly big upgrade in itself (@notmandatory what do you think?), but in any case I'm happy to take a look at this PR.

notmandatory · 2022-06-09T23:42:17Z

This looks like great stuff. As for the next bdk-ffi release the focus will be on upgrading to the bdk 0.19.0 release. But once that's out we can get back to adding new APIs like this one.

thunderbiscuit · 2022-06-17T19:18:13Z

src/lib.rs

+    }
+}
+
+pub struct LocalUtxo {


The BDK version of this struct has a boolean is_spent. I assume you didn't include it here because LocalUtxo will only be returned from the list_unspent() method, which means that boolean will only ever been set to false. In this case, however, I think the naming could lead to confusion if some developers are familiar with the Rust version of BDK.

I propose we either:

Rename the struct UnspentLocalUtxo to make explicit the fact that it is unspent, or

Add the is_spent boolean to it, which for now will only ever be false

I suggest we go with (2) because to me that feels more forward-compatible. If ever we implement other features that use the LocalUtxo struct we'll have it in its entirety already. I also feel like it might allow you to simplify some of the mapping between the LocalUtxo struct you have here and the one defined in BDK. Finally, we've been discussing how we want to try and keep the API in the bindings as close as we can to the BDK API. It's not always possible, but in this case I think it is.

I also feel that (2) is a better choice here so just added the field

thunderbiscuit · 2022-06-17T19:20:19Z

src/lib.rs

+    address: String,
+}
+
+pub enum KeychainKind {


I'm not sure we need the 0 and 1 here but maybe there is something I'm missing. Can you tell me why you added them and not just use the enum defined in BDK directly? (i.e. no need to define it here in Rust and you could simply have it in the UDL file I think)

Follow-up: my tests pass when refactoring the KeychainKind out of the lib.rs file like so:

// pub enum KeychainKind { // External = 0, // Internal = 1, // } // impl From<bdk::KeychainKind> for KeychainKind { // fn from(x: bdk::KeychainKind) -> KeychainKind { // match x { // bdk::KeychainKind::External => KeychainKind::External, // bdk::KeychainKind::Internal => KeychainKind::Internal, // } // } // } impl NetworkLocalUtxo for LocalUtxo { fn from_utxo(x: &bdk::LocalUtxo, network: Network) -> LocalUtxo { LocalUtxo { outpoint: OutPoint { txid: x.outpoint.txid.to_string(), vout: x.outpoint.vout, }, txout: TxOut { value: x.txout.value, address: bdk::bitcoin::util::address::Address::from_script( &x.txout.script_pubkey, network).unwrap().to_string(), }, keychain: x.keychain, } } }

Sorry, my mistake. I initially assumed that was a requirement of uniFFI but then forgot to try without. Thanks for the suggestion!

thunderbiscuit · 2022-06-17T20:05:15Z

src/lib.rs

+
+pub struct TxOut {
+    value: u64,
+    address: String,


At first glance it looks good to me, but it's still something I want to bring to the attention of other reviewers (@notmandatory, @artfuldev). The TxOut in BDK contains a Script instead of an address of type String. I don't think we need the Script at the moment, but it does change the shape of the API, so something to note.

thunderbiscuit

Thanks a bunch for tackling this @zoedberg! Coin selection is an important feature and this is the first step.

I left a few comments there for you; let me know if they all make sense.

thunderbiscuit · 2022-06-17T20:16:17Z

src/lib.rs

+trait NetworkLocalUtxo: {
+    fn from_utxo(x: &bdk::LocalUtxo, network: Network) -> LocalUtxo;
+}
+
+impl NetworkLocalUtxo for LocalUtxo {
+    fn from_utxo(x: &bdk::LocalUtxo, network: Network) -> LocalUtxo {


I'd love it if we started commenting our codebase more (the FFI layer can be quite opaque and I want to make sure we make it easy on new contributors), and I think this is a perfect place to start.

It took me a few minutes to understand what this NetworkLocalUtxo trait was for, and if I understand correctly, you added this trait because the TxOut struct now contains an address (String), and for that you need the Address::from_script() which requires a network. That's great stuff and let me know if I didn't understand correctly. Long-winded way to say... I think we should just add a small comment explaining what this NetworkLocalUtxo is used for 🤣.

I had some hard times finding a fitting and self-explanatory name for this trait, so I agree a comment could be useful here. For next contributions I will keep in mind that comments are welcome 😄

zoedberg · 2022-06-18T13:32:01Z

@thunderbiscuit A big thanks to you and everybody who participated to this project, it's a pleasure to contribute to such a well designed and written project. I agree with all the comments, working on in it right now, will push the changes soon :)

zoedberg · 2022-06-18T14:28:07Z

Rebased the PR branch and added the suggested changes. Tested them on Android where they seem to work fine :)

thunderbiscuit · 2022-06-22T17:41:41Z

This is ready for merge IMO. I'll wait for one more reviewer to chime in and will squash your commits into one + merge.

thunderbiscuit · 2022-07-11T19:31:11Z

@notmandatory would love your review on this. I'm not sure if we should merge before #154, as this is an easier and ready to go change, 154 might be still WIP a bit (but maybe not? happy to do it the other way too). In any case merging this will allow us to start reviewing #164, which I'd love to start working on this week.

notmandatory

ACK fad5e3c

zoedberg mentioned this pull request Jun 9, 2022

Add Wallet list_unspent method #134

Closed

notmandatory assigned zoedberg Jun 9, 2022

This was referenced Jun 10, 2022

Add TxBuilder control coin selection methods #131

Closed

Add coin selection methods #164

Merged

thunderbiscuit mentioned this pull request Jun 14, 2022

W24 BDK Language Bindings Dev Call bitcoindevkit/.github#10

Closed

8 tasks

thunderbiscuit added this to the Release 0.8.0 milestone Jun 17, 2022

thunderbiscuit reviewed Jun 17, 2022

View reviewed changes

zoedberg added 2 commits June 18, 2022 15:37

Add Wallet list_unspent method

8f5ca7f

Post review changes

28a9b30

zoedberg force-pushed the list_unspent branch from 6e048ef to 28a9b30 Compare June 18, 2022 14:24

thunderbiscuit requested a review from notmandatory June 22, 2022 17:40

Fix comment for NetworkLocalUtxo trait

fad5e3c

thunderbiscuit mentioned this pull request Jun 28, 2022

W26 BDK Language Bindings Dev Call bitcoindevkit/.github#11

Closed

notmandatory approved these changes Jul 12, 2022

View reviewed changes

Add list_unspent and add_data methods to changelog

2ac42b3

thunderbiscuit merged commit 692904d into bitcoindevkit:master Jul 12, 2022

This was referenced Jul 18, 2022

W28 BDK Language Bindings Dev Call bitcoindevkit/.github#13

Closed

W30 BDK Language Bindings Dev Call bitcoindevkit/.github#15

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Wallet list_unspent method #158

Add Wallet list_unspent method #158

zoedberg commented Jun 9, 2022

thunderbiscuit commented Jun 9, 2022

notmandatory commented Jun 9, 2022

thunderbiscuit Jun 17, 2022 •

edited

Loading

zoedberg Jun 18, 2022

thunderbiscuit Jun 17, 2022

thunderbiscuit Jun 17, 2022

zoedberg Jun 18, 2022

thunderbiscuit Jun 17, 2022

thunderbiscuit left a comment

thunderbiscuit Jun 17, 2022

zoedberg Jun 18, 2022

zoedberg commented Jun 18, 2022

zoedberg commented Jun 18, 2022

thunderbiscuit commented Jun 22, 2022

thunderbiscuit commented Jul 11, 2022

notmandatory left a comment

Add Wallet list_unspent method #158

Add Wallet list_unspent method #158

Conversation

zoedberg commented Jun 9, 2022

thunderbiscuit commented Jun 9, 2022

notmandatory commented Jun 9, 2022

thunderbiscuit Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

zoedberg Jun 18, 2022

Choose a reason for hiding this comment

thunderbiscuit Jun 17, 2022

Choose a reason for hiding this comment

thunderbiscuit Jun 17, 2022

Choose a reason for hiding this comment

zoedberg Jun 18, 2022

Choose a reason for hiding this comment

thunderbiscuit Jun 17, 2022

Choose a reason for hiding this comment

thunderbiscuit left a comment

Choose a reason for hiding this comment

thunderbiscuit Jun 17, 2022

Choose a reason for hiding this comment

zoedberg Jun 18, 2022

Choose a reason for hiding this comment

zoedberg commented Jun 18, 2022

zoedberg commented Jun 18, 2022

thunderbiscuit commented Jun 22, 2022

thunderbiscuit commented Jul 11, 2022

notmandatory left a comment

Choose a reason for hiding this comment

thunderbiscuit Jun 17, 2022 •

edited

Loading