-
Notifications
You must be signed in to change notification settings - Fork 14
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
Reaction content type proposal #23
Conversation
Im-Kunal-13
commented
Mar 24, 2023
- Create xip-reaction-content-type.md
XIPs/xip-reaction-content-type.md
Outdated
```ts | ||
{ | ||
messageId: string | ||
senderAddress: string |
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'm not sure what value the senderAddress
field is adding here. All XMTP messages have some indication of who the sender is (headers for V1 messages and a signature for V2 messages). Given that any party who is able to send messages into the conversation could create a reaction, it would be possible for Party A to send a reaction and put Party B's address as the senderAddress
(or even some other random address).
Feels safer to just rely on the senderAddress
from the XMTP wrapper. For V2 messages (the bulk of the messages on our network right now), this is cryptographically authenticated via signature.
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.
Yes right. We can use the senderAddress
from the XMTP message itself instead of relying on this. Should I remove it and commit ?
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.
Yes right. We can use the senderAddress from the XMTP message itself instead of relying on this. Should I remove it and commit ?
Ya you can remove this and commit.
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.
Yes right. We can use the senderAddress from the XMTP message itself instead of relying on this. Should I remove it and commit ?
Ya you can remove this and commit.
Okay sure.
XIPs/xip-reaction-content-type.md
Outdated
{ | ||
messageId: string | ||
senderAddress: string | ||
messageContent: string |
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.
Would this be the message.content
from the message you are reacting to? Why do we need to duplicate that in the Reaction? Couldn't clients just read the original message?
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.
yes messageContent
is the message.content
from the message we're replying to.
It's necessary to duplicate it for the following use cases :
- We're loading the last messages of all the conversations of a user to show them in the chat previews. Whenever the last message is a Reaction, we need the
messageContent
andcontentTypeId
to display which message it has been reacted to. ( See the image)
- There are cases where the message we reacted to is not loaded yet (due to the pagination) and so we won't have
messageContent
andcontentTypeId
info if we try to filter and find it from the message's state in the application through the Reaction's messageId.
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 think we should just pass IDs here, since duplication could lead to different content being passed in different places which would lead to inconsistency. It also doesn't account for non-text messages.
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.
XIPs/xip-reaction-content-type.md
Outdated
senderAddress: string | ||
messageContent: string | ||
contentTypeId: string | ||
reaction: string |
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.
If the reaction field is only ever expected to be a single UTF-8 emoji character, we can indicate and enforce that the maximum allowed length for this field is 4 bytes. Otherwise someone can dump a short novel into the field and it would be up to the receiving client to figure out what to do with that.
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.
Yes exactly that's better. At what step do you recommend the validation should be done ?
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.
Both encode and decode should validate
Just pushed some updates here to remove duplicate fields from the proposal as well as tweak where the reaction actually gets stored (the content rather than as a parameter). I've also added a link to a reference implementation here. |
I just wanted to note that we added reaction content types into the JS SDK here: xmtp/xmtp-js-content-types#13 & xmtp/xmtp-js-content-types#9 and we gave a reference implementation in our React playground here: xmtp/xmtp-react-playground#1 If you have questions, comments, feedback, or concerns please raise them here: https://github.com/orgs/xmtp/discussions/36 |
After more than six months of inactivity on this XIP, we have set its status to Stagnant. We have merged the XIP to the repo for historical purposes and are closing this PR. Thank you very much for your contribution, @Im-Kunal-13! |