-
Notifications
You must be signed in to change notification settings - Fork 16
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
Reply content type proposal #22
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,60 @@ | ||||||
--- | ||||||
xip: | ||||||
title: Reply Content Type | ||||||
description: Content type for reply messages | ||||||
author: Kunal Mondal | ||||||
status: Draft | ||||||
type: Standards Track | ||||||
category: XRC | ||||||
created: 2023-03-24 | ||||||
--- | ||||||
|
||||||
## Abstract | ||||||
|
||||||
This XRC proposes a new content type for messages that enriches the messaging experience. It attempts to include only the bare minimum for clients to determine how to display such messages in order to provide flexibility for more rich types in the future. | ||||||
|
||||||
## Motivation | ||||||
|
||||||
When it comes to having a comfortable conversation, replying to messages is an absolute no brainer as it gives users the freedom to converse without any miscommunications. In order to provide users a real messaging platform, XMTP needs to provide this capability. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I buy this argument. XMTP conversations provide a way to order messages in natural response patterns. A message in a conversation can be presumed to be a response to messages preceding it in a conversation. I'd grant that this is somewhat lose structure and that there may be cases where a more refined substructure would be useful, is that what you're after with this proposal? (threads within conversations?) Could you elaborate more on what you're trying to achieve with this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although XMTP conversations provide a way to order messages in a natural response pattern and the response to How are we suposed to respond to this ? It is a natural instinct to reply to the newest message first so here Introducing the functionality to reply to particular messages solves this and lets users freely respond in any There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Threads within conversations can be a good option but it doesn't seem to have much use case when it comes to 1 on 1 conversations. There are just 2 people in a chat conversing and why would they want to make a completely new thread to separate the conversation from the original one ? Although threads' refined substructure could be pretty great for a group chat as there's substantial use case. In the future, when group chats are introduced to XMTP, thread conversations is definitely a go to feature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are great motivational examples. I think this is the right content for the Motivation section. |
||||||
|
||||||
## Specification | ||||||
|
||||||
Proposed content type: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
```js | ||||||
{ | ||||||
authorityId: "xmtp.org" | ||||||
typeId: "reply" | ||||||
versionMajor: 0 | ||||||
versionMinor: 1 | ||||||
} | ||||||
``` | ||||||
|
||||||
The reply message MUST include the following parameters: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is sufficient. How does a codec decode this message if its full structure is not known? How do you decode something if all you know about it is that it should have a string field in it, but the rest is unknown? The reference implementation suggests that you intend a Reply to be a wrapper around a nested EncodedContent structure. If so, it needs to be explicitly shown here. Also if the intended encoding format is protobuf, the spec should include the protobuf spec of the structures. The spec must spell out in complete detail how things are expected to be encoded. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apparently I misunderstood that |
||||||
|
||||||
```ts | ||||||
{ | ||||||
// The ID of the message that is being replied to | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the ID of the message? Where does it come from. Are implementors of this XIP free to chose what they want to use as an ID? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Here the ID refers's to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming you're referring to https://github.com/xmtp/proto/blob/main/proto/message_contents/message.proto#L63. Unfortunately the protocol spec doesn't define the value. I think we'll need to rectify that for this reference to work. If different SDKs compute the ID differently (which likely isn't the case today, but it could be), then the reference will be broken in different contexts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And again, I'm asking the questions because the XIP should answer them. |
||||||
inReplyToID: string | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the message also contain the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apparently what I missed earlier is that |
||||||
} | ||||||
``` | ||||||
|
||||||
The content of the encoded message is an EncodedContent object, serialized in the protobuf format (since we're guaranteed to always have the ability to deserialize that.) | ||||||
|
||||||
The content type of the reply's `EncodedContent` MUST be allowed. By default, the only allowed content type is `ContentTypeText`, but additional types can be optionally allowed as well. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the purpose of this allowed type notion. What does make a content type allowed? What is the process of making a type allowed? |
||||||
|
||||||
## Backward compatibility | ||||||
|
||||||
Clients encountering messages of this type must already be able to deal with messages of an unknown content type, so whatever considerations they're making there should work here too. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It sounds like there are no backward compatibility concerns with this proposal, then let's just say that with the fewest words possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should strongly encourage users of the Reply content type to use The codecs can help a bit here. If the child content has a fallback, we could bubble that up to make it the fallback of the parent. That way even if your client doesn't support Replies, you can still read the message. If the child content is of type text, we could also make that text the fallback of the parent. |
||||||
|
||||||
## Reference implementation | ||||||
|
||||||
- [PR implementation for the JS SDK](https://github.com/xmtp/xmtp-js-content-types/blob/baed02476cd94c6ceef24c107a86a162efaec678/reply/src/Reply.ts) | ||||||
|
||||||
## Security considerations | ||||||
|
||||||
Having different message content structure in content types breaks the uniformity which might be risky, but this is also the case for other content types, since there's no server side validation of message contents (besides size). The same protections we have now would be in place while the same pitfalls we have would still be there as well. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand what you're trying to say here. Content types is a mechanism that allows different content structures in messages. What risks are you referring to? |
||||||
|
||||||
## Copyright | ||||||
|
||||||
Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/). |
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.
Abstract should summarize what is being proposed (https://github.com/xmtp/XIPs/blob/main/XIPs/xip-0-purpose-process.md#what-belongs-in-a-successful-xip). I don't think this paragraph does that. I'd expect to see something like
This XRC proposes a reply content type that can be used to ...
. Most of what's in this paragraph can be deleted without noticeable loss of information.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.
Yeah, we have changed it.
951c0fe