Add support for scanning QR codes during verification, with Rust crypto #3565
Conversation
richvdh
added
the
T-Enhancement
Normally, the application specifies the supported verification methods when creating the MatrixClient (and matrix-react-sdk does so). If the application leaves it unset, then the idea is that the js-sdk offers all known verification methods. However, by default, the rust-sdk doesn't specify `m.qr_code.scan.v1`. So basically, we need to set our own list of supported methods, rather than relying on the rust-sdk's defaults.
richvdh
force-pushed
the
rav/element-r/38_scan_qr_code
branch
from
a1f8ab7
to
5d2f7bc
Compare
richvdh
force-pushed
the
rav/element-r/38_scan_qr_code
branch
from
5d2f7bc
to
21bc652
Compare
| * | ||
| * If `undefined`, we will offer all the methods supported by the Rust SDK. | ||
| */ | ||
| public supportedVerificationMethods: string[] | undefined; |
There was a problem hiding this comment.
Looks like a breaking change, any reason to not use a getter+setter?
There was a problem hiding this comment.
this isn't a public class, so it's not a breaking change.
There was a problem hiding this comment.
What makes it not public? Where is it marked as internal or experimental?
There was a problem hiding this comment.
it's just not exported through index.ts or browser-index.ts (ie, the public interface of the js-sdk).
There was a problem hiding this comment.
Neither are a bunch of the things necessary to use the SDK so I don't think that's the rule. (Namely the Typed EventEmitter events which are necessary to set up listeners0.
There was a problem hiding this comment.
Neither are a bunch of the things necessary to use the SDK so I don't think that's the rule. (Namely the Typed EventEmitter events which are necessary to set up listeners0.
Not quite sure what you're referring to, but anything not being exported through index.js that we expect clients to be able to refer to is very definitely a bug.
There was a problem hiding this comment.
Not quite sure what you're referring to
There was a problem hiding this comment.
yes well, that's a bug, and there's the issue to track it. Great.
The point remains that (a) if anybody has tried using rust crypto at all outside the react-sdk I will eat my hat; (b) even if they have, using RustCrypto or RustVerificationRequest directly is an application bug. I won't obfuscate this API just to support someone relying on the internals of the library in this way.
There was a problem hiding this comment.
#3574 then, if my argument isn't convincing.
There was a problem hiding this comment.
This is now an @internal class.
| @@ -56,17 +56,18 @@ export class RustVerificationRequest | |||
| public constructor( | |||
| private readonly inner: RustSdkCryptoJs.VerificationRequest, | |||
| private readonly outgoingRequestProcessor: OutgoingRequestProcessor, | |||
| private readonly supportedVerificationMethods: string[] | undefined, | |||
| private readonly supportedVerificationMethods: string[], | |||
richvdh
requested review from
a team and
florianduros
and removed request for
a team
Before I change displaying QR codes, I need an end-to-end test for it. And before I can write an end-to-end test for it, I need to be able to scan QR codes. So here is support in the js-sdk for scanning QR codes. It will only work with Rust crypto, but that's enough for a Cypress test, and Rust is the future.
Suggest review commit-by-commit.
Here's what your changelog entry will look like:
✨ Features