Refactor conversations by kaichaosun · Pull Request #40 · logos-messaging/libchat

kaichaosun · 2026-02-03T06:59:07Z

What changes:

renaming traits related to ConvFactory
move shared traits to common submodule
inbox, dm(direct message), group split
fix dead code
isolate FFI into submodule
restructure prost related bytes conversion

jazzz

It's hard to understand the motivation behind these changes, and the reason why to change it now.

I can see that you are focusing on better organization of the project, which is welcome as the project could use improvements - However I don't grok why this path is preferred.

Clearly articulating the motivations would allow discussions to occur.

Sticky points:

Conversation != Session, they are not substitutable.
Trait Renaming doesn't accurately describe what is happening.
Folder structure logic, I don't understand the vision.

jazzz · 2026-02-03T15:59:00Z

conversations/src/dm/privatev1.rs

+impl HasConversationId for PrivateV1Convo {
+    fn id(&self) -> SessionId<'_> {


Session is not the correct term to use here.

Session refers to a double-ratchet state shared between two participants.

Conversation is an abstract way to communicate between clients.

Yeah, session seems not fit, I may find a new name, feel free to recomment new ones.
Conversation seems long, a short one would be good.

I may find a new name, feel free to recomment new ones.

Unfortunately Conversation is the best I have 😄 - which is why its used everywhere.

Conversation seems long, a short one would be good.

I shortened it to Convo as i figured the team would want something shorter 😅

jazzz · 2026-02-03T16:21:50Z

conversations/src/dm/mod.rs

@@ -0,0 +1 @@
+pub mod privatev1;


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.

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?

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.

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,

refactor structure, isolate ffi, clear code and naming

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).

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.)

jazzz · 2026-02-03T17:11:25Z

conversations/src/inbox/handshake.rs

 use rand_core::{CryptoRng, RngCore};

-use crate::crypto::{PublicKey, StaticSecret};
+use crate::identity::{PublicKey, StaticSecret};


[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.

jazzz · 2026-02-03T17:19:11Z

conversations/src/inbox/inbox.rs

        payload: proto::EncryptedPayload,
    ) -> Result<(SecretKey, proto::InboxV1Frame), ChatError> {
        let handshake = Self::extract_payload(payload)?;
+        let payload_bytes = handshake.payload.clone();


Why clone here, instead of consuming the slice directly?

jazzz · 2026-02-03T17:21:19Z

conversations/src/inbox/inbox.rs

-        enc_payload: proto::InboxHandshakeV1,
-    ) -> Result<proto::InboxV1Frame, ChatError> {
-        let frame_bytes = enc_payload.payload;
+    fn decrypt_frame(payload_bytes: prost::bytes::Bytes) -> Result<proto::InboxV1Frame, ChatError> {


This change, weakens the type constraint on the function - Making auto complete more difficult and adding new failure modes to this function.

Whats the motivation?

The function is not used before, I'm trying to use it in some way, so it don't get deleted or become dangling code.

so it don't get deleted

Then I would delete it. I don't see much harm in that. Changing the signature to accept a more generic type (Bytes)loses context which is more harmful.

Follow up here. I looked into this when solving another problem and realized I had missed the intention here.

I now see that the type constraint cannot be maintained easily. Changing the decrypt_frame function to accept Bytes was a good move in my opinion.

jazzz · 2026-02-03T18:50:05Z

conversations/src/types.rs


 // Used by Conversations to attach addresses to outbound encrypted payloads
-pub(crate) struct AddressedEncryptedPayload {
+pub struct AddressedEncryptedPayload {


I don't understand why its required to expose an exclusively internal type outside of the crate?

The compiler complains about it being internal public after change a few modules to be public.

jazzz · 2026-02-03T18:53:09Z

conversations/src/lib.rs

+pub mod common;
+pub mod dm;
+pub mod ffi;
+pub mod group;
+pub mod inbox;
+


[Boulder] Why expose internal implementation details? This will make maintenance of the library more difficult. I don't understand why this would be desirable.

This is trying to solve unused code without adding #![allow(dead_code)] everywhere.

I agree it's not the best solution, since we don't fully understand the dev experience yet (used as a Rust library), this may also help bring some discussion about the internal design of logos-chat.

The library is early, broken changes is not problem in my opinion.

This is trying to solve unused code without adding #![allow(dead_code)] everywhere.

The deadcode is there because core functionality is not yet completed. Which needs to be done in the next 2 weeks. If thats too long, then I'd say remove all the offending code completely.

conversations/src/common.rs

jazzz · 2026-02-03T19:14:27Z

conversations/src/common.rs

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

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.

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.

jazzz · 2026-02-03T19:24:50Z

conversations/src/dm/privatev1.rs

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".

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

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.

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

kaichaosun · 2026-02-04T01:35:48Z

It's hard to understand the motivation behind these changes, and the reason why to change it now.

by running the code before, there are clearly warnings in a lot of places. And I really want to see the codebase has very little even 0 warnings if possible.
after reading through the code, the FFI logic is taking focus from me to understand the functional business logic.
I need to jump around different places to understand how the a single business like privatev1 works.
want to adding storage directly, really hard to find the necessary fields to save.

So I think maybe the codebase needs such refactor to help other devs to run and reason about the code. The namings introduced in this PR is definitely not final or best, it's just a place to discuss and try make it more clear before more users reading on it.

It takes good energy to think about most items introduced in this PR, it may not reflect other's coding styling, but my motivation is to make the codebase as perfect as I can. BTW most items is handmade, AI helps with some namings and docs. (I don't trust AI on refactor for now)

… it.

jazzz · 2026-02-04T04:04:44Z

@kaichaosun

It takes good energy to think about most items introduced in this PR, it may not reflect other's coding styling, but my motivation is to make the codebase as perfect as I can.

I see that. Its very clear that is your goal - and I'm grateful thats where your mind is. I think there are good changes in here. But its hard to see.

by running the code before, there are clearly warnings in a lot of places. And I really want to see the codebase has very little even 0 warnings if possible.

I would suggest that could have been a dedicated PR - as the changes are narrow in scope. That would allow discussion and make it easier to find alignment.

after reading through the code, the FFI logic is taking focus from me to understand the functional business logic.

I understand its not great. That part is evolving quickly, and there are lots of changes. The API file will eventually need to be exported to a crate but its not finished yet. I'd like to get it working before we start moving things around. It's my current focus, perhaps we can ignore api.rs and a few types in context.rs for now? If there are blockers lets discuss.

I need to jump around different places to understand how the a single business like privatev1 works.

Perhaps we can book a Pairing-Session to go over this. I don't understand the exact friction - but would like to learn more.

want to adding storage directly, really hard to find the necessary fields to save.

Thats fair. I could see it being unclear. Especially since functionality is not complete.
I would focus on making smaller problems; Go protocol by protocol and ensure that they can survive a restart. Starting with PrivateV1 . Participant information is missing but that can be added after. If there are questions lets use the tracking ticket to have those discussions and gather the needed requirements.

kaichaosun · 2026-02-06T03:32:35Z

Replaced by other PRs.

kaichaosun added 5 commits February 3, 2026 12:39

chore: refactor conversations

6c54bbd

chore: use inbound and outbound for session

2a9fe1c

chore: fix dead code

572ae6d

chore: remove crypto for identity

2e48e49

chore: isolate ffi functions

0ca0bb9

kaichaosun requested review from jazzz and osmaczko February 3, 2026 07:00

jazzz requested changes Feb 3, 2026

View reviewed changes

chore: change signature of decrypt_frame back and allow dead code for…

cc6988d

… it.

kaichaosun added 2 commits February 4, 2026 12:17

chore: reanme session to chat

0ddfd3f

chore: merge main

a5943ae

kaichaosun closed this Feb 6, 2026

		impl HasConversationId for PrivateV1Convo {
		fn id(&self) -> SessionId<'_> {

Conversation

kaichaosun commented Feb 3, 2026

Uh oh!

jazzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jazzz Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jazzz Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaichaosun Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaichaosun Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaichaosun commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jazzz commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaichaosun commented Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jazzz Feb 4, 2026 •

edited

Loading

jazzz Feb 4, 2026 •

edited

Loading

kaichaosun Feb 4, 2026 •

edited

Loading

kaichaosun Feb 4, 2026 •

edited

Loading

kaichaosun commented Feb 4, 2026 •

edited

Loading

jazzz commented Feb 4, 2026 •

edited

Loading