Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC2477: User-defined ephemeral events in rooms #2477
base: old_master
Are you sure you want to change the base?
MSC2477: User-defined ephemeral events in rooms #2477
Changes from 9 commits
aad3e26
4aa82b8
d4930c0
ba0b657
0b27339
5dc9372
dd488d3
9d73697
2f4dd47
047d63b
2cae98c
ff2ddbe
64e9460
d01e95d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
worth noting that this limits custom EDUs to rooms compared to something like presence which is user based instead of room based.
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.
The second line in my proposal block already notes this, as I didn't think the usefulness of custom non-room EDUs merited the work of designing some whole new kind of permission system for sending them. Though perhaps a reference to user EDUs built on the Profile-as-a-Room MSC1769 could be added.
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.
Following the new per-endpoint versioning spec.
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 if we should make this off-limits to anything
m.*
. In fact, I wonder if we should go the opposite way and transition all room-based ephemeral messages to this endpoint - so read receipts and typing notifications.At the moment, typing notifications are using:
and read markers are sent with:
The
{userId}
bit of the typing endpoint is especially useless, and could be cleaned up in the same move.Presence and to-device messages would be excluded from this, as they are not bound to a room.
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.
Read markers seem to be a bit of a special case, as they have an effect on the server to store something quasi-persistent, shouldn't read_markers stay out of this then? or what is the idea behind 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.
I wrote this MSC on the assumption that the custom EDUs would be a separate beast to the built-in EDUs, but I guess it could just as well be done to instead have the server handle well-defined EDUs in a certain manner. So that you - as a client dev/user - could just post
m.typing
orm.receipt
/m.fully_read
/m.read
EDUs instead of calling the specific endpoints for them.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.
Good point.
m.fully_read
does not fit this model, as its purpose is to store your read-up-to position, which lives in the current user's room account data. The linked endpoint just happens to allow optionally sending anm.read
receipt in the same call.Instead, we should be thinking about whether to replicate the behaviour of
POST /_matrix/client/v3/rooms/{roomId}/receipt/{receiptType}/{eventId}
with this then.With that, I'm not so sure. They do fall under the banner of EDU, but a receipt is always tied to an event ID (e.g. I read up to this event ID). We'd need to include that event ID in the body of the request for
m.read
or for any other type of receipt. That can work, though I suppose it comes down to how much we'd like to differentiate receipts as their own thing, versus just another type of EDU.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.
As mentioned earlier, any change to the auth rules requires a new room version. If we wanted to have these only apply to user-defined types initially, and then fully apply later, that would take two room versions.
Given rooms will need to be recreated anyways, it seems sensible to simply have homeservers pre-fill
0
form.typing
andm.fully_read
entries when creating a room to preserve existing behaviour?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 used
m.receipt
as the type when I was writing, as I wasn't sure aboutm.read
(andm.fully_read
). The resulting event ism.receipt
, and the endpoint for sending the information isrooms/{roomId}/receipt
. But the receipt type as defined in the content ism.read
.m.fully_read
is in the spec handled in a separate endpoint, but includes a link to receipts as you can includem.read
in the request to combine the two into a single request. Not sure how that would be combined into things.For now I think I'm going to keep
m.receipt
as the general EDU type, since it's the type that - for now - is seen in an actual ephemeral event. (after allm.fully_read
only ends up in the account data, andm.read
are agglomerated intom.receipt
events)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.
room_id
isn't part of the spec for ephemeral event bodies down /sync.Likewise below.
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.
The spec entry you linked includes an
m.typing
event in the example response for code 200, which does includeroom_id
in the data;I think that's where that came from, since I tried to base my examples directly on the spec examples.
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 see, thanks for pointing that out! I've made a PR to fix that, as I believe that's a spec bug: #3679.
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 I understand correctly, this is referring to adding the
sender
andorigin_server_ts
fields to a JSON object. In the past I believe we've considered adding keys to JSON to not break backwards compatibility, as clients should just be pulling out the keys they need anyhow - and these events are not signed or otherwise wholly checked for integrity.This would have to be a problem that we solve anyways, regardless of backwards-compatibility periods.
I'm also not clear why it would be difficult to populate these fields for typing and read receipt lists.
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.
As one example on the second point;
m.typing
events are aggregated down from the source data into a single event with a list ofuser_ids
that are currently typing. If they are to have correct timestamps and sender data attached to match the EDU type as defined in this MSC, then they'd have to be split apart into separate objects each with their own metadata. I'm not really sure if that's desirable.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.
Unstable prefixes will need to be defined in order for an implementation of the MSC to be carried out. I suggest implementation replace:
ephemeral
andephemeral_default
fields in them.room.power_levels
state event withorg.matrix.msc2477.ephemeral
andorg.matrix.msc2477.ephemeral_default
respectively.PUT /_matrix/client/v1/rooms/{roomId}/ephemeral/{eventType}/{txnId}
endpoint withPUT /_matrix/client/unstable/org.matrix.msc2477/rooms/{roomId}/ephemeral/{eventType}/{txnId}
.And an experimental room version with name
org.matrix.msc2477
should be used where this feature is enabled.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.
And a whole lot later than it should've been, but I've added a section on an unstable prefix.
Wasn't sure about adding blurbs on when the
m.room.power_levels
keys should be allowed, but I figured that it'd a bit superfluous since the only method that will act on them is limited to the experimental room version.