Skip to content

feat: Add forwarder: ForwarderInfo to EncryptionInfo. #5945

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.

Sign up for GitHub

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

Jump to bottom
Merged
kaylendog merged 7 commits into from
Dec 19, 2025
Merged

feat: Add forwarder: ForwarderInfo to EncryptionInfo. #5945

kaylendog merged 7 commits into from
Dec 19, 2025

Conversation

@kaylendog
Copy link
Contributor

@kaylendog kaylendog commented Dec 10, 2025 •
edited
Loading

Depends on #5943, #5980

  • Public API changes documented in changelogs (optional)

@kaylendog kaylendog force-pushed the kaylendog/history-sharing/encryption-info branch from ae1d3b3 to b99aba0 Compare December 10, 2025 17:09
@kaylendog kaylendog self-assigned this Dec 10, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 10, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #5945 will not alter performance

Comparing kaylendog/history-sharing/encryption-info (b85fc0d) with main (42a5910)

Summary

✅ 50 untouched

@kaylendog kaylendog force-pushed the kaylendog/history-sharing/encryption-info branch from b99aba0 to 78a9811 Compare December 11, 2025 10:13
This was referenced Dec 11, 2025
Draft
Open
@kaylendog kaylendog force-pushed the kaylendog/history-sharing/encryption-info branch from c26b078 to 040681b Compare December 16, 2025 12:11
@codecov
Copy link

codecov bot commented Dec 16, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.59%. Comparing base (42a5910) to head (b85fc0d).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...es/matrix-sdk-common/src/deserialized_responses.rs 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5945   +/-   ##
=======================================
  Coverage   88.58%   88.59%           
=======================================
  Files         364      364           
  Lines      104323   104341   +18     
  Branches   104323   104341   +18     
=======================================
+ Hits        92412    92437   +25     
+ Misses       7546     7538    -8     
- Partials     4365     4366    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kaylendog kaylendog force-pushed the kaylendog/history-sharing/encryption-info branch 3 times, most recently from b58ef8d to 04f17ae Compare December 17, 2025 19:20
@kaylendog kaylendog force-pushed the kaylendog/history-sharing/encryption-info branch from 04f17ae to abc80ff Compare December 17, 2025 19:23
@kaylendog kaylendog marked this pull request as ready for review December 18, 2025 11:34
@kaylendog kaylendog requested review from a team as code owners December 18, 2025 11:34
@kaylendog kaylendog requested review from poljar and removed request for a team December 18, 2025 11:34
kaylendog
kaylendog commented Dec 19, 2025
View reviewed changes
andybalaam
andybalaam requested changes Dec 19, 2025
View reviewed changes
Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Nice, thanks. A couple of minor questions and comments.

crates/matrix-sdk-common/src/deserialized_responses.rs Outdated
/// The device ID of the device that sent us the event, note this is
/// untrusted data unless `verification_state` is `Verified` as well.
pub sender_device: Option<OwnedDeviceId>,
/// If the keys for this message shared-on-invite as part of an
Copy link
Member

@andybalaam andybalaam Dec 19, 2025

Choose a reason for hiding this comment

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

Suggested change
/// If the keys for this message shared-on-invite as part of an
/// If the keys for this message were shared-on-invite as part of an

Copy link
Contributor Author

@kaylendog kaylendog Dec 19, 2025

Choose a reason for hiding this comment

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

Addressed in 0ee914e

crates/matrix-sdk-common/src/deserialized_responses.rs Outdated
/// sent.
///
/// [MSC4268]: https://github.com/matrix-org/matrix-spec-proposals/pull/4268
pub forwarder_device: Option<OwnedDeviceId>,
Copy link
Member

@andybalaam andybalaam Dec 19, 2025

Choose a reason for hiding this comment

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

I think I would prefer a single Option that contains a struct containing these 2 values, since it sounds like they will both be either None or Some in lockstep. I am persuadable though.

Copy link
Contributor Author

@kaylendog kaylendog Dec 19, 2025

Choose a reason for hiding this comment

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

Changed in #5980

crates/matrix-sdk-crypto/src/machine/mod.rs Outdated
known_sender_data.device_id.clone()
}
_ => {
// TODO: Should this return an error?
Copy link
Member

@andybalaam andybalaam Dec 19, 2025

Choose a reason for hiding this comment

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

I don't know. Should this never happen? If so, printing and continuing is probably OK. I think it should be an error rather than a warning though, if we're confident it shouldn't happen.

Copy link
Contributor Author

@kaylendog kaylendog Dec 19, 2025

Choose a reason for hiding this comment

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

The new changes in #5980 let us avoid this, but we still have some optional chaining shenanigans to get around KnownSenderData::device_id being Optional for backwards compatibility.

@andybalaam andybalaam removed the request for review from poljar December 19, 2025 11:46
@kaylendog kaylendog force-pushed the kaylendog/history-sharing/encryption-info branch from a9d9036 to 0ee914e Compare December 19, 2025 15:15
@kaylendog kaylendog changed the title feat: Add forwarder and forwarder_device to EncryptionInfo. feat: Add forwarder: ForwarderInfo to EncryptionInfo. Dec 19, 2025
@kaylendog kaylendog requested a review from andybalaam December 19, 2025 15:35
andybalaam
andybalaam approved these changes Dec 19, 2025
View reviewed changes
Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@kaylendog kaylendog force-pushed the kaylendog/history-sharing/encryption-info branch from 0904b0d to b85fc0d Compare December 19, 2025 17:11
@kaylendog kaylendog enabled auto-merge (squash) December 19, 2025 17:15
@kaylendog kaylendog disabled auto-merge December 19, 2025 17:17
@kaylendog kaylendog enabled auto-merge (squash) December 19, 2025 17:18
@kaylendog kaylendog merged commit cd9f433 into main Dec 19, 2025
52 checks passed
@kaylendog kaylendog deleted the kaylendog/history-sharing/encryption-info branch December 19, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andybalaam andybalaam andybalaam approved these changes

Assignees

@kaylendog kaylendog

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kaylendog @andybalaam