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

MSC2998: Room Version 7 #2998

Merged
merged 6 commits into from
Mar 1, 2021
Merged

MSC2998: Room Version 7 #2998

merged 6 commits into from
Mar 1, 2021

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Feb 8, 2021

@anoadragon453 anoadragon453 changed the title MSCXXXX: Room Version 7 MSC2998: Room Version 7 Feb 8, 2021
@anoadragon453 anoadragon453 added kind:core MSC which is critical to the protocol's success proposal A matrix spec change proposal labels Feb 8, 2021
A new room version, `7`, is proposed using [room version 6](https://matrix.org/docs/spec/rooms/v6.html) as a base
and incorporating the following MSCs:

* [MSC2174](https://github.com/matrix-org/matrix-doc/pull/2174) - Move the `redacts` key to a sane place.
Copy link
Member

Choose a reason for hiding this comment

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

There's no point in having two migrations of redaction format, we should just skip this step and go straight for mass redactions

Copy link
Member Author

Choose a reason for hiding this comment

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

@tulir I didn't include Mass Redactions as it doesn't currently have an implementation.

You're right in that putting MSC2244 and MSC2174, especially as the former builds on the latter, into the same room version makes sense though. I'd be happy to review/help with a Synapse PR for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

also, doesn't content.redacts have to be protected from redaction now?

Copy link
Member

Choose a reason for hiding this comment

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

It is protected from redaction by MSC2176, that part works the same way for MSC2174 and MSC2244.

@turt2live turt2live self-requested a review February 8, 2021 18:01
@anoadragon453
Copy link
Member Author

It turns out MSC2175 (remove the creator field from create events) and MSC2174 (move the redacts key) do not have an implementation yet after all, so they will also be excluded. MSC2176 (update redacted event fields) does have an implementation however, and while related to redactions, is only concerned about the keys removed from an event by the server when an event is redacted. This does not affect the structure of a redaction event, like MSC2174 and MSC2244 (mass redactions) is concerned with.

While this leaves us with a fairly lean room version, releasing new features and changes more regularly in smaller room versions is a good precedence to set.

@clokep
Copy link
Member

clokep commented Feb 18, 2021

It turns out MSC2175 (remove the creator field from create events) and MSC2174 (move the redacts key) do not have an implementation yet after all, so they will also be excluded. MSC2176 (update redacted event fields) does have an implementation however, and while related to redactions, is only concerned about the keys removed from an event by the server when an event is redacted.

I'm not sure I fully grok what this is saying, but #2176 seems to imply it only makes sense if #2174 was also implemented?

@anoadragon453
Copy link
Member Author

MSC2174, MSC2176 and MSC2274 do indeed rely on each other and it would be best to land all of those in a single room version. As the implementation for each are not yet ready, as well as lacking an implementation for MSC2175 we're going to leave them til the next room version, and just go ahead with knocking for this one.

Note that this MSC has been updated to simply reserve v7 for knocking. It does not prevent an MSC for room v8+ appearing in parallel once the above MSCs (or any other MSCs for that matter) have implementations ready.

Given the above, I'm going to propose FCP on this one to get the rest of the team's feedback.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Feb 22, 2021

Team member @anoadragon453 has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. and removed proposal-in-review labels Feb 22, 2021
@turt2live turt2live removed their request for review February 22, 2021 19:14
* [MSC2403](https://github.com/matrix-org/matrix-doc/pull/2403) - Add "knock" feature.

Though other MSCs are capable of being included in this version, they do not have sufficient implementation to be
considered stable enough for v7 rooms. A future room version may still include them.
Copy link
Member

@dbkr dbkr Feb 24, 2021

Choose a reason for hiding this comment

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

This took me about 5 reads to understand - isn't it just saying, "other mscs aren't ready yet because they don't have impls"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, but written a bit tersely for the spec. I'm open to suggestions for something slightly clearer though.

Choose a reason for hiding this comment

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

Maybe Though other MSCs could be included in this version, they do not have satisfactory implementations...?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've dropped the stable enough bit. Hopefully that makes things slightly clearer.

@mscbot
Copy link
Collaborator

mscbot commented Feb 24, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Feb 24, 2021
@mscbot
Copy link
Collaborator

mscbot commented Mar 1, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Mar 1, 2021
@turt2live turt2live merged commit 09e5015 into master Mar 1, 2021
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Apr 6, 2021
@turt2live turt2live self-assigned this Apr 28, 2021
turt2live added a commit that referenced this pull request Apr 29, 2021
Spec for #2998
Spec for #2403

This deliberately does not help towards fixing #3153 in order to remain consistent with prior room versions, and to keep the diff smaller on this change. A future change will address room version legibility.
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review merged A proposal whose PR has merged into the spec! and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Apr 29, 2021
@turt2live
Copy link
Member

merged 🎉

richvdh pushed a commit that referenced this pull request Aug 23, 2021
richvdh pushed a commit that referenced this pull request Aug 23, 2021
Spec for #2998
Spec for #2403

This deliberately does not help towards fixing #3153 in order to remain consistent with prior room versions, and to keep the diff smaller on this change. A future change will address room version legibility.
richvdh pushed a commit that referenced this pull request Aug 27, 2021
richvdh pushed a commit that referenced this pull request Aug 27, 2021
Spec for #2998
Spec for #2403

This deliberately does not help towards fixing #3153 in order to remain consistent with prior room versions, and to keep the diff smaller on this change. A future change will address room version legibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants