Add address index to return type of get_address

[thunderbiscuit]

This is my first stab at adding an API from bdk into to bdk-ffi. Looking for feedback. In particular, I found I had to create a brand new struct in lib.rs (AddressInformation) whose purpose was mostly just to mirror the AddressInfo struct coming from bdk. I assume there is a better way and I should be able to use the type from bdk directly; please let me know if so.

This PR is marked as draft because if the approach is agreed upon I'd still need to change the get_new_address() method to follow the same API. I assume also worthy of discussion is whether we might as well bring the whole of AddressInfo into bdk-ffi, which would add the KeychainKind field (and associated type) to AddressInfo/AddressInformation.

This PR fixes #130.

Notes to reviewers

This PR changes the return type of get_last_unused_address() from String to AddressInfo.

pub struct AddressInfo {
    pub index: u32,
    pub address: String,
}

[thunderbiscuit]

Major update to this PR:

This now defines a method get_address() similar to the one in bdk, which takes an enum AddressIndex which specifies whether to return a "new" address or a "last unused" address.

I don't use the full AddressIndex enum from bdk which has the Peek(u32) and Reset(u32) variants, but instead define a subset of it with only the New and LastUnused variants.

[thunderbiscuit]

@artfuldev @notmandatory namespacing here is unclear for me, and I'm wondering if you have ideas to make sure it's consistent or insights in one way or another. Here is the problem as I see it:

It is sometimes the case that we can't expose a struct fully, because it itself uses another struct which itself uses another struct and so on, which would all need to be defined/exposed in the ffi layer but are not always relevant to the task at hand. In such cases we can define "simplified" versions of those structs which flatten the complexity. But now those new structures need naming, and the bdk names are quite well chosen, so it's a bit of a tricky situation, also because from the user's point of view they now have to know that AddressInformation actually maps to AddressInfo and the like.

Here is what I've done in this PR. Not sure if it was a clean/good idea or if something else might be better, but let me know what you think. In cases where I needed to simplify a struct, I kept/stole the name from bdk and redefined a simpler struct, and then wrote the conversion methods (thanks for the tip @artfuldev!) which pull from the fully qualified bdk struct. It looks something like this:

// we redefine a simpler version of bdk::wallet::AddressInfo
// it has an `address` field of type String
// instead of the `address` field of type `Address` defined in bitcoin::util::address::Address
pub struct AddressInfo {
    pub index: u32,
    pub address: String,
}

impl From<bdk::wallet::AddressInfo> for AddressInfo {
    fn from(x: bdk::wallet::AddressInfo) -> AddressInfo {
        AddressInfo {
            index: x.index,
            address: x.address.to_string()
        }
    }
}

[artfuldev]

This is fine. We can retain the names and use bdk namespace qualifiers to refer to the original types. I don't think it's a deviation. We do the same for transaction, TxBuilder, and for the database and blockchain configuration types.

[thunderbiscuit]

One more thing I'm not so sure on is the use or not of a reference in the type parameter in the definition of the From trait. The current code for TransactionDetails is this:

 impl From<&bdk::TransactionDetails> for TransactionDetails {
    fn from(x: &bdk::TransactionDetails) -> TransactionDetails {
        TransactionDetails {
            fees: x.fee,
            txid: x.txid.to_string(),
            received: x.received,
            sent: x.sent,
        }
    }
}

But mine doesn't take a reference:

impl From<bdk::wallet::AddressInfo> for AddressInfo {
    fn from(x: bdk::wallet::AddressInfo) -> AddressInfo {
        AddressInfo {
            index: x.index,
            address: x.address.to_string()
        }
    }
}

I'm unclear as per the tradeoff here between the two.

[artfuldev]

I've added a few minor improvements

[artfuldev]

We probably have to rebase after the bdk 0.18 update.

[thunderbiscuit]

Yeah I think we should wait until #150 and #148 are in and then I'll be able to finish the rebase. If I'm quick enough we can even fit it in 0.6.0.

[artfuldev]

Do we need this? I think it's fairly evident that we're shadowing/proxying here. If it's notes for the reviewers, we can add them as comments instead.

[thunderbiscuit]

Yeah agreed I'll remove them.

[artfuldev]

Same as above

[artfuldev]

nitpick: we can also alias bdk::wallet::AddressIndex as BdkAddressIndex

[thunderbiscuit]

I like it for consistency with the others!

[thunderbiscuit]

Thanks for the comments @artfuldev!

[artfuldev]

LGTM! 🎉