Skip to content

Conversation

@gnieto
Copy link
Contributor

@gnieto gnieto commented Sep 25, 2022

Related to: matrix-org/matrix-spec#998

Setting origin field in events/PDUs is not required any more, so I've removed assertions and assignments to this field.

Related to: matrix-org/matrix-spec#998

Setting `origin` field in PDUs is not required anymore, so I've removed
assertions and assignments to this field.
@gnieto gnieto requested a review from a team as a code owner September 25, 2022 19:24
@gnieto
Copy link
Contributor Author

gnieto commented Sep 26, 2022

Apparently, Dendrite is no happy with this change.
I can just remove the assertions, alternatively, if you are fine with that.

@DMRobertson
Copy link
Contributor

Apparently, Dendrite is no happy with this change.

Probably worth opening a Dendrite bug (if there isn't one for this already).

I can just remove the assertions, alternatively, if you are fine with that.

Sounds reasonable.

@DMRobertson DMRobertson requested a review from a team September 26, 2022 10:36
@richvdh
Copy link
Member

richvdh commented Sep 26, 2022

I can just remove the assertions, alternatively, if you are fine with that.

If you take this path, please can you comment the places we set origin with a link to the dendrite bug?

auth_events content depth prev_events room_id sender
state_key type ) ),

# The spec does not require the origin field, but Dendrite does. This can be removed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@richvdh Is this similar to what you were expecting? Let me know if you want me to change the message. Thanks!

@neilalexander
Copy link
Contributor

matrix-org/dendrite#2737
matrix-org/gomatrixserverlib#341

Please hold off on doing anything until we get these landed.

Copy link
Contributor

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

Right, this should be OK now that we've fixed Dendrite. I have reverted aed2b10 — LGTM if CI is green.

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thanks Neil A for the prompt response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants