Wallet persistence

[thunderbiscuit]

This PR brings in the flat file persistence feature.

Notes:

1. The tests have changed a little bit, because we now need a path to create this flat file and we need to destroy it at the end of each test.
2. Adding persistence required removing the memory wallet. This is a feature I'd like to bring back as soon as we can, given we find a good API for working with the generics around it.
   3. I found errors related to the creating the file common, so I decided to expose that error as part of this PR.

[thunderbiscuit]

Getting persistence to work with uniffi has been difficult, and a persistent PITA (see what I did there? 😆). Here is a writeup of the decisions and tradeoffs that I am contemplating at the moment. Note that as of this morning I decided that having an no-persistence wallet was a nice-to-have, but the burden of all the problems below come down to this idea of having multiple types of wallets (WalletNoPersist, Wallet, WalletSqlite, etc.), one for each of the generics for the Store type, so for now if I have to choose between two wallets I'd rather have the persisted one, and we can work on exposing a WalletNoPersist over time (this would at least unlock this PR and allow us to release a version with persistence).

Core Issue

The basic problem we are facing is that the BDK Wallet type is generic in its Store, but we can't expose generic types in uniffi. The first-pass solution was to create two structs for the two types of Stores; Wallet and WalletNoPersist. The problem arises when you attempt to pass this wallet to your TxBuilder: our current uniffi TxBuilder.finish() method takes in a Wallet...

Explored Solutions

Approach 1: Defining a Wallets trait which both Wallet and WalletNoPersist implement

This looks like this:

pub trait Wallets: Send + Sync {}

impl Wallets for WalletNoPersist {}
impl Wallets for Wallet {}

The approach above doesn't work because the trait doesn't define a get_wallet() method that the txbuilder can call, so you can update to something like this:

pub trait Wallets: Send + Sync {
    fn get_wallet(&self) -> MutexGuard<BdkWallet>;
}

But now you're back to square 1, because the get_wallet needs to return a BdkWallet with a particular file store.

Approach 2: Define an enum with the two wallet types

A second approach is to define an enum with the given wallets inside as fields on the variants, and then do a check on the enum when calling the txbuilder.finish() method:

pub enum Wallets {
    Wallet { wallet: Arc<Wallet> },
    WalletNoPersist { wallet: Arc<WalletNoPersist> },
}

When attempting this, we get a uniffi error:

Error: Objects cannot currently be used in enum variant data

Approach 3: Passing the TxBuilder to the TxBuilder.finish() method

This is super ugly, but you could in theory pull the Rust txbuilder out of the wallet, and simply pass it to the ffi txbuilder as an argument. The issue we get here is that the TxBuilder is actually generic in 4 ways, so it feels like pushing the problem forward without making any progress (I haven't even made a true attempt at this, because it also involves lifetimes and contexts, all of which would be hard to expose).

[reez]

I tested this bdk-ffi PR locally, looks good! Is there any testing on iOS you’d like me to do (if you already did testing on Android or anything I’m cool with that).



I remember there was mention of trying an alternative enum approach to Approach 2, is that something you tried as well (just wondering if I need to wait on anything from that experiment or anything before I ACK)



Thanks for adding in the errors, just wondering if you were planning on exposing them any further in this PR or if that was for a follow up PR? I see they are exposed but just turn into Alpha3Error Generic’s I think. I’m cool either way since it would be nice to have but I also know sometimes exposing these errors gets hairy and this is already a PR that is “a persistent PITA” as you said ha.

Looks like kuutamo worked out well for you instead of mempool, glad that was helpful for those tests, are you thinking we want to for sure switch all the tests to use it now instead of mempool for the time being?



Good job getting all the set up and tear down for db done across all platforms (iOS, Android, Python) tests, amazing! I’m sure that was no small feat, kudos.



I’ll wait for your feedback on any of my comments in this review, or if there is anything else you might be thinking about this PR, but overall I am ready to ACK. Great job working thru all of this and looking at all the angles.

[thunderbiscuit]

Update @reez: I have finally brought the errors out. A few things to note: our wallet constructor goes through actually two BDK functions:

• open_or_create_new()
• new_or_load()

Each of them returns a different error type, so I had to combine them into one, which I called WalletCreationError.

We could eventually decide to expose the Store itself as a type and its constructor, but I think it's not super useful at this point.

At this point I think this is ready to merge/iterate further in other PRs.

[reez]

Update @reez: I have finally brought the errors out. A few things to note: our wallet constructor goes through actually two BDK functions:

• open_or_create_new()
• new_or_load()

Each of them returns a different error type, so I had to combine them into one, which I called WalletCreationError.

We could eventually decide to expose the Store itself as a type and its constructor, but I think it's not super useful at this point.

At this point I think this is ready to merge/iterate further in other PRs.

whoa way to bring the errors out! yeah let me just pull it down locally and to a quick check again and then I will ACK in a few minutes

[reez]

ACK 43c1ca6