Convert verification request and transaction to protocols #1528
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Anderas
force-pushed
the
andy/1_verification_protocols
branch
from
a860cea
to
d75c881
Compare
Anderas
requested review from
a team and
stefanceriu
and removed request for
a team
Anderas
commented
Jul 18, 2022
| @@ -63,13 +63,13 @@ @interface MXKeyVerificationManager () | |||
| dispatch_queue_t cryptoQueue; | |||
|
|
|||
| // All running transactions | |||
| MXUsersDevicesMap<MXKeyVerificationTransaction*> *transactions; | |||
| MXUsersDevicesMap<MXDefaultKeyVerificationTransaction *> *transactions; | |||
There was a problem hiding this comment.
MXKeyVerificationManager is accessing some of the internal methods of request and transaction which are defined in _Private.h header files and not exposed in the protocol. For this reason it is still using implementation types rather than protocol types
Codecov Report
@@ Coverage Diff @@
## develop #1528 +/- ##
========================================
Coverage 12.04% 12.04%
========================================
Files 512 512
Lines 83822 83822
Branches 35786 35786
========================================
+ Hits 10097 10099 +2
+ Misses 73357 73355 -2
Partials 368 368
Continue to review full report at Codecov.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As part of an ongoing work to implement Rust-based device verification I need to prepare the ground by converting existing
MXKeyVerificationRequest,MXKeyVerificationTransactionandMXSASTransactionclasses into protocols, so that future PRs can implement these protocols withMatrixCryptoSDKobjects. This is not a full list of classes that need to be hidden behind an interface, only those directly needed for SAS flow.I have previously taken a subclassing approach to solve a similar problem, but this was because refactoring existing code for
MXCryptoclass was too risky without extensive test coverage and it was best to leave this untouched. Swapping request and transaction objects for protocols should be safer, and now validated by some newly enabled integration tests. Subclassing existing implementation on the other hand leaves more technical debt behind and does not safeguard against some methods not being overriden.Regarding naming, I considered both alternatives where protocol adopts existing name (e.g.
MXKeyVerificationRequest) and subclasses extend this (e.g. Default, V2 ...), as well as naming the protocolMXKeyVerificationRequestProtocoland leaving the implementation names intact. Reviewing the SDK I did not find many examples of addingProtocolto protocol definitions so I opted for the first naming variant to preserve consistency. This choice also means that only objective-c files need to perform renames, whereas swift files remain unchanged.