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.

Sign up for GitHub

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

Jump to bottom

MSC3585: Allow the base event to be omitted from /federation/v1/event_auth response #3585

Closed
wants to merge 8 commits into from
Closed

MSC3585: Allow the base event to be omitted from /federation/v1/event_auth response #3585

wants to merge 8 commits into from

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Dec 22, 2021 •
edited by github-actions bot
Loading

@richvdh richvdh added the proposal A matrix spec change proposal label Dec 22, 2021
@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal-in-review s2s Server-to-Server API (federation) labels Dec 22, 2021
@richvdh richvdh changed the title MSC3585: Allow the base event can be omitted from /federation/v1/event_auth response MSC3585: Allow the base event to be omitted from /federation/v1/event_auth response Dec 22, 2021
richvdh and others added 2 commits December 22, 2021 16:19
@richvdh
Note that conduit already does this
0bb0893
@richvdh @Cadair
review comments; thanks @Cadair
bc37a02 
Co-authored-by: Stuart Mumford <stuart@cadair.com>
richvdh added a commit to matrix-org/synapse that referenced this pull request Dec 22, 2021
@richvdh
Elide the requested event in /event_auth
6e479aa 
[MSC3585](matrix-org/matrix-spec-proposals#3585) says this is a
thing we can do.
@richvdh richvdh mentioned this pull request Dec 22, 2021
Closed
@richvdh richvdh removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Dec 22, 2021
@richvdh
Copy link
Member Author

richvdh commented Dec 22, 2021

I think this is a no-brainer.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Dec 22, 2021 •
edited
Loading

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

Concerns:

  • Check Dendrite compatibility

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 Dec 22, 2021
uhoreg
uhoreg approved these changes Dec 22, 2021
View reviewed changes
Copy link
Member

@uhoreg uhoreg left a comment

Choose a reason for hiding this comment

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

Looks sensible. The fact that conduit already does this seems like a reasonable indication that it won't cause the world to explode.

proposals/3585-drop-event-from-event_auth.md Outdated Show resolved Hide resolved
@richvdh @uhoreg
fix typo
571b8bc 
Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
@turt2live turt2live self-requested a review December 22, 2021 23:04
turt2live
turt2live reviewed Dec 28, 2021
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

proposals/3585-drop-event-from-event_auth.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

@mscbot concern Check Dendrite compatibility

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Dec 28, 2021
@turt2live
Copy link
Member

@mscbot resolve Check Dendrite compatibility

@mscbot mscbot removed the unresolved-concerns This proposal has at least one outstanding concern label Jan 5, 2022
@richvdh
Copy link
Member Author

richvdh commented Jan 6, 2022

SIGH. It turns out that synapse actually already does what this MSC proposes. The code is misleading, but the functionality is correct. The whole premise of this MSC is that the /de-facto/ spec, based on what Synapse does, is that eventId should be returned - but that is now clearly incorrect.

I also think that #3583 has clarified the written spec sufficiently: it is clear that the auth chain of an event does not include that event itself. So no further changes are needed to the spec imho.

Meanwhile, @kegsay has been actually writing tests to verify behaviour (matrix-org/complement#280) (thank you).

@richvdh richvdh closed this Jan 6, 2022
@turt2live turt2live added obsolete A proposal which has been overtaken by other proposals and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turt2live turt2live turt2live left review comments

@Cadair Cadair Cadair left review comments

@uhoreg uhoreg uhoreg approved these changes

Assignees
No one assigned
Labels
kind:maintenance MSC which clarifies/updates existing spec obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal s2s Server-to-Server API (federation)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@richvdh @mscbot @turt2live @uhoreg @Cadair