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

document device list synchronisation over federation. #1648

Merged
merged 10 commits into from
Sep 3, 2018

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Sep 1, 2018

closes #1212

@ara4n ara4n requested a review from a team September 1, 2018 01:39
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.

I haven't checked accuracy, but here's some structural stuff

edu_type:
type: enum
enum: ['m.device_list_update']
description: The string ``m.device_list_update``
Copy link
Member

Choose a reason for hiding this comment

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

Descriptions should end in full stops.

Copy link
Member

Choose a reason for hiding this comment

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

(this is a theme throughout)

The updated identity keys (if any) for this device. May be absent if the
device has no E2E keys defined.
allOf:
- $ref: ../../../client-server/definitions/device_keys.yaml
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs an allOf and can just have the $ref at the same level as description.

Matrix currently uses a custom pubsub system for synchronising information
about the list of devices for a given user over federation. When a server
wishes to determine a remote user's device list for the first time,
it should populate its local cache by calling the /user/keys/query API
Copy link
Member

Choose a reason for hiding this comment

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

preferably API routes should be surrounded in backticks (or be links)

which EDUs a server needs to have received in order to apply an update to its local
copy of the remote user's device list. If a server receives an EDU which refers to
a ``prev_id`` it does not recognise, it must resynchronise its list by calling the
/user/keys/query API and resume the process. The response contains a ``stream_id``
Copy link
Member

Choose a reason for hiding this comment

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

backticks around api routes

@ara4n
Copy link
Member Author

ara4n commented Sep 1, 2018

@erikjohnston please can you check this for factual accuracy so we can merge it (or at least fix & merge it? :)

@erikjohnston
Copy link
Member

Ideally this would also talk about when a server should send device updates and to whom, as well as documenting what to do when a server receives an update that points to a prev stream ID it hasn't seen.

What's there looks broadly accurate.

@erikjohnston erikjohnston assigned ara4n and unassigned erikjohnston Sep 2, 2018
@ara4n
Copy link
Member Author

ara4n commented Sep 2, 2018

Ideally this would also talk about when a server should send device updates and to whom

Yup, it didn't spell out who the device updates should be sent to; hopefully now fixed.

as well as documenting what to do when a server receives an update that points to a prev stream ID it hasn't seen.

This was documented twice:

If a server receives an EDU which refers to a prev_id it does not recognise, it must resynchronise its list by calling the /user/keys/query API and resume the process.

in the s2s verbiage

and

If the receiving server does not recognise any of the prev_ids, it means an EDU has been lost and the server should query a snapshot of the devicelist via /user/keys/query in order to correctly interpret future m.device_list_update EDUs.

in the m.device_list_update definition.

If this is insufficient, can you please say what the gap is so I can fill it in?

@ara4n ara4n assigned erikjohnston and unassigned ara4n Sep 2, 2018
@turt2live
Copy link
Member

Finishes off the last of #501

any existing entry in the local server's cache of that device list. Servers must send
``m.device_list_update`` EDUs to all the servers whose users participate in their rooms,
and must be sent whenever a local user's device list changes (i.e. new or deleted devices,
or changes of identity keys).
Copy link
Member

Choose a reason for hiding this comment

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

Or a user joins (gets invited?) into a room.

@erikjohnston
Copy link
Member

D'oh. Apparently you need to say it a few more times for it to actually sink here.

Otherwise LGTM modulo small comment

@ara4n
Copy link
Member Author

ara4n commented Sep 3, 2018

hopefully fixed the small comment.

@ara4n ara4n merged commit 6dab4b2 into master Sep 3, 2018
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.

Device List Update Stream
3 participants