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

MSC3682: Sending Account Data to Application Services #3682

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

reivilibre
Copy link
Contributor

@reivilibre reivilibre commented Jan 28, 2022

@reivilibre reivilibre marked this pull request as ready for review January 28, 2022 15:54
@reivilibre reivilibre changed the title MSCXXXX: Sending Account Data to Application Services MSC3682: Sending Account Data to Application Services Jan 28, 2022
@turt2live turt2live added application services kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal proposal-in-review labels Jan 28, 2022
@turt2live turt2live self-requested a review January 29, 2022 04:02
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.

Generally looks good! Thanks for writing it up.

Just a handful of relatively minor comments, all things considered.

proposals/3682-account-data-for-application-services.md Outdated Show resolved Hide resolved
Comment on lines +29 to +30
"@asuser1:example.org": {
"events": [
Copy link
Member

Choose a reason for hiding this comment

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

we could also just collapse this down to "@user:example.org": [ {"type": ... } ] as we don't need the layer of indirection events provides.

Comment on lines +60 to +61
This introduces additional implementation complexity to homeservers which now need to detect when
account data changes and send changes. Specifically, it shifts the interaction from a pull model to a push model, which some homeserver implementations may not have been designed for initially.
Copy link
Member

Choose a reason for hiding this comment

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

it's somewhat worth mentioning that the "push" model is expected by servers already in the form of /sync: with long-polling, the request only concludes if no changes happened, which means an internal wake/push to the sync loop to do something. In the case of appservices, instead of it being a wake to the sync loop it's a wake to the transaction builder.


Some Application Services may not benefit from this additional information in which case it would
be wasteful of computational resources to compute and transmit it.
⇒ **TODO** opt-in/out in the AS registration file. Good idea or not?
Copy link
Member

Choose a reason for hiding this comment

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

Probably best as an implementation detail for now - the spec should support it not being present at all, though I've never been happy with us defining how a registration file works in the spec.

@anoadragon453
Copy link
Member

Note to future implementers: if your project has implemented MSC3391 then you may need to be cautious about when you fully "delete" an account data entry. Make sure you inform application services of the change first before you do!

The second half of the description of matrix-org/synapse#14244 has some details about the implementation of MSC3391 in Synapse and tracking when a user's devices has seen an account data item be deleted.

I don't imagine it'll be too bad as we'll likely inform application services of changes to account data right away - but just something to consider for your reliability model.

Co-authored-by: Travis Ralston <travisr@matrix.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application services kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants