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.

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

Add key-value-db and sqlite-db features, separate wallet directories #71

Merged
merged 4 commits into from
May 11, 2022

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Feb 18, 2022

Description

  • Add distinct key-value-db and sqlite-db features, keep default as key-value-db

  • Put cached wallet data in separate directories: ~/.bdk-bitcoin/<wallet_name>

  • Put compact filter data in <wallet_name>/compact_filters

  • Depending on the db used put cached wallet data in: <wallet_name>/wallet.sled/ or <wallet_name>/wallet.sqlite

Notes to the reviewers

This change will help test BDK with different databases, in particular for manually testing DB migrations such as in bitcoindevkit/bdk#502.

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
  • I've updated CHANGELOG.md

@notmandatory notmandatory force-pushed the sqlite branch 4 times, most recently from dfa8c2a to 2e38011 Compare February 20, 2022 00:21
@notmandatory notmandatory marked this pull request as ready for review February 20, 2022 00:35
@notmandatory
Copy link
Member Author

notmandatory commented Mar 4, 2022

@rajarshimaitra can you take a look at this one?

EDIT:
Never mind for now, I'm moving this back to draft so I can make the DB features additive per #75.

@notmandatory notmandatory marked this pull request as draft March 5, 2022 02:54
@rajarshimaitra rajarshimaitra linked an issue Mar 7, 2022 that may be closed by this pull request
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Review ACK 2e38011

Looks good for now.. Will review again when additive features stuff is done..

src/bdk_cli.rs Show resolved Hide resolved
@notmandatory notmandatory self-assigned this Mar 14, 2022
@notmandatory notmandatory mentioned this pull request Apr 27, 2022
@notmandatory notmandatory marked this pull request as ready for review May 6, 2022 23:07
@notmandatory
Copy link
Member Author

notmandatory commented May 6, 2022

@rajarshimaitra I changed my mind again on the additive features thing. I still think it'd be good to try but will be a biggish change (if it's even possible) for bdk-cli and probably not worth the effort right now.

This PR is now rebased and should be ready to go.

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Ack 00454be

With just one question..

Comment on lines +126 to +130
#[cfg(not(feature = "sqlite-db"))]
if !db_dir.exists() {
info!("Creating database directory {}", db_dir.as_path().display());
fs::create_dir(&db_dir).map_err(|e| Error::Generic(e.to_string()))?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not understand the reason of feature guard here. For both sled and sqlite we would want to check if the dir exists right??

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason is that sqlite creates only a single file and doesn't need a directory to exist to put it's files in.

@notmandatory notmandatory merged commit d5155a5 into bitcoindevkit:master May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add optional features for wallet data
2 participants