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

task(crypto): Support receiving stable identifier for MSC4147 #4420

Merged
merged 1 commit into from
Dec 19, 2024

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 bnjbvr and BillCarsonFr 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.

@@ -177,7 +177,7 @@ where
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

@@ -177,7 +177,7 @@ where
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