Split storage crate to abstract database layer#30
Conversation
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
I'm starting to see where you are going with this. The changes to Dr::storage look better. I see work towards supporting rollbacks, and managing state transitions. I'm still concerned with this direction, but understand its an exploratory phase.
--
The Storage crate seems pre-mature and doesn't add value IMO.
- I'd be favouring modules over crates until defined interfaces are made.
- The storage crate provides no functionality and only acts as a wrapping to sqlite, could be a module.
- I'd invert the trait direction. Currently this code wants to define external traits like StorageBackend which are then used/consumed in
double-ratchet, instead havedouble-ratchetdefine what it needs and allow callers to choose how to provide that.
| // Re-export rusqlite types that domain crates will need | ||
| pub use rusqlite::{Error as RusqliteError, Transaction, params}; |
There was a problem hiding this comment.
I don't understand the utility of this crate.
The intention appears to be to wrap SQLite and isolate dependencies. However this crate re-exposes dependencies which creates tight coupling.
There was a problem hiding this comment.
The storage crates currently serves as configuration of sqlite, and it can be reused by other crates like inbox or conversations.
Over engineered traits are deleted now.
For now, I can't fully extract rusqlite out from DR crate, tried the idea mentioned here #30 (comment), but seems not perfect either on quick glance, will look into it more later.
| storage: &'a mut SqliteStorage, | ||
| conversation_id: impl Into<String>, | ||
| storage: &'a mut RatchetStorage, | ||
| conversation_id: &str, |
There was a problem hiding this comment.
Love this, much easier to managed at the callsite
| /// Provides rollback semantics - state is only saved if the operation succeeds. | ||
| pub struct RatchetSession<'a, D: HkdfInfo + Clone> { | ||
| storage: &'a mut SqliteStorage, | ||
| storage: &'a mut RatchetStorage, |
There was a problem hiding this comment.
I think this pattern works.
RatchetSession is {State+Persistence}, I think that is clean.
[SAND] In the future I think RatchetStorage should become a trait, which would allow callers to implement which ever storage process they'd like.
That would mean:
- modules define its storage requirements via a trait
- Callers provide an impl via generics.
- modules are then agnostic of the storage being used
There was a problem hiding this comment.
I really like the idea, I also see some challenges on the transactional storage requirement and ratchet data highly coupled with storage record. Will continue exploring in this direction.
|
|
||
| [workspace.dependencies] | ||
| blake2 = "0.10" | ||
| storage = { path = "storage" } |
There was a problem hiding this comment.
[Dust] As a practice crate dependancies should have 2 or more consumers
storage/src/sqlite.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl StorageBackend for SqliteDb { |
There was a problem hiding this comment.
[Sand] This trait has low utility. I can see the intention of decoupling implementations from a specific sql implementation.
However I don't think its the best abstract level. Having Stateful objects define traits for persistence which are then implemented by a storage provider would provide more flexibility and separation of concerns.
pub trait DrKeyStore {
fn get_message_key(..);
fn add_message_key(..);
fn remove_message_key(..);
}
pub struct DrSession<K: DrKeyStore> {
storage: &'a mut K,
}
impl DrSession {
pub fn encrypt_message(&mut self, ..) -> Result<..> {
...
self.storage.get_message_key();
...
}
}
// ---------------------------------
impl DrKeyStore for SqliteStore {}
impl DrKeyStore for EphemeralStore {}
impl DrKeyStore for PostgresStore {}
impl
Rather than making DR agnostic to which SQL database is being provided, make DR agnostic to how data is stored all together. Modules should focus on how data needs to be used, defining it's needs.
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
I can see this taking form
| let mut alice_storage = RatchetStorage::new(alice_db_path, encryption_key) | ||
| .expect("Failed to create alice encrypted storage"); | ||
| let mut bob_storage = RatchetStorage::new(bob_db_path, encryption_key) | ||
| .expect("Failed to create bob encrypted storage"); |
There was a problem hiding this comment.
I like how much cleaner this got.
2 participants
What have changed: