Crypto | Add support for verified identities change detection. #3795
Conversation
BillCarsonFr
force-pushed
the
valere/invisible_crypto/verification_pin
branch
from
3218831
to
7ff2f75
Compare
| @@ -2023,4 +2023,269 @@ pub(crate) mod tests { | |||
|
|
|||
There was a problem hiding this comment.
This commit: "crypto: Verified identity changes TDD" - I would say that the tests for something could be in the same commit as the code itself, so no need for a separate commit here.
If you want a separate commit I suggest "crypto: Tests for verified_latch" or similar.
|
|
||
| pub struct VerificationLatchTestData {} | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Would #[cfg(test)] work? If so, it's better than allowing dead code.
There was a problem hiding this comment.
No it's not, if I add it then the crypto crate won't compile. If I understand well the test config doesn't propagate to other crates, so the annotated code here won't be accessible. I thinking that's why there is the testing cfg? Not sure how to make it work
There was a problem hiding this comment.
Yes, that is why the testing feature exists. It's painful, but better than #[allow(dead_code)]. There is an example in crates/matrix-sdk-crypto/src/lib.rs in the module called testing.
There was a problem hiding this comment.
There are other #[allow(dead_code)] in this file unrelated to the current changes. Will do a follow-up PR to remove them
| own_user_identity.is_identity_signed(&identity).is_ok() | ||
| }); | ||
| if is_verified { | ||
| identity.mark_as_verified_once(); |
There was a problem hiding this comment.
Wait, do the tests in the previous commit fail without this? I would like those tests to be in the same commit that makes them pass, or a later one.
There was a problem hiding this comment.
Now I see why you called the commit "TDD" - because the tests were written before the code! That is awesome during development, but we don't want commits in the history that fail tests because it is much harder to git bisect to find what caused a problem.
There was a problem hiding this comment.
So now, as I can't force push, I have to wait the end of the review, then reset all to re-order the changes and then force push?
Won't all of this squashed at the end?
There was a problem hiding this comment.
No, we don't squash when we merge: after review, we force push a sensible set of commits, then merge with "Rebase and merge".
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3795 +/- ##
==========================================
+ Coverage 84.07% 84.15% +0.07%
==========================================
Files 263 263
Lines 27471 27584 +113
==========================================
+ Hits 23097 23212 +115
+ Misses 4374 4372 -2 ☔ View full report in Codecov by Sentry. |
Clone of identities prevent detect changes due shared Arc. Force serializing/deserializing
BillCarsonFr
force-pushed
the
valere/invisible_crypto/verification_pin
branch
from
66d6e45
to
e715223
Compare
BillCarsonFr
force-pushed
the
valere/invisible_crypto/verification_pin
branch
from
e715223
to
1b05380
Compare
Err, at least this wasn't addressed: #3795 (comment) |
|
Argh. @poljar, @andybalaam, @BillCarsonFr: please remember to document changes in the changelog, especially if they are going to cause breaking changes to the cryptostore such that old apps won't run with new stores. |
Fixes #1129
Signed-off-by: