feat(crypto): Add optional withheld reason to `UnableToDecryptReason` #4305

BillCarsonFr · 2024-11-21T10:56:28Z

Fixes #4270

Public API changes documented in changelogs (optional)

Signed-off-by:

crates/matrix-sdk-common/src/deserialized_responses.rs

codecov · 2024-11-28T09:09:32Z

Codecov Report

Attention: Patch coverage is 77.27273% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.11%. Comparing base (a4434d7) to head (6801811).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...es/matrix-sdk-crypto/src/types/events/utd_cause.rs	20.00%	4 Missing ⚠️
...es/matrix-sdk-common/src/deserialized_responses.rs	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4305      +/-   ##
==========================================
- Coverage   85.15%   85.11%   -0.05%     
==========================================
  Files         280      280              
  Lines       30826    30837      +11     
==========================================
- Hits        26251    26248       -3     
- Misses       4575     4589      +14

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

richvdh

Looks like a sensible direction to me; a few comments though

Mostly I'd like to know if we can avoid introducing yet another type

crates/matrix-sdk-common/src/deserialized_responses.rs

crates/matrix-sdk-crypto/src/types/events/utd_cause.rs

crates/matrix-sdk-common/src/deserialized_responses.rs

richvdh · 2024-12-02T16:51:37Z

crates/matrix-sdk-common/src/deserialized_responses.rs

+// Wrapper around `Box<str>` that cannot be used in a meaningful way outside of
+// this crate. Used for string enums because their `_Custom` variant can't be


cannot be used in a meaningful way outside of this crate

It's used in matrix-sdk-crypto, so that doesn't seem right?

Yeah, that seems like a copy-paste error.

Perhaps we should just remove that line.

richvdh · 2024-12-02T16:56:39Z

crates/matrix-sdk-common/src/deserialized_responses.rs

+// XXX had to make the field public to make ruma `StringEnum` macro happy.
+pub struct PrivOwnedStr(pub Box<str>);


It strikes me it's not really a PrivOwnedStr if it's pub.

