-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add referencing_message_id to EncodedContent #65
Conversation
I'd argue that replies ought not be implemented via a reference, but rather via a content copy. Most modern messaging applications (Tested in Signal + Whatsapp) implement replies by creating a copy of the content. benefits:
For performance the number of bytes copied could be capped at X bytes for text input. |
How can we ensure the copied content is actually real? Or do we not super care about that? I could see malicious clients just ending along replies that make it look like something was said that wasn't. |
@nakajima It helps to look at the attack vector. If your app is compromised, then the app could show you whatever it wanted. It is a prerequisite that you trust your own Application. If the senders app is compromised, and have the capability to send messages then they can send fake messages and the send a reply to it if they desired. In either case, a malicious App would have catastrophic effects independently of replies. |
Signals (Newish) Threaded replies is a good example of design balance here. The contents are copied AND a reference is included such that forward links could be built into the UI. Giving applications/sdk the ability to validate the messages if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me, going to defer on ultimate approval power though
@@ -41,6 +41,8 @@ message EncodedContent { | |||
optional Compression compression = 5; | |||
// encoded content itself | |||
bytes content = 4; | |||
// optional ID of a message that this message is referencing | |||
optional string referencing_message_id = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall feels okay to me, but admittedly without thinking too hard about any far-reaching consequences. If referencing_message_id
is the sha256 digest of the referenced message envelope, this seems pretty safe. It's effectively a handle to aid in looking up the referenced message which should be very difficult to predict or forge (envelope contents include randomly generated salt, etc).
For tactical use seems totally fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my sake, might be good to reiterate in comment what the message id is e.g. "sha256 of envelope message as unencoded bytes"
added question: which proto field is being fed into sha256 to get the id again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would advocate that Replies and Reactji be handled differently.
- Reactji messages are normally not rendered as independent messages to the user.
- Replies are rendered and should contain all the information needed to render them with an additional message reference to support advanced UIs
Since the messages are rendered differently multiplexing the types would add confusion
Sorry, flubbed the button there. Disregard the PR closing action :/ |
I think that if we want to include a copy of the content with replies, that'd be a good case for a new content type (maybe a composite content type where one part is the original message copy and another part is the reply message itself?). It'd still be nice to include a For reactions, I also think a new content type would be good to let clients make better decisions about display/rendering. The matter of which message they're reacting to still seems like a more core concept that's more broadly applicable. So basically I agree that this isn't enough on its own to implement those things, but I think it'd be very useful to both at the same time. |
I think the part I'm missing is why this needs to be in the Changing the Why does the field need to exist at all for |
While @mkobetic and I were discussing replies earlier, we were leaning away from the idea of copying the content over, so the Now that we're talking more about including the original message content along with these messages, I'm feeling less than 100% about it, though I do still think there's something to the idea of reifying relationships between messages. Maybe we go back to seeing what replies/reactions look like as composite types? |
I don't know much what other chat apps do, but content copy, especially partial, seems too ambiguous to link a message to a specific preceding message, which is ultimately what this proposal is about.
My argument was that the notion of linking to a previous message seems to apply universally and not to specific content types, and thus posed the question whether this notion belongs to the protocol level instead. xmtp/XIPs#22 (review)
I thought that adding a field is backwards compatible in protobuf, isn't it? I assumed older apps would simply skip that field and not get the backward reference. https://protobuf.dev/programming-guides/proto2/#updating
I would assume the same thing as any other content type, e.g. thread under the original message or something. |
Great point @mkobetic, there may be some ambiguity on what the task is here. Re-reading the messages I see two different interpretations:
The difference being that a threaded message can stand on its own(Slack shows individual messages in notifications for example), where a quoted message cannot. My previous comments are targeted towards achieving 1. Which of the options is the intended feature? |
I'm wondering how much flexibility in the interpretation/usage of the back-linking could be left to the apps without creating semantic confusion. Maybe we don't have to say that this is how to solve replies, especially not the "quote" type that @jazzz described. Threading could be an example usage, but maybe we don't have to restrict to just that? Is it asking for trouble down the line? |
There have recently been a couple XIPs opened for content types that need to reference other messages:
This PR adds a an optional new field to
EncodedContent
namedreferencing_message_id
. The value of this field should be theid
of aDecodedMessage
.To implement message replies, clients can just send a new message with whatever content types they support and include the original message's ID as the
referencing_message_id
.To implement message reactions, we could add a new content type that can focus on the actual behavior of reactions, including things like
added
,removed
etc, leaving the relationship between the content and the original message to this new field.I could also see a future where we could use
referencing_message_id
to implement message editing and deletion or starring/pinning.