Include sender information on missing messages#120
Conversation
|
|
||
| ReliablePayload { | ||
| message_id, | ||
| message_id: encoded_info, |
There was a problem hiding this comment.
In previous discussion, we decided to reuse the message_id in ReliablePayload protobuf for sender info.
Future improvements includes wire format naming and frontiers calculations is planned later.
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
Change looks good.
My comments are all future considerations.
| use crate::utils::{blake2b_hex, hash_size}; | ||
|
|
||
| /// `hash_size::MessageId` is U32 (32 bytes) → 64 hex chars. | ||
| const MESSAGE_ID_HEX_LEN: usize = 64; |
There was a problem hiding this comment.
[Sand] As hash_size are defined at compile time, you can derive constants from them.
use blake2::digest::typenum::Unsigned;
const MESSAGE_ID_HEX_LEN: usize = 2 * <hash_size::MessageId as hash_size::HashLen>::Size::USIZE;
Deriving it would allow it to be automatically updated should the value change in the future. Thought its not the prettyiest syntax currently. Your call.
|
|
||
| /// Render to the wire-form string used by the `message_id` proto field. | ||
| pub fn encode(&self) -> String { | ||
| self.to_string() |
There was a problem hiding this comment.
[Dust] Encoding as a String seem unnecessarily wasteful. Bytes would be more compact on the wire.
There was a problem hiding this comment.
I try to make it consistent with nim-sds. Worth to make a change later with more communication happened.
I do notice that nim-sds add sender id recently, this is protobuf updated: logos-messaging/chat_proto#9, also updates changes here to use new wire format.
| pub fn decode(s: &str) -> Result<Self, ChatError> { | ||
| if s.len() < MESSAGE_ID_HEX_LEN { | ||
| return Err(ChatError::BadParsing("MessageId")); | ||
| } | ||
| let split = s.len() - MESSAGE_ID_HEX_LEN; | ||
| Ok(Self { | ||
| sender_id: s[..split].to_owned(), | ||
| message_id: s[split..].to_owned(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
[Sand] Prefer self describing memory layouts, when possible.
This is more of a design principal - but helpful to keep in mind.
This code as written is tightly coupled to the code that produced it - changing MESSAGE_ID_HEX_LEN would be break interop between clients.
Three improved design choices in order of robustness:
- Deterministic parsing via TLV, such that any recipient can parse the values regardless of how they were generated.
- Use a defined delimiter such as '|' or ':'. Decouples from payload from size changes but requires consistent order.
- Use a single byte
Sigilto define which parsing format should be used.
Delimiter seems like the right trade off here, given the string implementation and the narrowed scope.
There was a problem hiding this comment.
Not needed in new wire format.
| let frontier = Frontier::new(sender.to_string(), message_id); | ||
|
|
||
| let causal_history = state | ||
| .frontier | ||
| .frontiers | ||
| .iter() | ||
| .cloned() | ||
| .map(|message_id| HistoryEntry { | ||
| message_id, | ||
| .map(|f| HistoryEntry { | ||
| message_id: f.encode(), | ||
| retrieval_hint: Bytes::new(), | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
[Dust] consider flattening Frontier to a String with dedicated encode and decode functions.
I love adding types especially when:
- Types represent a state transition through a pipeline. TypeState pattern.
- Theres complicated functionality being provided
- A Type is expected to grow to take on new behavior.
In this case it seems like we may just want to define causual_history::message_id as a string, which is a combination of the sender_id and derived_message_id.
Creating a type that is only used to concat two strings together, distracts from what is really happening.
There was a problem hiding this comment.
Specifically 'fn derived_message_id` already has access to SenderId and could handle the concat in one step.
There was a problem hiding this comment.
Looks like the Frontier struct has more meaning after the new wire format.
derived_message_id is isolated as we may want to introduce a consistent reusable function to calculate the identifier going through libchat.
| reported_missing: HashSet<String>, | ||
| reported_missing: HashSet<Frontier>, |
There was a problem hiding this comment.
[Dust] I like the removal of naked String types from the code base.
If you decided to flatten Frontier, consider using a type def or NewType wrapper (depending in your vision) to provide the same Type safety/Readability.
3 participants
In previous PR #105, we implemented causal history for detecting missing message.
This PR introduce extra
sender_idto detect who the missing messages belongs to.Fix #98