Skip to content

Conversation

andybalaam
Copy link
Member

@andybalaam andybalaam commented Dec 16, 2024

Part of element-hq/element-meta#2665

On the receiving side, allow sender_device_keys as well as org.matrix.msc4147.device_keys for the sender's device keys in to-device event. Once this has time to bed in, we will change the sending side to use this stable identifier since the MSC has been merged.

  • Public API changes documented in changelogs (optional)

@andybalaam andybalaam force-pushed the andybalaam/correct-name-of-sender_device_keys branch from 5bc353e to 723cb6a Compare December 16, 2024 17:23
@andybalaam andybalaam marked this pull request as ready for review December 16, 2024 17:25
@andybalaam andybalaam requested review from a team as code owners December 16, 2024 17:25
@andybalaam andybalaam requested review from BillCarsonFr and bnjbvr and removed request for a team December 16, 2024 17:25
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.42%. Comparing base (bc8c4f5) to head (e557586).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...rates/matrix-sdk-crypto/src/types/events/olm_v1.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4420   +/-   ##
=======================================
  Coverage   85.42%   85.42%           
=======================================
  Files         283      283           
  Lines       31556    31556           
=======================================
  Hits        26958    26958           
  Misses       4598     4598           

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

/// The recipient's signing keys of the encrypted event.
pub recipient_keys: OlmV1Keys,
/// The device keys if supplied as per MSC4147
#[serde(rename = "org.matrix.msc4147.device_keys")]
Copy link
Member

Choose a reason for hiding this comment

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

I tought that the trick was to use serde(alias = ?
how is it working here?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, unless I'm missing something, this is only renaming the variable when it's used on the Rust side, so this is an internal renaming, so this may not be what you intended to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Err, it's renaming the field during serialization/deserialization. The serialized JSON will contain org.matrix.msc4147.device_keys during serialization and we will expect org.matrix.msc4147.device_keys during deserialization. While the code have the more pleasant name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well spotted! A fix, and, critically, a test that proves it works, are in c093e7c

/// The recipient's signing keys of the encrypted event.
pub recipient_keys: OlmV1Keys,
/// The device keys if supplied as per MSC4147
#[serde(rename = "org.matrix.msc4147.device_keys")]
Copy link
Member

Choose a reason for hiding this comment

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

Yep, unless I'm missing something, this is only renaming the variable when it's used on the Rust side, so this is an internal renaming, so this may not be what you intended to do?

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

We do use mostly "deserialize" in this simplified en-us code base, but that might not be review comment worthy. LGTM :)

@andybalaam
Copy link
Member Author

We do use mostly "deserialize" in this simplified en-us code base, but that might not be review comment worthy. LGTM :)

Thanks, fixed in 17f074b

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

Thx, LGTM.

@andybalaam andybalaam force-pushed the andybalaam/correct-name-of-sender_device_keys branch from 17f074b to e557586 Compare December 19, 2024 15:06
@andybalaam andybalaam enabled auto-merge (rebase) December 19, 2024 15:07
@andybalaam andybalaam merged commit e4712be into main Dec 19, 2024
40 checks passed
@andybalaam andybalaam deleted the andybalaam/correct-name-of-sender_device_keys branch December 19, 2024 15:27
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.

4 participants