Skip to content

Conversation

@notmandatory
Copy link
Member

In addition to adding sqlite support I also added some instructions in the README for how to run the tests locally.

This is mainly a copy/paste of the postgres module with minor changes required to work with sqlite. At some point we could probably remove some of the duplication. But for now this works well enough for me to use in my TABConf workshop. 🙂

@notmandatory notmandatory added the enhancement New feature or request label Oct 3, 2024
@notmandatory notmandatory self-assigned this Oct 3, 2024
@notmandatory notmandatory force-pushed the add_sqlite_module branch 2 times, most recently from 4910d66 to bfbd51f Compare October 5, 2024 02:42
@notmandatory
Copy link
Member Author

notmandatory commented Oct 5, 2024

I changed all the tests to 1. run with both the postgres and sqlite stores, 2. drop the postgres tables at the start of each test instead of the end, this makes it possible to debug what is in the DB if a test fails, 3. removed duplicate store setup code and put it all in a new create_test_stores().

@notmandatory notmandatory force-pushed the add_sqlite_module branch 2 times, most recently from 49afea1 to aff27a9 Compare October 5, 2024 13:39
@ValuedMammal
Copy link
Collaborator

The postgres example will be easier to run if we have an [[example]] section in Cargo.toml and just have a file examples/bdk_sqlx_postgres.rs without the extra Cargo.toml. Or we can turn the project into a workspace with multiple crates like the bdk repo but that will be more work. Right now the example looks like a crate inside of a crate which is causing rust-analyzer to complain that it's an "unlinked file"

@notmandatory
Copy link
Member Author

@ValuedMammal thanks for the [[example]] suggestion, I should have looked up the right way to do it, fixed.

Copy link
Collaborator

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

This looks good to me, all tests passing. Not sure if there's any remaining comments from @matthiasdebernardini before merging

Comment on lines +487 to +489
/// Collects information on all the wallets in the database and dumps it to stdout.
#[tracing::instrument]
pub async fn easy_backup(db: Pool<Sqlite>) -> Result<(), BdkSqlxError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't find an easy way to call this from outside the library, I guess because the module is private?

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 we should remove these functions since they only print to stdout they belong in an example or new function on Store that returns list of wallet names. But can do that in a separate PR this one is already pretty big.

Copy link
Collaborator

@matthiasdebernardini matthiasdebernardini left a comment

Choose a reason for hiding this comment

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

Do we want to test trying to store a private key and make sure it only stores the public key? If not let's get this merged in, I still need to figure out how to make the migrations run without an external sql file. I also need to look into schemas so that these tables are segregated from the rest of the application.

@notmandatory
Copy link
Member Author

I don't think we need any new tests in this crate to confirm only pubkey descriptors are stored, those belong in the wallet persister tests.

@notmandatory notmandatory merged commit e6ca323 into bitcoindevkit:master Oct 22, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants