Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions conversations/src/common.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see how "common" is an improvement. The crate already has a "utils", "types" files which are intended to hold components useful to many modules. Having all three makes it unclear of where code ought to live.

In this case, Common doesn't seem common at all. It seems only to pertain to Conversations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Common is shared by different types of conversations like inbox, dm and group chat.
The logos-chat (this crate) is about chat related protocols, and the major business logic is around inbox and dm, the common module is shared trait definitions depend by the major business.

The shared types may also fit into common, but it may grow and better in split module for now.

Copy link
Collaborator

@jazzz jazzz Feb 4, 2026

Choose a reason for hiding this comment

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

Previously this code was in a file conversation.rs and the conversation types were submodules that sub-modules of Conversation.

Moving this code out of the Conversations Module seems to be the opposite of logically grouping business logic. It was contained logically and now its not.

by different types of conversations like inbox

Inbox is not a Conversation - Its the component which initializes conversations. It consumed the conversation module just as the Client module does. Though I will admit that Inbox is not perfect there is much to clean up after persistence is implemented.

Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
use std::collections::HashMap;
use std::fmt::Debug;
use std::sync::Arc;

pub use crate::errors::ChatError;
use crate::types::{AddressedEncryptedPayload, ContentData};

pub type ChatId<'a> = &'a str;
pub type ChatIdOwned = Arc<str>;

pub trait HasChatId: Debug {
fn id(&self) -> ChatId<'_>;
}

pub trait InboundMessageHandler {
fn handle_frame(
&mut self,
encoded_payload: &[u8],
) -> Result<(Box<dyn Chat>, Vec<ContentData>), ChatError>;
}

pub trait Chat: HasChatId + Debug {
fn send_message(&mut self, content: &[u8])
-> Result<Vec<AddressedEncryptedPayload>, ChatError>;

fn remote_id(&self) -> String;
}

pub struct ChatStore {
chats: HashMap<Arc<str>, Box<dyn Chat>>,
handlers: HashMap<Arc<str>, Box<dyn InboundMessageHandler>>,
}

impl ChatStore {
pub fn new() -> Self {
Self {
chats: HashMap::new(),
handlers: HashMap::new(),
}
}

pub fn insert_chat(&mut self, conversation: impl Chat + HasChatId + 'static) -> ChatIdOwned {
let key: ChatIdOwned = Arc::from(conversation.id());
self.chats.insert(key.clone(), Box::new(conversation));
key
}

pub fn register_handler(
&mut self,
handler: impl InboundMessageHandler + HasChatId + 'static,
) -> ChatIdOwned {
let key: ChatIdOwned = Arc::from(handler.id());
self.handlers.insert(key.clone(), Box::new(handler));
key
}

pub fn get_chat(&self, id: ChatId) -> Option<&(dyn Chat + '_)> {
self.chats.get(id).map(|c| c.as_ref())
}

pub fn get_mut_chat(&mut self, id: &str) -> Option<&mut (dyn Chat + '_)> {
Some(self.chats.get_mut(id)?.as_mut())
}

pub fn get_handler(&mut self, id: ChatId) -> Option<&mut (dyn InboundMessageHandler + '_)> {
Some(self.handlers.get_mut(id)?.as_mut())
}

pub fn chat_ids(&self) -> impl Iterator<Item = ChatIdOwned> + '_ {
self.chats.keys().cloned()
}

pub fn handler_ids(&self) -> impl Iterator<Item = ChatIdOwned> + '_ {
self.handlers.keys().cloned()
}
}
42 changes: 7 additions & 35 deletions conversations/src/context.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{collections::HashMap, rc::Rc, sync::Arc};

use crate::{
conversation::{ConversationStore, Convo, Id},
common::{Chat, ChatStore, HasChatId},
errors::ChatError,
identity::Identity,
inbox::Inbox,
Expand All @@ -20,9 +20,8 @@ pub type ConvoHandle = u32;
// Ctx manages lifetimes of objects to process and generate payloads.
pub struct Context {
_identity: Rc<Identity>,
store: ConversationStore,
store: ChatStore,
inbox: Inbox,
buf_size: usize,
convo_handle_map: HashMap<u32, Arc<str>>,
next_convo_handle: ConvoHandle,
}
Expand All @@ -33,22 +32,13 @@ impl Context {
let inbox = Inbox::new(Rc::clone(&identity)); //
Self {
_identity: identity,
store: ConversationStore::new(),
store: ChatStore::new(),
inbox,
buf_size: 0,
convo_handle_map: HashMap::new(),
next_convo_handle: INITIAL_CONVO_HANDLE,
}
}

pub fn buffer_size(&self) -> usize {
self.buf_size
}

pub fn set_buffer_size(&mut self, size: usize) {
self.buf_size = size
}

pub fn create_private_convo(
&mut self,
remote_bundle: &Introduction,
Expand Down Expand Up @@ -99,43 +89,25 @@ impl Context {
Ok(Introduction::from(pkb).into())
}

fn add_convo(&mut self, convo: impl Convo + Id + 'static) -> ConvoHandle {
fn add_convo(&mut self, convo: impl Chat + HasChatId + 'static) -> ConvoHandle {
let handle = self.next_convo_handle;
self.next_convo_handle += 1;
let convo_id = self.store.insert_convo(convo);
let convo_id = self.store.insert_chat(convo);
self.convo_handle_map.insert(handle, convo_id);

handle
}

// Returns a mutable reference to a Convo for a given ConvoHandle
fn get_convo_mut(&mut self, handle: ConvoHandle) -> Result<&mut dyn Convo, ChatError> {
fn get_convo_mut(&mut self, handle: ConvoHandle) -> Result<&mut dyn Chat, ChatError> {
let convo_id = self
.convo_handle_map
.get(&handle)
.ok_or_else(|| ChatError::NoConvo(handle))?
.clone();

self.store
.get_mut(&convo_id)
.get_mut_chat(&convo_id)
.ok_or_else(|| ChatError::NoConvo(handle))
}
}

#[cfg(test)]

mod tests {
use super::*;
use crate::conversation::GroupTestConvo;

#[test]
fn convo_store_get() {
let mut store: ConversationStore = ConversationStore::new();

let new_convo = GroupTestConvo::new();
let convo_id = store.insert_convo(new_convo);

let convo = store.get_mut(&convo_id).ok_or_else(|| 0);
convo.unwrap();
}
}
83 changes: 0 additions & 83 deletions conversations/src/conversation.rs

This file was deleted.

33 changes: 0 additions & 33 deletions conversations/src/conversation/group_test.rs

This file was deleted.

17 changes: 0 additions & 17 deletions conversations/src/crypto.rs

This file was deleted.

1 change: 1 addition & 0 deletions conversations/src/dm/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub mod privatev1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed I don't think converting this crate to legacy style modules adds any value, and makes working in the project more difficult. I understand there is not agreement on this topic, but I don't see a need to force this change now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This crate clearly scope related logic in its own directory, without growing the files in src folder, and no need to just around to understand about the related logic.
The value is meaning in my opinion, do you mind switch to this branch in local and try it a bit, and see if you like it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

do you mind switch to this branch in local and try it a bit, and see if you like it?
I will gladly give it another try.
However this is not my first experience. I've worked in projects with both approaches. I think they both have pros - I don't deny that the approach you are suggesting has its merits.

I don't see a need to force this change now.

My main point is that this doesn't seem like a blocking item, it seems like personal preference. Given the current work load this seems low priority. There are many issues with imports and code structure which have larger impacts that we can agree on have benefit.

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 restructure helps assemble my mind, the reason has been discussed in different places.

And this is my overall plan to go through this journey,

  1. refactor structure, isolate ffi, clear code and naming
  2. refactor api include Context (I don't have clear idea how to do it, but looks like have to, because the convo_handle_map needs to be persisted somehow).
  3. then in this stage, we will have clear data model that's worth to be persisted. (Or maybe just persist the hash map with KV is also acceptable? But remember data migration is hard, and reasonable data model helps everyone in long journey.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the motivation on flattening the "Conversation" folder which contained conversations into these hyper-specific root level modules.

  • This exposes the root of the project to more folders. Will future folders include things like "broadcast", and "communities"?
  • This destroys the logical grouping of related components. Flattening to: "Group","storage", "dm", "broadcast", "communities", "ffi" seems harder to follow what is happening in the crate than "conversations", "storage", "ffi".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps the part bothering you is the crate being named "conversations"? Would renaming that to "context", "client", "chat" help at all?

Copy link
Contributor Author

@kaichaosun kaichaosun Feb 4, 2026

Choose a reason for hiding this comment

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

The confusing part is the unclear relationship between conversations and inbox, before make it clear, we should flatten the modules.

Also I don't think the library will grow enough to the need of have 3 layers directory like src/conversation/xyz, each layer needs clear abstraction, and we should first improve on the use of secondary one which is dm, inbox, and group for now. Even adding broadcast and communities, or a few others is not a problem, because most devs only need to care about the directory the need, and this also justifies the need to not depend on a inbox.rs (other module based file) in src.

Copy link
Contributor Author

@kaichaosun kaichaosun Feb 4, 2026

Choose a reason for hiding this comment

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

I like the idea to use chat to replace conversations. The flatten modules are still needed though.

Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ use std::fmt::Debug;
use x25519_dalek::PublicKey;

use crate::{
conversation::{ChatError, ConversationId, Convo, Id},
errors::EncryptionError,
common::{Chat, ChatId, HasChatId},
errors::{ChatError, EncryptionError},
proto,
types::AddressedEncryptedPayload,
utils::timestamp_millis,
Expand Down Expand Up @@ -91,14 +91,14 @@ impl PrivateV1Convo {
}
}

impl Id for PrivateV1Convo {
fn id(&self) -> ConversationId {
impl HasChatId for PrivateV1Convo {
fn id(&self) -> ChatId<'_> {
// TODO: implementation
"private_v1_convo_id"
}
}

impl Convo for PrivateV1Convo {
impl Chat for PrivateV1Convo {
fn send_message(
&mut self,
content: &[u8],
Expand Down
File renamed without changes.
1 change: 1 addition & 0 deletions conversations/src/ffi/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub mod api;
1 change: 1 addition & 0 deletions conversations/src/group/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// Group chat module
3 changes: 1 addition & 2 deletions conversations/src/identity.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use blake2::{Blake2b512, Digest};
use std::fmt;

use crate::crypto::{PublicKey, StaticSecret};
pub use x25519_dalek::{PublicKey, StaticSecret};

pub struct Identity {
secret: StaticSecret,
Expand Down
2 changes: 1 addition & 1 deletion conversations/src/inbox/handshake.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use blake2::{
use crypto::{DomainSeparator, PrekeyBundle, SecretKey, X3Handshake};
use rand_core::{CryptoRng, RngCore};

use crate::crypto::{PublicKey, StaticSecret};
use crate::identity::{PublicKey, StaticSecret};
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Sand] I don't have a blocker here - but Identity doesn't seem like the correct place to manage Keys.

crypto was a wrapper around the crate to isolate dependencies. If the wrapper is not desired, then the crypto crate should be imported directly.


type Blake2bMac256 = Blake2bMac<U32>;

Expand Down
Loading