I don't really understand what's going on here (why does it need to be pub now when it didn't before?), but I see you discussed this with @poljar, so I'm happy if he's happy. @poljar can you confirm this is what you were envisaging?

Yes nothing makes sense with the struct anymore (the name, the doc, ..)
Not sure how to properly document it though.

The Ruma macro expects the type to have this name, it's not something that is in under our control...

The alternatives are to not use the Ruma macro or to have multiple copies of this struct.

ok, fair enough. It would be helpful if this could be explained in a comment.

Why does this need to be pub, when it didn't need to be pub when it lived in the crypto crate?

1f50e4b

when it didn't need to be pub when it lived in the crypto crate

Because it is also used by other enums (still in crypto) that now uses the one that is in common.

Actually there was another copy left 2151fcd

I would be ok to revert that and have multiple copies if it makes it simpler

Why does this need to be pub, when it didn't need to be pub when it lived in the crypto crate?

Because otherwise you can't construct the variant outside of the crate where the type lives unless it's pub. Now I don't exactly know where that is needed but if we remove the pub the compiler might tell us.

For EventEncryptionAlgorithm

error[E0616]: field `0` of struct `matrix_sdk_common::deserialized_responses::PrivOwnedStr` is private --> crates/matrix-sdk-crypto/src/types/mod.rs:430:49 | 430 | #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, StringEnum)] | ^^^^^^^^^^ private field | = note: this error originates in the derive macro `StringEnum` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0423]: cannot initialize a tuple struct which contains private fields

crates/matrix-sdk-crypto/src/types/events/utd_cause.rs

crates/matrix-sdk-common/src/deserialized_responses.rs

richvdh

LGTM now, with the exception of PrivOwnedStr, for which I defer to @poljar

fixed

poljar

Left a small nit, I think the rest looks good.

crates/matrix-sdk-crypto/src/types/events/utd_cause.rs

poljar · 2024-12-03T14:44:43Z

crates/matrix-sdk-common/src/deserialized_responses.rs

+// XXX had to make the field public to make ruma `StringEnum` macro happy.
+pub struct PrivOwnedStr(pub Box<str>);


The Ruma macro expects the type to have this name, it's not something that is in under our control...

The alternatives are to not use the Ruma macro or to have multiple copies of this struct.

poljar · 2024-12-03T14:46:55Z

crates/matrix-sdk-common/src/deserialized_responses.rs

+// Wrapper around `Box<str>` that cannot be used in a meaningful way outside of
+// this crate. Used for string enums because their `_Custom` variant can't be


Yeah, that seems like a copy-paste error.

Perhaps we should just remove that line.

Adds new UtdCause variants for withheld keys, enabling applications to display customised messages when an Unable-To-Decrypt message is expected. refactor(crypto): Move WithheldCode from crypto to common crate

BillCarsonFr commented Nov 21, 2024

View reviewed changes

crates/matrix-sdk-common/src/deserialized_responses.rs Outdated Show resolved Hide resolved

BillCarsonFr force-pushed the valere/support_for_withheld_reason branch from 9abe4ba to ea5fcdb Compare November 28, 2024 08:54

BillCarsonFr marked this pull request as ready for review November 28, 2024 08:54

BillCarsonFr requested review from a team as code owners November 28, 2024 08:54

BillCarsonFr requested review from bnjbvr and richvdh and removed request for a team November 28, 2024 08:54

poljar requested review from poljar and removed request for bnjbvr November 28, 2024 09:12

richvdh requested changes Nov 28, 2024

View reviewed changes

BillCarsonFr requested a review from richvdh December 2, 2024 09:29

richvdh reviewed Dec 2, 2024

View reviewed changes

crates/matrix-sdk-common/src/deserialized_responses.rs Show resolved Hide resolved

crates/matrix-sdk-common/src/deserialized_responses.rs Outdated Show resolved Hide resolved

BillCarsonFr mentioned this pull request Dec 2, 2024

feat(crypto): Support new expected UTD causes UX + Analytics element-hq/element-x-android#3980

Merged

10 tasks

richvdh previously requested changes Dec 2, 2024

View reviewed changes

BillCarsonFr mentioned this pull request Dec 3, 2024

dependencies : update rust sdk to 0.2.68 element-hq/element-x-android#3988

Merged

10 tasks

BillCarsonFr requested a review from richvdh December 3, 2024 14:21

richvdh reviewed Dec 3, 2024

View reviewed changes

poljar requested changes Dec 3, 2024

View reviewed changes

BillCarsonFr requested review from richvdh and poljar December 3, 2024 17:04

poljar approved these changes Dec 4, 2024

View reviewed changes

BillCarsonFr force-pushed the valere/support_for_withheld_reason branch from 2151fcd to 8f8cd17 Compare December 4, 2024 12:07

feat(crypto): Supports new UtdCause variants for withheld keys

6801811

Adds new UtdCause variants for withheld keys, enabling applications to display customised messages when an Unable-To-Decrypt message is expected. refactor(crypto): Move WithheldCode from crypto to common crate

BillCarsonFr force-pushed the valere/support_for_withheld_reason branch from f09b3e4 to 6801811 Compare December 4, 2024 14:33

BillCarsonFr enabled auto-merge December 4, 2024 14:35

BillCarsonFr merged commit 1009ea8 into main Dec 4, 2024
39 of 40 checks passed

BillCarsonFr deleted the valere/support_for_withheld_reason branch December 4, 2024 14:57

BillCarsonFr mentioned this pull request Dec 6, 2024

Report withheld messages to posthog using RoomKeysWithheldForUnverifiedDevice code element-hq/element-meta#2589

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(crypto): Add optional withheld reason to `UnableToDecryptReason` #4305

feat(crypto): Add optional withheld reason to `UnableToDecryptReason` #4305

BillCarsonFr commented Nov 21, 2024

codecov bot commented Nov 28, 2024 •

edited

Loading

richvdh left a comment

richvdh Dec 2, 2024

poljar Dec 3, 2024

richvdh Dec 2, 2024

BillCarsonFr Dec 3, 2024

poljar Dec 3, 2024

richvdh Dec 3, 2024

BillCarsonFr Dec 3, 2024 •

edited

Loading

BillCarsonFr Dec 3, 2024

poljar Dec 4, 2024

BillCarsonFr Dec 4, 2024

richvdh left a comment

poljar left a comment

poljar Dec 3, 2024

poljar Dec 3, 2024

		// Wrapper around `Box<str>` that cannot be used in a meaningful way outside of
		// this crate. Used for string enums because their `_Custom` variant can't be

		// XXX had to make the field public to make ruma `StringEnum` macro happy.
		pub struct PrivOwnedStr(pub Box<str>);

feat(crypto): Add optional withheld reason to UnableToDecryptReason #4305

feat(crypto): Add optional withheld reason to UnableToDecryptReason #4305

Conversation

BillCarsonFr commented Nov 21, 2024

codecov bot commented Nov 28, 2024 • edited Loading

Codecov Report

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillCarsonFr Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

poljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(crypto): Add optional withheld reason to `UnableToDecryptReason` #4305

feat(crypto): Add optional withheld reason to `UnableToDecryptReason` #4305

codecov bot commented Nov 28, 2024 •

edited

Loading

BillCarsonFr Dec 3, 2024 •

edited

Loading