Drop the in-memory database #613

2025-02-23T04:17:50Z

crodas commented

2025-02-23 04:17:50 +00:00

(Migrated from github.com)

Description

Fixes #607 (this PR depends on #620)

This PR drops the implementation of in-memory database traits.

They are useful for testing purposes since the tests should test our codebase and assume the database works as expected (although a follow-up PR should write a sanity test suite for all database trait implementors).

As complexity is worth with database requirements to simplify complexity and add more robustness, for instance, with the following plans to add support for transactions or buffered writes, it would become more complex and time-consuming to support a correct database trait. This PR drops the implementation and replaces it with a SQLite memory instance

Notes to the reviewers

Suggested CHANGELOG Updates

CHANGED

ADDED

REMOVED

FIXED

Checklist

I followed the code style guidelines
I ran just final-check before committing

### Description Fixes #607 (this PR depends on #620) This PR drops the implementation of in-memory database traits. They are useful for testing purposes since the tests should test our codebase and assume the database works as expected (although a follow-up PR should write a sanity test suite for all database trait implementors). As complexity is worth with database requirements to simplify complexity and add more robustness, for instance, with the following plans to add support for transactions or buffered writes, it would become more complex and time-consuming to support a correct database trait. This PR drops the implementation and replaces it with a SQLite memory instance ----- ### Notes to the reviewers  ----- ### Suggested [CHANGELOG](https://github.com/cashubtc/cdk/blob/main/CHANGELOG.md) Updates  #### CHANGED #### ADDED #### REMOVED #### FIXED ---- ### Checklist * [x] I followed the [code style guidelines](https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md) * [x] I ran `just final-check` before committing

thesimplekid (Migrated from github.com) reviewed

2025-02-23 15:50:49 +00:00

crates/cdk-integration-tests/Cargo.toml

Do you think it makes sense to add a memory db feature? Or should we just keep it as you have to bring in one of the cdk-sqlite or redb crates. And whether the sqlite is in mem or not can be handled by that crate. The feature maybe is a little similar for the end user but it creates two para dimes to accomplish the same thing one enabling the feature one bringing in the create.

Related: https://github.com/cashubtc/cdk/issues/511

Do you think it makes sense to add a memory db feature? Or should we just keep it as you have to bring in one of the cdk-sqlite or redb crates. And whether the sqlite is in mem or not can be handled by that crate. The feature maybe is a little similar for the end user but it creates two para dimes to accomplish the same thing one enabling the feature one bringing in the create. Related: https://github.com/cashubtc/cdk/issues/511

crodas (Migrated from github.com) reviewed

2025-02-23 23:10:27 +00:00

crates/cdk-integration-tests/Cargo.toml

This PR has already undergone several iterations, but in its final version, which has just two functions per DB implementation, I believe that moving those functions from cdk to the SQLite crate makes sense.

Regarding #511, that does make sense.

This PR has already undergone several iterations, but in its final version, which has just two functions per DB implementation, I believe that moving those functions from `cdk` to the SQLite crate makes sense. Regarding #511, that does make sense.

thesimplekid (Migrated from github.com) requested changes

2025-03-02 09:02:49 +00:00

thesimplekid (Migrated from github.com) left a comment

Just a small question think there was a feature flag left in from a rebase? Other then that lgtm

crates/cdk/src/wallet/README.md

I dont think we have this feature flag anymore?

I dont think we have this feature flag anymore? ```suggestion ```

crates/cdk/src/wallet/keysets.rs

					
				@ -90,2 +90,4 @@

				    #[instrument(skip(self))]

				    pub async fn get_active_mint_keyset(&self) -> Result<KeySetInfo, Error> {

				        // Important

				        let _ = self.get_mint_info().await?;