Extract key backup engine #1578
Conversation
Anderas
force-pushed
the
andy/backup_engine
branch
from
3ac6866 to
da216b6
Compare
Anderas
requested review from
a team and
aringenbach
and removed request for
a team
Codecov ReportBase: 12.48% // Head: 12.47% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1578 +/- ##
===========================================
- Coverage 12.48% 12.47% -0.02%
===========================================
Files 521 523 +2
Lines 84974 85079 +105
Branches 36180 36226 +46
===========================================
+ Hits 10610 10612 +2
- Misses 73977 74081 +104
+ Partials 387 386 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@aringenbach it seems that codecov has made reviewing this PR quite difficult, in that it annotates almost every line of the refactor. I can disable codecov for the purpose of the review, unless you have some other flow of reviewing (e.g. from local command line) |
There was a problem hiding this comment.
@Anderas No worries I reviewed locally instead.
Code LGTM 👍
| @param crypto the related 'MXCrypto'. | ||
| @param engine backup engine that stores and manages keys | ||
| @param restClient rest client to perform http requests | ||
| @param secretShareManager manages of secrets hsaring |
There was a problem hiding this comment.
| @param secretShareManager manages of secrets hsaring | |
| @param secretShareManager manager of secrets sharing |
As part of implementing key backups with Rust-based CryptoSDK I need to separate out shared and distinct backup functionality in the existing codebase. In the past I have simply duplicated a class (
MXCrypto,MXKeyVerificationManager) and overriden all of its methods, but this approach is not suitable for backups, because a lot more code ends up being shared (state management, enabling/disabling backups, restoring, sending http requests etc).The approach I have chosen instead is to identify all of the code specific to native / legacy implementation and refactor it as new
MXKeyBackupEngineprotocol. At the moment onlyNativeimplementation exists, but in a future PR I will add the rust-based variant. It is absent from this PR to make the refactor cleaner and easier to review.As for the refactor I have copy-pasted entire chunks of code with no or very minimal changes to avoid any regressions. There are some integration tests that cover this functionality, but given the complexity of this code it is best to be extra careful. In reviewing I recommend comparing the deleted and added chunks to confirm there are no behaviour changes.