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.

Sign up for GitHub

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

Jump to bottom

Refactor wallet and persist mod, remove bdk_persist crate #1454

Merged
merged 7 commits into from
Jun 13, 2024
Merged

Refactor wallet and persist mod, remove bdk_persist crate #1454

merged 7 commits into from
Jun 13, 2024

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented May 31, 2024 •
edited
Loading

Description

Sorry to submit another refactor PR for the persist related stuff, but I think it's worth revisiting. My primary motivations are:

  1. remove db from Wallet so users have the ability to use async storage crates, for example using sqlx. I updated docs and examples to let users know they are responsible for persisting changes.
  2. remove the anyhow dependency everywhere (except as a dev test dependency). It really doesn't belong in a lib and by removing persistence from Wallet it isn't needed.
  3. remove the bdk_persist crate and revert back to the original design with generic error types. I kept the Debug and Display constrains on persist errors so they could still be used with the anyhow! macro.

Notes to the reviewers

I also replaced/renamed old Persist with StagedPersist struct inspired by #1453, it is only used in examples. The Wallet handles it's own staging.

Changelog notice

Changed

  • Removed db from Wallet, users are now responsible for persisting changes, see docs and examples.
  • Removed the bdk_persist crate and moved logic back to bdk_chain::persist module.
  • Added PersistBackendAsync, StagedExt, and StageExtAsync traits.
  • Added Wallet functions commit_to and commit_to_async.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@notmandatory notmandatory added this to the 1.0.0-alpha milestone May 31, 2024
@notmandatory notmandatory self-assigned this May 31, 2024
@notmandatory notmandatory force-pushed the refactor/persist_mod branch 6 times, most recently from 2fc8540 to 6b047ea Compare June 1, 2024 05:08
@notmandatory notmandatory marked this pull request as ready for review June 1, 2024 18:57
@notmandatory notmandatory modified the milestones: 1.0.0-alpha, 1.0.0-beta Jun 1, 2024
@notmandatory notmandatory force-pushed the refactor/persist_mod branch 3 times, most recently from 96c758c to 4ab0648 Compare June 2, 2024 05:00
@notmandatory
Copy link
Member Author

@matthiasdebernardini I added an async version of the persist trait called PersistAsync for you to test out with your sqlx crate, see 4ab0648.

@evanlinjin
Copy link
Member

evanlinjin commented Jun 3, 2024 •
edited
Loading

This is looking good. Removing the persistence backend from wallet is a welcome change. The main concern with doing so is newly revealed addresses not being persisted, but this is addressed with good documentation. Will have a proper review tomorrow. (give me a couple more days)

ValuedMammal
ValuedMammal reviewed Jun 10, 2024
View reviewed changes
crates/chain/src/persist.rs Outdated Show resolved Hide resolved
crates/sqlite/src/store.rs Outdated Show resolved Hide resolved
@ValuedMammal
Copy link
Contributor

Maybe worth keeping the Wallet::staged method in order to preview what's currently staged? or else consider returning a cloned changeset from apply_update.

@notmandatory notmandatory force-pushed the refactor/persist_mod branch 4 times, most recently from c2ac570 to c23d2bc Compare June 12, 2024 23:46
evanlinjin
evanlinjin reviewed Jun 13, 2024
View reviewed changes
crates/chain/Cargo.toml Outdated Show resolved Hide resolved
@notmandatory notmandatory force-pushed the refactor/persist_mod branch 2 times, most recently from 395ffa3 to 4b5166b Compare June 13, 2024 05:21
@evanlinjin evanlinjin force-pushed the refactor/persist_mod branch 3 times, most recently from c1b3e0c to ded0bfe Compare June 13, 2024 10:42
evanlinjin
evanlinjin approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK c9d1b41

notmandatory
notmandatory commented Jun 13, 2024
View reviewed changes
Copy link
Member Author

@notmandatory notmandatory left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6323d55

I rebased to add one more small fix to the bdk_wallet README.md.

notmandatory and others added 7 commits June 13, 2024 12:40
@notmandatory
refactor(persist): rename PersistBackend to Persist, move to chain crate
aa640ab 
Also add refactored StagedPersist to chain crate with tests.
@notmandatory
feat(persist): add PersistAsync trait and StagedPersistAsync struct
54b0c11
@notmandatory
refactor(persist): update file_store, sqlite, wallet to use bdk_chain…
9e97ac0 
…::persist

Also update examples and remove bdk_persist crate.
@evanlinjin @notmandatory
chore(chain): relax miniscript feature flag scope
36e82ec 
Still enable the `persist` submodule without `miniscript` feature flag.
Only disable `CombinedChangeSet`.

Also stop `cargo clippy` from complaining about unused imports when
`miniscript` is disabled.
@evanlinjin @notmandatory
feat(chain): reintroduce a way to stage changesets before persisting
2e40b01 
A staging area is helpful because we can contain logic to ignore empty
changesets and not clear staging area if the persistence backend fails.
@evanlinjin @notmandatory
feat(wallet)!: change persist API to use StageExt and StageExtAsync
19328d4
@evanlinjin @notmandatory
feat(example): use changeset staging with rpc polling example
ec36c7e
@notmandatory
Copy link
Member Author

notmandatory commented Jun 13, 2024 •
edited
Loading

Had to do one final force push after rebasing on master.

@ValuedMammal
Copy link
Contributor

ACK ec36c7e

@notmandatory notmandatory merged commit 782eb56 into bitcoindevkit:master Jun 13, 2024
12 checks passed
This was referenced Jun 14, 2024
Closed
Open
@evanlinjin evanlinjin mentioned this pull request Jun 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rustaceanrob rustaceanrob rustaceanrob left review comments

@evanlinjin evanlinjin evanlinjin approved these changes

@ValuedMammal ValuedMammal ValuedMammal approved these changes

Assignees

@notmandatory notmandatory

Labels
api A breaking API change module-blockchain module-database module-wallet
Projects
BDK
Archived in project
Milestone
1.0.0-alpha
Development

Successfully merging this pull request may close these issues.
4 participants
@notmandatory @evanlinjin @ValuedMammal @rustaceanrob