-
Notifications
You must be signed in to change notification settings - Fork 186
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
Stabilize MSC2409 (Send typing, presence and receipts to appservices) #17881
base: develop
Are you sure you want to change the base?
Conversation
"ephemeral": ephemeral, | ||
# NOTE: This is actually https://github.com/matrix-org/matrix-spec-proposals/blob/tulir/appservice-to-device/proposals/4203-appservice-to-device.md | ||
# but for legacy reasons uses an older MSC number. | ||
"de.sorunome.msc2409.to_device": to_device_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.
I was unsure whether this should be included if the AS opts into the stable flag in it's registration file, but on balance I think this would cause us to have to maintain the legacy flag for the sake of ASes that want to have to-device messages.
This field is still only populated if the homeserver opts into MSC4203, so it's not like we're ever emitting unstable information on a stable HS.
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.
This field is still only populated if the homeserver opts into MSC4203, so it's not like we're ever emitting unstable information on a stable HS.
I'm not following. It seems like this is shown when supports_ephemeral
is enabled which is derived from supports_ephemeral = as_info.get("receive_ephemeral", False)
which is a field that is specced from MSC2409 and is now stable.
I see that we have msc4203_to_device_messages_enabled
but seems like de.sorunome.msc2409.to_device
is still returned. Unclear if an empty list will be omitted from the response or something.
"de.sorunome.msc2409.to_device": to_device_messages, | ||
} | ||
) | ||
elif service.supports_ephemeral_legacy: |
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've chosen to keep the old fields around for a bit to allow ASes to migrate, but at some point this will need to be deprecated.
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.
This might need to be a bigger note in changelogs when it's deprecated/removed, because ASes can't update registrations themselves, server admins have to do it manually
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, sounds reasonable. Worth putting in the upgrade notes too.
"ephemeral": ephemeral, | ||
# NOTE: This is actually https://github.com/matrix-org/matrix-spec-proposals/blob/tulir/appservice-to-device/proposals/4203-appservice-to-device.md | ||
# but for legacy reasons uses an older MSC number. | ||
"de.sorunome.msc2409.to_device": to_device_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.
This field is still only populated if the homeserver opts into MSC4203, so it's not like we're ever emitting unstable information on a stable HS.
I'm not following. It seems like this is shown when supports_ephemeral
is enabled which is derived from supports_ephemeral = as_info.get("receive_ephemeral", False)
which is a field that is specced from MSC2409 and is now stable.
I see that we have msc4203_to_device_messages_enabled
but seems like de.sorunome.msc2409.to_device
is still returned. Unclear if an empty list will be omitted from the response or something.
# For ASes that haven't transitioned to the stable fields yet | ||
supports_ephemeral_legacy = False | ||
if not supports_ephemeral: | ||
supports_ephemeral_legacy = as_info.get( | ||
"de.sorunome.msc2409.push_ephemeral", False | ||
) |
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.
Any tests for the legacy version?
@@ -270,7 +269,7 @@ def notify_interested_services_ephemeral( | |||
# Ignore to-device messages if the feature flag is not enabled | |||
if ( | |||
stream_key == StreamKeyType.TO_DEVICE | |||
and not self._msc2409_to_device_messages_enabled | |||
and not self._msc4203_to_device_messages_enabled | |||
): | |||
return | |||
|
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 don't think StreamKeyType.TO_DEVICE
is supposed to be in the list of supported streams for supports_ephemeral
(see the linked line below)
For matrix-org/matrix-spec-proposals#2409
Related matrix-org/matrix-spec-proposals#4203
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.(run the linters)