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

Add Gateway Opcode enum #571

Merged
merged 3 commits into from
Oct 14, 2024
Merged

Add Gateway Opcode enum #571

merged 3 commits into from
Oct 14, 2024

Conversation

bitfl0wer
Copy link
Member

Introduces and publicly exports an enum Opcode which represents all currently known Opcodes that can be sent to/received from Spacebar Gateway Protocol compatible services.

src/gateway/mod.rs Fixed Show fixed Hide fixed
Copy link
Member

@kozabrada123 kozabrada123 left a comment

Choose a reason for hiding this comment

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

Generally I'm not opposed to this, but #527 (comment) kind of applies here as well. #[non_exhaustive] is definitely a solution, we could possibly even add the other two.

Though I think in our client implementation we shouldn't use these kinds of enums, since in the case we receive a variant we haven't added yet we'd just get a deserialization error, so I prefer a u8 and constants

src/gateway/mod.rs Show resolved Hide resolved
src/types/utils/opcode.rs Show resolved Hide resolved
@bitfl0wer
Copy link
Member Author

Generally I'm not opposed to this, but #527 (comment) kind of applies here as well. #[non_exhaustive] is definitely a solution, we could possibly even add the other two.

non-exhaustive is a good solution, I think. I am also not sure about the comment on #527. We are implementing API v9 for now, which is pretty much fixed at this point.

Though I think in our client implementation we shouldn't use these kinds of enums, since in the case we receive a variant we haven't added yet we'd just get a deserialization error, so I prefer a u8 and constants

You'd have the same problem when using u8/consts, no? As in, if you encounter an undefined opcode, that will be an error which has to be handled somehow.

Co-authored-by: kozabrada123 <59031733+kozabrada123@users.noreply.github.com>
@kozabrada123
Copy link
Member

Generally I'm not opposed to this, but #527 (comment) kind of applies here as well. #[non_exhaustive] is definitely a solution, we could possibly even add the other two.

non-exhaustive is a good solution, I think. I am also not sure about the comment on #527. We are implementing API v9 for now, which is pretty much fixed at this point.

Though I think in our client implementation we shouldn't use these kinds of enums, since in the case we receive a variant we haven't added yet we'd just get a deserialization error, so I prefer a u8 and constants

You'd have the same problem when using u8/consts, no? As in, if you encounter an undefined opcode, that will be an error which has to be handled somehow.

Yes, but I prefer handling it as: "Unexpected opcode, please open an issue" instead of "Failed to deserialize gateway message". That's how we handle it now.

We are implementing API v9 for now, which is pretty much fixed at this point.

Sadly I don't think that's the case, especially for the user api 🥴

@bitfl0wer
Copy link
Member Author

To add some context for this PR:
I need to match GatewayEvents for the Gateway impl in symfonia. This requires me to match the u8 opcode to an Event type. To avoid mismatches, I thought it'd be a good idea to have chorus act as a single source of truth for this. The enum is not entirely necessary - pub exported consts would also work, but they'd arguably amount to the same solution in the end.

@bitfl0wer
Copy link
Member Author

bitfl0wer commented Oct 14, 2024

Yes, but I prefer handling it as: "Unexpected opcode, please open an issue" instead of "Failed to deserialize gateway message". That's how we handle it now.

Is this not something that is up to us? When handling incoming gateway messages, if the opcode does not match anything in Opcode, we can handle it as you described, no?

@kozabrada123
Copy link
Member

Yes, but I prefer handling it as: "Unexpected opcode, please open an issue" instead of "Failed to deserialize gateway message". That's how we handle it now.

Is this not something that is up to us? When handling incoming gateway messages, if the opcode does not match anything in Opcode, we can handle it as you described, no?

We could do that, sure. But then the field that serde deserializes will still have to be a u8, which is kind of ugly. I suppose what we'd do then is manually try do fit it into the enum and display the same message as we do now

@bitfl0wer
Copy link
Member Author

And what about the as-is solution, where the consts we declare simply refer to the Opcode type as u8? This way, we have both the publicly exported Opcode type and no further code-changes are needed, if I am not mistaken.

@kozabrada123
Copy link
Member

kozabrada123 commented Oct 14, 2024

Sure, that's the easiest solution. Since the constants now just relate to the enum as u8, we could also remove them, but it honestly doesn't matter

@bitfl0wer
Copy link
Member Author

I can do that! Thank you for your review, your opinion is, as always, much appreciated :))

@bitfl0wer bitfl0wer merged commit 27c228e into dev Oct 14, 2024
9 checks passed
kozabrada123 added a commit that referenced this pull request Nov 24, 2024
Release tracker for v0.18.0 of chorus, set to release on November 24th, 2024.

## Public API changes
- #570: Various entity public api changes
- 644d3be, 85e922b: Add type `OneOrMoreSnowflakes`, allow `GatewayRequestGuildMembers` to request multiple guild and user ids
- f65b9c1: Differentiate `PresenceUpdate` and `GatewayPresenceUpdate`
- 0e5fd86: Temporarily fix `PresenceUpdate` for Spacebar Client by making `user` optional
- 61ac7d1: Updated `LazyRequest` (op 14) to use the `Snowflake` type for ids instead of just `String`

## Additions
- #564: MFA implementation, by @xystrive and @kozabrada123 
- 4ed68ce: Added [Last Messages request](https://docs.discord.sex/topics/gateway-events#request-last-messages) and [response](https://docs.discord.sex/topics/gateway-events#last-messages)
- b23fb68: Add `ReadState` to `GatewayReady`
- #571: Gateway Opcode enum
- #573: Gateway Disconnect Opcode enums

## Bugfixes

- #565: Fix sqlx En-/Decoding of `PremiumType`
- 7460d3f: Fix `GatewayIdentifyConnectionProps` for Spacebar Client by deriving default on all fields, since the client does not send it
-  3d9460f: Derive Default for `MessageReferenceType`, assume default reference_type if none is provided
- 4baecf9: Fixed a deserialization error related to `presences` in `GuildMembersChunk` being an array, not a single value
- 1b20102: Fixed a deserialization error with deserializing `activities` in `PresenceUpdate` as an empty array when they are sent as `null`
- 7feb571: Fixed a deserialization error on discord.com related to experiments (they are not implemented yet, see #578)
- fb94afa: Fixed a deserialization error on discord.com related to `last_viewed` in `ReadState` being a version / counter, not a `DateTime`

## Internal changes
- 40754c5: bump sqlx-pg-uint to v0.8.0
- #575: Refactor of gateway close code handling
- 4ed68ce: Refactored the gateway to fully use the `Opcode` enum instead of constants
- #579

---------

Co-authored-by: bitfl0wer <florian@pro-weber.com>
Co-authored-by: Flori <39242991+bitfl0wer@users.noreply.github.com>
Co-authored-by: xystrive <xys@xystrive.dev>
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.

2 participants