-
Notifications
You must be signed in to change notification settings - Fork 380
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
Room versions 8 and 9: Restricted rooms #3387
Conversation
Co-authored-by: Patrick Cloke <[email protected]>
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.
Looks good except for a couple of inconsistencies!
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.
thanks for your work on this!
I'm afraid I'd completely paged out where we'd got to previously, so unlucky for you, you get a complete re-review. A few bits and bobs, hopefully nothing major.
title: SignedMembershipEvent | ||
x-addedInMatrixVersion: "1.2" | ||
description: |- | ||
Required if the room version [supports restricted join rules](/rooms/#feature-matrix). The signed |
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 think this should be always required as of this matrix version.
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 MSC doesn't call for this, and Synapse doesn't appear to check it that way. Source?
Co-authored-by: Richard van der Hoff <[email protected]>
blocked on internal SCT review. |
conclusion from internally appears to be that these do indeed only affect requests with the given room version - to capture the nuance, we'll probably want to look at adjusting the "Joining Rooms" section of the spec to cover signatures, residents, etc. The original worry was that suggestions to change the spec implied that the process was changed universally, which is not the case. Still blocked on me adapting the notes to the PR though. |
@richvdh this should be ready for re-review. |
@@ -199,6 +199,10 @@ completeness. | |||
|
|||
{{% rver-fragment name="v4-event-format" %}} | |||
|
|||
### Handling redactions |
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.
it'd be nice if we could figure out what order these things go in (in v5 'Handling redactions' is before 'Event IDs' and 'Event format'), but I guess I'm just nit-picking really.
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.
they're supposed to be consistent, but that ship sailed when I thought they were consistent last time. Will attempt to fix in a future PR.
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.
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.
generally seems fine, maybe a couple of nits
MSCs:
Preview: https://pr3387--matrix-org-previews.netlify.app