Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

Im-Kunal-13
Copy link

  • Create xip-reaction-content-type.md

```ts
{
messageId: string
senderAddress: string

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.

Copy link
Author

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 ?

Copy link
Contributor

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.

Copy link
Author

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.

{
messageId: string
senderAddress: string
messageContent: string

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?

Copy link
Author

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 :

  1. 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 and contentTypeId to display which message it has been reacted to. ( See the image)

image

  1. There are cases where the message we reacted to is not loaded yet (due to the pagination) and so we won't have messageContent and contentTypeId info if we try to filter and find it from the message's state in the application through the Reaction's messageId.

Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can remove the messageContent and contentTypeId and only keep the messageId and reaction for this to work.

Although as we won't have the the content of the message we reacted to, we can't show them in the preview. It will be just the reaction.

image

senderAddress: string
messageContent: string
contentTypeId: string
reaction: string

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.

Copy link
Author

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 ?

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

@nakajima
Copy link
Contributor

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.

@nplasterer
Copy link
Contributor

nplasterer commented Jul 14, 2023

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
This should unblock any jsSDK or reactnativeSDK integrators.
We have tasks to add support to kotlin, swift, and flutter soon.

If you have questions, comments, feedback, or concerns please raise them here: https://github.com/orgs/xmtp/discussions/36

@jhaaaa
Copy link
Contributor

jhaaaa commented May 16, 2024

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!

@jhaaaa jhaaaa closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants