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.

Sign up for GitHub

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

Jump to bottom

json deserialization for legacy reactions #1488

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

json deserialization for legacy reactions #1488

wants to merge 9 commits into from

Conversation

cameronvoell
Copy link
Contributor

@cameronvoell cameronvoell commented Jan 11, 2025 •
edited
Loading

Part of #1403

@cameronvoell Graphite App
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@cameronvoell cameronvoell force-pushed the cv/01-10-json_deserialization_for_legacy_reactions branch from 7b61e5b to a7ab65f Compare January 16, 2025 00:34
@cameronvoell cameronvoell force-pushed the cv/01-10-json_deserialization_for_legacy_reactions branch from a7ab65f to 1744eca Compare January 16, 2025 00:44
@cameronvoell cameronvoell marked this pull request as ready for review January 16, 2025 17:56
@cameronvoell cameronvoell requested a review from a team as a code owner January 16, 2025 17:56
neekolas
neekolas reviewed Jan 17, 2025
View reviewed changes
xmtp_content_types/src/reaction.rs
@@ -47,6 +48,41 @@ impl ContentCodec<ReactionV2> for ReactionCodec {
}
}

#[derive(Debug, Serialize, Deserialize)]
pub struct LegacyReaction {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't shoe-horn the legacy reaction into the new reaction type? I thought the data was about the same

Copy link
Contributor Author

@cameronvoell cameronvoell Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Original type had arbitrary String for reaction action and reaction schema (js sdk used union type to restrict it), I updated to enum in the new proto type to be more precise so we wouldnt need to do this type of conversion across all the SDKs: https://github.com/xmtp/xmtp-android/blob/51c7b170d53f73d32d3a1d33b0d170d156183e43/library/src/main/java/org/xmtp/android/library/codecs/ReactionCodec.kt#L40-L55

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh. Too bad.

Agree the new format is nicer.

@cameronvoell cameronvoell requested a review from neekolas January 17, 2025 23:09
@nplasterer
Copy link
Contributor

👋 Sorry for the drive by comment here just getting caught up on my notifications.
When you say legacy reaction type are you referencing the legacy reaction types from our very very first reaction iteration in 2023? We have to support that in V2 because of existing reactions existing on the network before we switched to the new scheme. We switched to the new scheme before V3 was ever supported so there shouldn't be any need to continue to support these in V3. There shouldn't be any legacy reactions in V3. This legacy check here https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/codecs/ReactionCodec.kt#L82-L88 should actually be deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neekolas neekolas Awaiting requested review from neekolas

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cameronvoell @nplasterer @neekolas