Skip to content
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

Verification request and QR listener in Crypto V2 #1663

Merged
merged 2 commits into from
Dec 15, 2022

Conversation

Anderas
Copy link
Contributor

@Anderas Anderas commented Dec 14, 2022

Integrate verification signalling which exposes state stream for VerificationRequest and QrCode the same way as was previously done for SAS. This makes it possible to completely remove updatePendingVerification method which manually compares the state of verification objects.

There are a few things that this PR does not address yet and will be resolved in future PR:

  • sprinkling @mainactor in too many places now meaning I'll have to extract common functionality into a dedicated actor type to make concurrency more rigorous
  • calling processOutgoingRequests from multiple places also makes it less obvious when and how this should be called (after sync, after decrypting events, ...)

@Anderas Anderas requested review from a team and alfogrillo and removed request for a team December 14, 2022 20:54
@Anderas Anderas force-pushed the andy/verification_changes branch 2 times, most recently from 4184f5c to 574641d Compare December 15, 2022 09:56
Copy link
Contributor

@alfogrillo alfogrillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
(just left a couple of minor comments)

Comment on lines 34 to 40
if state != oldValue {
log.debug("\(oldValue.description) -> \(state.description)")

DispatchQueue.main.async {
NotificationCenter.default.post(name: .MXKeyVerificationRequestDidChange, object: self)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if state != oldValue {
log.debug("\(oldValue.description) -> \(state.description)")
DispatchQueue.main.async {
NotificationCenter.default.post(name: .MXKeyVerificationRequestDidChange, object: self)
}
}
guard state != oldValue else { return }
log.debug("\(oldValue.description) -> \(state.description)")
DispatchQueue.main.async {
NotificationCenter.default.post(name: .MXKeyVerificationRequestDidChange, object: self)
}

qrCode.state
private(set) var state: MXQRCodeTransactionState = .unknown {
didSet {
if state != oldValue {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same guard consideration

@Anderas Anderas merged commit 77b55bd into develop Dec 15, 2022
@Anderas Anderas deleted the andy/verification_changes branch December 15, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants