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

Switch the FFI bindings to use the SQLite cryptostore #1711

Merged
merged 2 commits into from Mar 29, 2023

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Mar 29, 2023

Since we have no migration from Sled to SQLite we'll have to log our EX users out. cc @stefanceriu.

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (8b5de47) 76.14% compared to head (8597d06) 76.14%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1711   +/-   ##
=======================================
  Coverage   76.14%   76.14%           
=======================================
  Files         137      137           
  Lines       15222    15222           
=======================================
  Hits        11591    11591           
  Misses       3631     3631           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

🙌


let state_store = state_store.build()?;

let crypto_store = RUNTIME.block_on(matrix_sdk_sqlite::SqliteCryptoStore::open(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this method should be called something along the lines of configureWithPath if it does both creation and opening

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say creation, you mean creation of the DB or the SqliteCryptoStore object?

If the former, you can't create a SQLite DB without opening it. If the later what's the point of having the object if it's unable to do anything except call a build() or open() method.

Note that the open method is somewhat idiomatic in the SQLite context: https://docs.rs/rusqlite/latest/rusqlite/struct.Connection.html#method.open.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the open method is somewhat idiomatic in the SQLite context

Right okay, that explains it


let state_store = state_store.build()?;

let crypto_store = RUNTIME.block_on(matrix_sdk_sqlite::SqliteCryptoStore::open(
Copy link
Contributor

Choose a reason for hiding this comment

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

But also, why does the sled one have a builder and this one doesn't? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The state store is a bit more complex, i.e. you have the ability to expand the state store into a crypto store which uses the same underlying database.

In any case, once we have a SQLite state store as well, we'll add the ability to configure the SQLite store on the client builder and you won't have to access this low level interface.

@poljar poljar merged commit 03aba95 into main Mar 29, 2023
47 checks passed
@poljar poljar deleted the poljar/ex-sqlite-cryptostore branch March 29, 2023 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants