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

Remove persist submodule #1473

Merged
merged 3 commits into from
Jun 15, 2024
Merged

Remove persist submodule #1473

merged 3 commits into from
Jun 15, 2024

Conversation

evanlinjin
Copy link
Member

Description

@LLFourn suggested these changes which greatly simplifies the amount of code we have to maintain and removes the async-trait dependency. We remove PersistBackend, PersistBackendAsync, StageExt and StageExtAsync completely. Instead, we introduce Wallet::take_staged(&mut self) -> Option<ChangeSet>.

The original intention to keep a staging area (StageExt, StageExtAsync) is to enforce:

  1. The caller will not persist an empty changeset.
  2. The caller does not take the staged changeset if the database (PersistBackend) fails.

We achieve 1. by returning None if the staged changeset is empty.

2. is not too important. The caller can try figure out what to do with the changeset if persisting to db fails.

Notes to the reviewers

I added a take convenience method to the Append trait. I thought it would be handy for the caller to do a staging area with this.

Changelog notice

  • Remove persist submodule from bdk_chain.
  • Change Wallet to outsource it's persistence logic by introducing Wallet::take_staged.
  • Add take convenience method to Append trait.

Checklists

All Submissions:

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

New Features:

* [ ] I've added tests for the new feature

  • I've added docs for the new feature

@evanlinjin
feat!: rm persist submodule
1eca568 
Remove `PersistBackend`, `PersistBackendAsync`, `StageExt` and
`StageExtAsync`. Remove `async` feature flag and dependency. Update
examples and wallet.
@evanlinjin
feat(chain)!: add take convenience method to Append trait
feb27df 
This is useful if the caller wishes to use the type as a staging area.

This is breaking as `Append` has a `Default` bound now.
@evanlinjin evanlinjin marked this pull request as ready for review June 14, 2024 16:53
@evanlinjin evanlinjin self-assigned this Jun 14, 2024
@evanlinjin evanlinjin added the api A breaking API change label Jun 14, 2024
@evanlinjin evanlinjin added this to the 1.0.0-alpha milestone Jun 14, 2024
ValuedMammal
ValuedMammal reviewed Jun 14, 2024
View reviewed changes
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

Approach ACK

crates/chain/src/changeset.rs Outdated Show resolved Hide resolved
crates/wallet/src/wallet/mod.rs
bdk_chain::persist::StageExtAsync::commit_to(&mut self.stage, persist_backend).await?;
Ok(committed.is_some())
/// Get a reference of the staged [`ChangeSet`] that are yet to be committed (if any).
pub fn staged(&self) -> Option<&ChangeSet> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the previous implementation of this was fine too.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean without the option?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I was worried that people will try persist using this and then call take_staged only after saving to db succeeded.

notmandatory
notmandatory approved these changes Jun 14, 2024
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK a0bf45b

crates/chain/src/tx_data_traits.rs
@@ -114,12 +114,21 @@ pub trait AnchorFromBlockPosition: Anchor {
}

/// Trait that makes an object appendable.
pub trait Append {
pub trait Append: Default {
Copy link
Member

@notmandatory notmandatory Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

👍 This has the added benefit that it should solve #1103 by requiring all change set fields implement Default. So if we load a wallet data file from an older bdk version we should gracefully handle fields added in newer bdk versions since they must have a reasonable default value.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't really solve #1033. Backwards compatibility is only important for wallet, and we need the wallet's changeset to impl default already. Solving #1033 requires the serialized changeset to be deserializable with newer versions

crates/chain/src/changeset.rs Outdated Show resolved Hide resolved
@notmandatory
Copy link
Member

I pushed a commit to fix some docs that still mentioned PersistBackend and updated the bdk_wallet README example.

@notmandatory notmandatory mentioned this pull request Jun 14, 2024
Closed
31 tasks
evanlinjin
evanlinjin commented Jun 15, 2024
View reviewed changes
Copy link
Member Author

@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.

self-ACK a0bf45b

@notmandatory notmandatory merged commit 410ba17 into bitcoindevkit:master Jun 15, 2024
12 checks passed
@ValuedMammal ValuedMammal mentioned this pull request Jun 17, 2024
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

@ValuedMammal ValuedMammal ValuedMammal left review comments

@notmandatory notmandatory notmandatory approved these changes

@LLFourn LLFourn Awaiting requested review from LLFourn

Assignees

@evanlinjin evanlinjin

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

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