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

Added shared_history flag to room key events #1669

Merged
merged 9 commits into from Oct 6, 2023

Conversation

Michael-Hollister
Copy link
Contributor

@Michael-Hollister Michael-Hollister commented Oct 2, 2023

Fixes #1104

Added shared_history flag for MSC 3061 implementation. See following PR for more details: matrix-org/matrix-rust-sdk#2650

Signed-off-by: Michael Hollister michael@futo.org


Preview Removed

Signed-off-by: Michael Hollister <michael@futo.org>
Copy link
Contributor

@zecakeh zecakeh left a comment

Choose a reason for hiding this comment

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

Thanks!

We should also put this new field behind a cargo feature, like other non-merged MSCs. Please expose that feature in the main ruma crate and add it to the __ci cargo feature.

crates/ruma-events/src/forwarded_room_key.rs Outdated Show resolved Hide resolved
crates/ruma-events/src/room_key.rs Outdated Show resolved Hide resolved
@Michael-Hollister
Copy link
Contributor Author

Thanks!

We should also put this new field behind a cargo feature, like other non-merged MSCs. Please expose that feature in the main ruma crate and add it to the __ci cargo feature.

Before feature-gating the changes, I would like to confirm with you I should do this, since the MSC is merged into the spec per my understanding: matrix-org/matrix-spec-proposals#3061. Also, the reason why I renamed the shared_history fields is for interoperability with the JS SDK that uses the MSC namespacing: https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/crypto/algorithms/megolm.ts#L127C24-L127C24. Otherwise I would have not renamed the fields.

So to confirm for this situation:

  1. Do you recommend in this case to keep the field names renamed to the MSC prefix? Or at this point should the JS SDK change to remove the MSC prefix for these events?
  2. Given that the MSC is merged, should I still feature-gate the changes?

Signed-off-by: Michael Hollister <michael@futo.org>
@zecakeh
Copy link
Contributor

zecakeh commented Oct 3, 2023

My understanding is that a MSC is "merged" into the spec only when it has had a Spec PR that was actually merged into the matrix-spec repo. The current state of this MSC in the MSC process is that it has been "accepted", i.e. it has passed FCP and has been merged into the matrix-spec-proposals repo.

It doesn't seem to be on the SCT roadmap so I doubt the spec PR is going to be made soon. However if there is a reason why the spec PR is not to be created the MSC should have a "blocked" label, which it does not. It might be worth asking on the MSC or in the SCT office if there is a reason it is not on the roadmap. The reservations from the rust SDK crypto team might be why it is not planned to be merged in its current state.

Being unsure of the future of the MSC is reason enough for me to keep using the unstable prefix and put it behind a cargo feature.

@Michael-Hollister
Copy link
Contributor Author

My understanding is that a MSC is "merged" into the spec only when it has had a Spec PR that was actually merged into the matrix-spec repo. The current state of this MSC in the MSC process is that it has been "accepted", i.e. it has passed FCP and has been merged into the matrix-spec-proposals repo.

It doesn't seem to be on the SCT roadmap so I doubt the spec PR is going to be made soon. However if there is a reason why the spec PR is not to be created the MSC should have a "blocked" label, which it does not. It might be worth asking on the MSC or in the SCT office if there is a reason it is not on the roadmap. The reservations from the rust SDK crypto team might be why it is not planned to be merged in its current state.

Being unsure of the future of the MSC is reason enough for me to keep using the unstable prefix and put it behind a cargo feature.

Thanks for the clarification! I'll go ahead and put this behind a cargo feature then.

@zecakeh
Copy link
Contributor

zecakeh commented Oct 4, 2023

So… it looks like the spec PR has been created: matrix-org/matrix-spec#1655.

So I believe we would accept either:

  1. Using the stable prefix without a cargo feature. We would wait until the spec PR is merged to merge this one, just to be sure.
  2. Using the unstable prefix behind a cargo feature. We could merge this as soon as it is ready. It can be updated later when the spec PR is merged and the JS SDK supports the stable prefix for example.

Signed-off-by: Michael Hollister <michael@futo.org>
@Michael-Hollister
Copy link
Contributor Author

Alright, I went ahead and added the changes behind the cargo feature. In the event that MSC3061 gets merged in the spec, it should be easy to revert the commit. And if not, then other projects depending on ruma can decide whether to include the feature.

Copy link
Contributor

@zecakeh zecakeh left a comment

Choose a reason for hiding this comment

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

The cargo feature is good now. I didn't notice before that you were making breaking changes to the API where we usually don't (also that makes the feature non-additive).

crates/ruma-events/src/forwarded_room_key.rs Outdated Show resolved Hide resolved
crates/ruma-events/src/forwarded_room_key.rs Outdated Show resolved Hide resolved
crates/ruma-events/src/forwarded_room_key.rs Outdated Show resolved Hide resolved
crates/ruma-events/src/room_key.rs Outdated Show resolved Hide resolved
crates/ruma-events/src/room_key.rs Outdated Show resolved Hide resolved
crates/ruma-events/src/room_key.rs Outdated Show resolved Hide resolved
Michael-Hollister and others added 6 commits October 5, 2023 12:42
Co-authored-by: Kévin Commaille <76261501+zecakeh@users.noreply.github.com>
Co-authored-by: Kévin Commaille <76261501+zecakeh@users.noreply.github.com>
Co-authored-by: Kévin Commaille <76261501+zecakeh@users.noreply.github.com>
Co-authored-by: Kévin Commaille <76261501+zecakeh@users.noreply.github.com>
Signed-off-by: Michael Hollister <michael@futo.org>
Signed-off-by: Michael Hollister <michael@futo.org>
@zecakeh zecakeh merged commit 7b898bf into ruma:main Oct 6, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Implement MSC3061 (Sharing room keys for past messages)
2 participants