Move double ratchet storage operations to PrivateV1#82
Conversation
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
Overall Love the direction here. Conversations becoming more autonomous.
Few comments around further clean up, and simplification.
Approved assuming the Justfile Pebble is cleaned up.
| } | ||
|
|
||
| impl PrivateV1Convo { | ||
| impl<S: ConversationStore + RatchetStore> PrivateV1Convo<S> { |
There was a problem hiding this comment.
[Dust] Creating an empty supertrait bound, would make the code easier to follow along, and easier to maintain.
There was a problem hiding this comment.
I have think about introduce a new trait to wrap the two traits here, but it would introduce too many traits since there will be other convo, and some of the traits is likely to be changed in near future.
| pub fn new_responder( | ||
| seed_key: SymmetricKey32, | ||
| dh_self: &PrivateKey, | ||
| store: Rc<RefCell<S>>, | ||
| ) -> Self { |
There was a problem hiding this comment.
[Sand] store should come before the other parameters.
Parameters should be ordered most-stable to least-stable. Such as:
self — the actor
context/deps — what the actor needs to function (an allocator, a fmt handle) — rarely changes between calls
the main data — the thing being transformed or inspected
options/flags — tweaks to the operation
output buffer — where to write results, if not returned directly
| let convo_id = self.persist_convo(&convo)?; | ||
| let convo_id = convo.persist()?; |
There was a problem hiding this comment.
If conversations know how to persist themselves, whats the advantage to calling persist externally from the context? Seems like its separating storage responsibilities across two different layers.
There was a problem hiding this comment.
Specifically, this would call persist regardless of whether the state needs to be updated. Removing persist from Conversation Trait would mean that clients didn't manage internal state at all.
There was a problem hiding this comment.
Looks like a good idea, will refactor it later to keep this RP small.
There is a lot of discussion spaces around here, like clean up state when error happens, and how to resume for error scenarios, etc. Such requirements will be much clear when MVPs start releasing.
| /// Loads a conversation from DB by constructing it from metadata + ratchet state. | ||
| fn load_convo(&self, convo_id: ConversationId) -> Result<Conversation, ChatError> { | ||
| /// Loads a conversation from DB by constructing it from metadata. | ||
| fn load_convo(&self, convo_id: ConversationId) -> Result<Conversation<T>, ChatError> { |
There was a problem hiding this comment.
[Dust] T implies that this Conversation has is an operand type. Better off choosing, S for Store or Store for Store, as there will be more Generic parameters.
|
|
||
| justfile |
There was a problem hiding this comment.
[Pebble] Can you exclude this instead. This project may have justfiles in the future which would cause a conflict.
There was a problem hiding this comment.
If the project include justfile in the future, remove it then.
I have been using justfile for this project heavily, not ignore it makes the git commit flow really bad.
There was a problem hiding this comment.
not ignore it makes the git commit flow really bad.
I asked if you could use git exlcude instead.
kaichaosun
force-pushed
the
private-store
branch
from
25dd475 to
54eb8ed
Compare
2 participants
Why this changes?
Based on the previous discussion, we want to put the store operations inside each module out of context, as each module has more knowledge on how to deal with storage.
What changes:
PrivateV1