-
Notifications
You must be signed in to change notification settings - Fork 260
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
crypto: Replace the implementation of get_verification_state with SenderDataFinder #3772
Conversation
0d9cf72
to
282320f
Compare
Pasting the patch for 282320f in case it ever disappears. This code was temporary code to validate the |
282320f
to
285f3ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow every change, but it overall LGTM.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3772 +/- ##
==========================================
- Coverage 84.09% 84.01% -0.08%
==========================================
Files 259 259
Lines 27109 27145 +36
==========================================
+ Hits 22797 22806 +9
- Misses 4312 4339 +27 ☔ View full report in Codecov by Sentry. |
) | ||
} | ||
} | ||
SenderData::UnknownDevice { owner_check_failed: true, .. } => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shame that we can't put this into a From
implementation, would cement down the fact that these two are parts of the same coin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I considered that, and in a future PR I moved it into a function, but it's (SenderData, is_imported) -> (VerificationState, DeviceId)
so it doesn't quite fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I saw that is_imported
messes everything up 😢.
This avoids repeating equivalent code.
89bcd6c
to
fedd813
Compare
Replace the implementation of
get_verification_state
withSenderDataFinder
, since they are now equivalent.Part of #3751
As part of developing this PR, I created (and then removed) this commit: 282320f which proves that for every case, converting from
SenderDataFinder
's result into aVerificationState
and anOption<DeviceId>
produces the same result as the oldget_verification_state
code, so we can safely replace it.