-
Notifications
You must be signed in to change notification settings - Fork 11
Add sqlite module #11
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 sqlite module #11
Conversation
4910d66 to
bfbd51f
Compare
|
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 |
49afea1 to
aff27a9
Compare
|
The postgres example will be easier to run if we have an |
73cb9b5 to
e624074
Compare
|
@ValuedMammal thanks for the |
f26e056 to
cb0958c
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.
This looks good to me, all tests passing. Not sure if there's any remaining comments from @matthiasdebernardini before merging
| /// 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> { |
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 didn't find an easy way to call this from outside the library, I guess because the module is private?
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 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.
cb0958c to
7388cfe
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.
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.
|
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. |
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. 🙂