Add variants to SenderData to represent different verification states

[uhoreg]

as previously discussed

• Public API changes documented in changelogs (optional)

Signed-off-by:

[uhoreg]

One test is known to fail because I need to work on migrating data. And I probably need to write a changelog. But otherwise this PR should be reviewable, and I wanted to get it out so that I'm not holding up Andy's work.

[andybalaam]

Looks good to me. My only question is whether the already-incorrect doc comment on is_known was actually describing the behaviour some other code expected.

We also need to think about migration, as you mention.

[andybalaam]

It looks like this comment was wrong before, but definitely needs updating now.

[uhoreg]

Looking at this function, it looks like it's only used in OlmMachine to determine whether it should recalculate the sender_data. So I definitely don't want it to return true if it's SenderUnverifiedButPreviouslyVerified. But I'm not sure that it should return true if it's SenderUnverified either, although that would match the previous behaviour, since sender_data_to_verification_state returns different values if the sender is verified or not. What do you think? What was your rationale for having it return true when the sender was unverified?

[andybalaam]

We agreed at some point that we should not update old sessions that were marked as unverified, even if the identity later became verified. Since this function's purpose is to decide whether to recalculate SenderData, it should return true for SenderUnverified.

So the current behaviour is "correct", and we just need a way to describe it, I think.

So to be absolutely clear I think it's:

match self {
    SenderUnverified | SenderVerified => true,
    UnknownDevice | DeviceInfo | SenderUnverifiedButPreviouslyVerified => false,
}

Maybe this function should be moved inside OlmMachine and be named should_recalculate?

[andybalaam]

I'm happy for you to merge this as-is or with one of the changes I mention above. If you want to change the behaviour I'll want to discuss it further.

[uhoreg]

Maybe this function should be moved inside OlmMachine and be named should_recalculate?

Done. It's ended up adding a bunch of negations, which I'm not really a fan of, but to avoid the negations, I'd either need to rename to should_not_recalculate, or switch the logic in the place where it's called, both of which seem at least as bad.

If you're OK with the way it is, can you hit the merge button? I don't have permissions.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.

Project coverage is 84.11%. Comparing base (ed19bf7) to head (a608cd3).
Report is 25 commits behind head on main.

Files	Patch %	Lines
...x-sdk-crypto/src/olm/group_sessions/sender_data.rs	89.28%	3 Missing ⚠️
...rypto/src/olm/group_sessions/sender_data_finder.rs	71.42%	2 Missing ⚠️
crates/matrix-sdk-crypto/src/machine/mod.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3856   +/-   ##
=======================================
  Coverage   84.10%   84.11%           
=======================================
  Files         262      262           
  Lines       27665    27698   +33     
=======================================
+ Hits        23267    23297   +30     
- Misses       4398     4401    +3     

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

[andybalaam]

I have reviewed this commit too and approve.

[andybalaam]

I wanted to make some very minor changes, so closing in favour of #3877