Spec event edits #1211
Spec event edits #1211
Conversation
There was a problem hiding this comment.
This is a first pass review of the PR, not taking into consideration what the MSC actually says (just relying on what I remember of the MSC).
Most of the comments here are about voice (effectively) as well as general clarity: where possible, I've suggested alternative wording alongside rationale for the change.
I believe this PR is factually accurate enough to ask that this batch of comments be dealt with before receiving a second pass of review, though if you'd prefer to have the second pass first then re-request review and I'll take a look.
|
also, deserving of its own comment: thank you for writing this up 😄 it's clear quite a lot of work went into this, and is well written :) |
Co-authored-by: Travis Ralston <travisr@matrix.org>
There was a problem hiding this comment.
overall this looks good to me. For blockers:
- Splitting out the cleartext description in the relationships section (the wording can stay unchanged in the edits module)
- Consistency on the feature name within the spec
- Stylistic things mentioned as suggestions (could be argued otherwise, though)
| {{% boxes/note %}} | ||
| The payload of an encrypted replacement event must be encrypted as normal, including | ||
| ratcheting any [Megolm](#mmegolmv1aes-sha2) session as normal. The original Megolm | ||
| ratchet entry should **not** be re-used. | ||
| {{% /boxes/note %}} |
There was a problem hiding this comment.
I'm afraid this still doesn't make any sense to me as a reader - we have zero mention of a "ratchet entry" in the spec, and it's not a term I've heard come up in conversation before. I realize this is coming from the MSC, but it also doesn't make sense there having re-read it.
Is this the "ratchet index" (a defined term in the spec) or can we just say it's "encrypted like any other event" and avoid the problem entirely?
There was a problem hiding this comment.
Right, it should be "ratchet value", from https://gitlab.matrix.org/matrix-org/olm/blob/master/docs/megolm.md#the-megolm-ratchet-algorithm.
My general feeling is: if you're implementing edits of encrypted events, you'll know what this means, and if you're not, you don't need to worry about it.
can we just say it's "encrypted like any other event"
Well, at that point, it doesn't seem to give any information at all, so we might as well just omit it.
TBH it seems kindof obvious to me that we wouldn't reuse the old key, so I'd be happy to omit this block, but this text was added in response to @erikjohnston's question at matrix-org/matrix-spec-proposals#2676 (comment). Erik: do you have any thoughts on whether this text is necessary?
There was a problem hiding this comment.
due to lack of comment I'm inclined to believe it's just a safety net thing. It doesn't feel particularly safe to assume that the person implementing edits of encrypted events is also aware of the intricacies of encryption, however I'm happy to go with whatever at this point.
Co-authored-by: Travis Ralston <travisr@matrix.org>
MSC: matrix-org/matrix-spec-proposals#2676
Preview: https://pr1211--matrix-spec-previews.netlify.app