-
Notifications
You must be signed in to change notification settings - Fork 307
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
Simplify wallet persistence #1547
Simplify wallet persistence #1547
Conversation
Concept ACK Even though we need to stop making breaking changes, this looks like like a reasonable isolated change that will simplify the persistence API for Wallet users. |
eee5efa
to
79f1051
Compare
LGTM, needs |
cd7eeab
to
c6962fb
Compare
This replaces `bdk_chain::PersistWith` which wanted to persist anything (not only `Wallet`), hence, requiring a whole bunch of generic parameters. Having `WalletPersister` dedicated to persisting `Wallet` simplifies the trait by a lot. In addition, `AsyncWalletPersister` has proper lifetime bounds whereas `bdk_chain::PersistAsyncWith` did not.
Changeset methods `.persist_to_sqlite` and `from_sqlite` no longer internally call `.init_sqlite_tables`. Instead, it is up to the caller to call `.init_sqlite_tables` beforehand. This allows us to utilize `WalletPersister::initialize`, instead of calling `.init_sqlite_tables` every time we persist/load.
Introduce `Wallet::staged_mut` method.
c6962fb
to
0616057
Compare
This forces the caller to use the same persister type that they used for loading/creating when calling `.persist` on `PersistedWallet`. This is not totally fool-proof since we can have multiple instances of the same persister type persisting to different databases. However, it does further enforce some level of safety.
4954b23
to
9600293
Compare
An idea would be to introduce some sort of test framework for Checks include:
Maybe a more elaborate version of bdk/crates/wallet/tests/wallet.rs Line 105 in e0822d7
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick review and found a few doc fixes but otherwise looks good. I'll do a deeper look this afternoon but so far looks ready to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a first pass. Also spent some time implementing AsyncWalletPersister
for a locally defined type and so far results are positive.
@@ -2312,6 +2303,15 @@ impl Wallet { | |||
} | |||
} | |||
|
|||
/// Get a mutable reference of the staged [`ChangeSet`] that is yet to be commited (if any). | |||
pub fn staged_mut(&mut self) -> Option<&mut ChangeSet> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub fn staged_mut(&mut self) -> Option<&mut ChangeSet> { | |
pub(crate) fn staged_mut(&mut self) -> Option<&mut ChangeSet> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that this method may be useful for some users. I.e. they don't need to query the staged area twice if they wanted to do something like we do in PersistedWallet::persist
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of discussion I think you can do it without ever getting the stage mutably such as having a method on Wallet
that resets the stage but doesn't return it, and this would prevent users from freely modifying their changesets.
/// Clears the stage.
pub fn clear_stage(&mut self) { self.stage = ChangeSet::default(); }
Then persist
would look like
/// Persist staged changes.
pub fn persist(&mut self, persister: &mut P) -> Result<bool, P::Error> {
if let Some(change) = self.inner.staged() {
P::persist(persister, change)?;
self.inner.clear_stage();
return Ok(true);
}
Ok(false)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I think it's a better approach to have/use a clear_stage
method, having explicit and clear meaning.
fn persist<'a>( | ||
persister: &'a mut Self, | ||
changeset: &'a ChangeSet, | ||
) -> FutureResult<'a, (), Self::Error> | ||
where | ||
Self: 'a; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if this would achieve the same thing
fn persist<'a>( | |
persister: &'a mut Self, | |
changeset: &'a ChangeSet, | |
) -> FutureResult<'a, (), Self::Error> | |
where | |
Self: 'a; | |
fn persist<'a>(&'a mut self, changeset: &'a ChangeSet) -> FutureResult<'a, (), Self::Error>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I think we still need Self: 'a
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I made purposely made persist
a non-member so that it's harder to call haha
@ValuedMammal @notmandatory I updated the docs! Let me know if everything makes sense. I'll be disappearing this weekend and be back Tuesday night. Feel free to push on commits for more doc fixes and merge (if appropriate). |
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f434c93 (pending comments by @oleonardolima )
/// persister implementations may NOT require initialization at all (and not error). | ||
/// | ||
/// [`persist`]: WalletPersister::persist | ||
fn initialize(persister: &mut Self) -> Result<ChangeSet, Self::Error>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit:
It might be more convenient for the implementer if the return type is Result<Option<ChangeSet>, Self::Error>
.
f434c93
to
340808e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 340808e
Description
Removed the persistence module in
bdk_chain
(which contained thePersistWith
andPersistAsyncWith
traits and aPersisted<T>
wrapper). Replaced it with simplified versions that areWallet
-specific.The new traits (
WalletPersister
andAsyncWalletPersister
) are simpler since they have less type-parameters (being wallet-specific). The old versions were more complex as they were intended to be used with any type. However, we need more time to finalize a works-for-all-bdk-types persistence API.Additionally,
WalletPersister
andAsyncWalletPersister
also introduce theinitialize
method. It is handy to contain db-initialization (i.e. create tables for SQL) here. We can callinitialize
inPersistedWallet
constructors. So thePersistedWallet::persist
method does not need to ensure the database is initialized. To accommodate this, I made the.init_sqlite_table
methods on changeset-types public, and removed the call of this infrom_sqlite
and.persist_to_sqlite
. Theinitialize
method now loads from the persister, so theload
associated function is removed.There was a bug in the old
PersistAsyncWith
trait where the lifetime bounds were not strict enough (refer to the conversation in #1552). This is now fixed in the newAsyncWalletPersister
trait.Docs for the new types are clearer (hopefully). I mentioned implementation details about the new traits and what safety checks were guaranteed in
PersistedWallet
.I think it makes sense just to have a wallet-specific persistence API which we will maintain alongside wallet for v1.0. This is less baggage for users and maintainers alike.
Notes to the reviewers
How breaking are these changes?
Unless if you were implementing your own persistence, then not breaking at all! If you were implementing your own persistence for BDK, the breakages are minimal. The main change is introducing the
initialize
method to persistence traits (which replacesload
).Acknowledgements
I came up with the idea of this while responding to @tnull's suggestions on Discord. Unfortunately we cannot fulfill all of his requests. However, I do believe that this will make things easier for our users.
Changelog notice
persist
module inbdk_chain
.WalletPersister
/AsyncWalletPersister
traits andPersistedWallet
struct tobdk_wallet
. These are simplified and safer versions of old structs provided by thepersist
module inbdk_chain
..init_sqlite_tables
method to be public on changeset types.from_sqlite
andpersist_into_sqlite
methods no longer call.init_sqlite_tables
internally.Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingWhat is left to do: