OTPFlow callback pattern#93
Conversation
…r pendingOTPFlow after getOrCreate
…ts, clean up review issues
…ts, extract loadSignerIfNeeded, apiKey factory, OTP callback log
…tion names, rename test file
…params Remove WalletSigner struct (naming conflict with parallel worktree branch that uses WalletSigner as a plain enum). Preserve OTPFlow wiring by adding onAuthRequired as an optional parameter on the existing getWallet/createWallet protocol methods, threaded through resolveEmailSigner in DefaultCrossmintWallets. Also fix afeight-review blockers: flatten performSign duplication in executeSignTransaction, extract makeSendOTPClosure/makeVerifyOTPClosure from makeOTPFlow, move otpCallbackFired log inside the dispatch task, extract startSigningAndCaptureFlow helper in tests, fix try? on sendOTP.
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
Sources/Web/CrossmintTEE.swift:427-433
The `Task { await onAuthRequired(flow) }` fires the callback in an unstructured task that runs concurrently with (and independently of) the stream loop. If the outer sign task is cancelled while this unstructured task is still queued, the stream loop will throw `CancellationError` first, `waitForOTP` returns, the caller cleans up (`pendingOTPFlow = nil`), and only then does the orphaned task run `onAuthRequired` — setting `pendingOTPFlow = flow` with a dead handle. The OTP sheet would appear with a flow whose `verifyOTP`/`cancel` closures are writing to an already-abandoned stream continuation (no-ops), leaving the user stuck.
Calling `await onAuthRequired(flow)` directly is safe here: `onAuthRequired` is expected to be a quick store-and-return operation, the stream and its closures are already set up before the call, and awaiting ensures the callback completes (and any cleanup `pendingOTPFlow = nil` runs) within structured concurrency before the loop begins.
```suggestion
let flow = makeOTPFlow(jwt: jwt, email: email, continuation: streamContinuation)
Logger.tee.debug(LogEvents.otpCallbackFired)
await onAuthRequired(flow)
do {
for try await _ in stream {}
```
### Issue 2 of 2
Tests/WebTests/Helpers/OTPFlowTestHelpers.swift:79-84
**Unsynchronised `@unchecked Sendable` capture class**
`OTPEmailCapture` writes `_value` from `onAuthRequired` (dispatched on `@MainActor`) and reads it from the test body. In this specific test both operations happen on the main actor (the test struct is `@MainActor` and the callback is `@MainActor`), so it works in practice. However, marking the class `@unchecked Sendable` without any synchronisation mechanism (lock, actor, or `nonisolated(unsafe)`) is fragile: if the test is ever moved off `@MainActor` or the callback's isolation changes, this silently becomes a data race. Consider making it an `actor` or using `nonisolated(unsafe) var _value`.
Reviews (1): Last reviewed commit: "Narrow PR to TEE/OTP mechanism, move onA..." | Re-trigger Greptile |
| let flow = makeOTPFlow(jwt: jwt, email: email, continuation: streamContinuation) | ||
| Task { | ||
| Logger.tee.debug(LogEvents.otpCallbackFired) | ||
| await onAuthRequired(flow) | ||
| } | ||
| do { | ||
| let otp = try await withCheckedThrowingContinuation { continuation in | ||
| self.otpContinuation?.resume(throwing: Error.newerSignatureRequested) | ||
| self.otpContinuation = continuation | ||
| self.isOTPRequired = true | ||
| } | ||
| Logger.tee.debug(LogEvents.otpReceived) | ||
| return otp | ||
| } catch CrossmintTEE.Error.userCancelled { | ||
| Logger.tee.warn(LogEvents.otpCancelled) | ||
| throw .userCancelled | ||
| } catch Error.newerSignatureRequested { | ||
| Logger.tee.warn(LogEvents.otpSuperseded) | ||
| throw .newerSignatureRequested | ||
| for try await _ in stream {} |
There was a problem hiding this comment.
The
Task { await onAuthRequired(flow) } fires the callback in an unstructured task that runs concurrently with (and independently of) the stream loop. If the outer sign task is cancelled while this unstructured task is still queued, the stream loop will throw CancellationError first, waitForOTP returns, the caller cleans up (pendingOTPFlow = nil), and only then does the orphaned task run onAuthRequired — setting pendingOTPFlow = flow with a dead handle. The OTP sheet would appear with a flow whose verifyOTP/cancel closures are writing to an already-abandoned stream continuation (no-ops), leaving the user stuck.
Calling await onAuthRequired(flow) directly is safe here: onAuthRequired is expected to be a quick store-and-return operation, the stream and its closures are already set up before the call, and awaiting ensures the callback completes (and any cleanup pendingOTPFlow = nil runs) within structured concurrency before the loop begins.
| let flow = makeOTPFlow(jwt: jwt, email: email, continuation: streamContinuation) | |
| Task { | |
| Logger.tee.debug(LogEvents.otpCallbackFired) | |
| await onAuthRequired(flow) | |
| } | |
| do { | |
| let otp = try await withCheckedThrowingContinuation { continuation in | |
| self.otpContinuation?.resume(throwing: Error.newerSignatureRequested) | |
| self.otpContinuation = continuation | |
| self.isOTPRequired = true | |
| } | |
| Logger.tee.debug(LogEvents.otpReceived) | |
| return otp | |
| } catch CrossmintTEE.Error.userCancelled { | |
| Logger.tee.warn(LogEvents.otpCancelled) | |
| throw .userCancelled | |
| } catch Error.newerSignatureRequested { | |
| Logger.tee.warn(LogEvents.otpSuperseded) | |
| throw .newerSignatureRequested | |
| for try await _ in stream {} | |
| let flow = makeOTPFlow(jwt: jwt, email: email, continuation: streamContinuation) | |
| Logger.tee.debug(LogEvents.otpCallbackFired) | |
| await onAuthRequired(flow) | |
| do { | |
| for try await _ in stream {} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Sources/Web/CrossmintTEE.swift
Line: 427-433
Comment:
The `Task { await onAuthRequired(flow) }` fires the callback in an unstructured task that runs concurrently with (and independently of) the stream loop. If the outer sign task is cancelled while this unstructured task is still queued, the stream loop will throw `CancellationError` first, `waitForOTP` returns, the caller cleans up (`pendingOTPFlow = nil`), and only then does the orphaned task run `onAuthRequired` — setting `pendingOTPFlow = flow` with a dead handle. The OTP sheet would appear with a flow whose `verifyOTP`/`cancel` closures are writing to an already-abandoned stream continuation (no-ops), leaving the user stuck.
Calling `await onAuthRequired(flow)` directly is safe here: `onAuthRequired` is expected to be a quick store-and-return operation, the stream and its closures are already set up before the call, and awaiting ensures the callback completes (and any cleanup `pendingOTPFlow = nil` runs) within structured concurrency before the loop begins.
```suggestion
let flow = makeOTPFlow(jwt: jwt, email: email, continuation: streamContinuation)
Logger.tee.debug(LogEvents.otpCallbackFired)
await onAuthRequired(flow)
do {
for try await _ in stream {}
```
How can I resolve this? If you propose a fix, please make it concise.| for await flow in flowStream { receivedFlow = flow } | ||
| return (signTask, receivedFlow) | ||
| } | ||
| } | ||
|
|
||
| final class OTPEmailCapture: @unchecked Sendable { |
There was a problem hiding this comment.
Unsynchronised
@unchecked Sendable capture class
OTPEmailCapture writes _value from onAuthRequired (dispatched on @MainActor) and reads it from the test body. In this specific test both operations happen on the main actor (the test struct is @MainActor and the callback is @MainActor), so it works in practice. However, marking the class @unchecked Sendable without any synchronisation mechanism (lock, actor, or nonisolated(unsafe)) is fragile: if the test is ever moved off @MainActor or the callback's isolation changes, this silently becomes a data race. Consider making it an actor or using nonisolated(unsafe) var _value.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Tests/WebTests/Helpers/OTPFlowTestHelpers.swift
Line: 79-84
Comment:
**Unsynchronised `@unchecked Sendable` capture class**
`OTPEmailCapture` writes `_value` from `onAuthRequired` (dispatched on `@MainActor`) and reads it from the test body. In this specific test both operations happen on the main actor (the test struct is `@MainActor` and the callback is `@MainActor`), so it works in practice. However, marking the class `@unchecked Sendable` without any synchronisation mechanism (lock, actor, or `nonisolated(unsafe)`) is fragile: if the test is ever moved off `@MainActor` or the callback's isolation changes, this silently becomes a data race. Consider making it an `actor` or using `nonisolated(unsafe) var _value`.
How can I resolve this? If you propose a fix, please make it concise.
The previous OTP pattern exposed
isOTPRequired,provideOTP, andcancelOTPdirectly onCrossmintSDK, requiring callers to observe state changes and coordinate the flow manually. This replaces it with a flow object passed to a callback: when a wallet operation needs email authentication, the SDK callsonAuthRequiredwith anOTPFlowthe caller drives directly. The wallet operation suspends untilverifyOTPorcancelis called.Passing
pendingOTPFlowtosheet(item:)works out of the box sinceOTPFlowconforms toIdentifiablevia a UUID, so each new signing operation presents a fresh sheet.Changes
OTPFlow(Web): new public struct withemail,sendOTP,verifyOTP,cancel, and a UUIDid.CrossmintTEE:signTransactionnow acceptsonAuthRequired. On a new device it calls the closure with anOTPFlowand suspends until the user verifies or cancels.CrossmintWallets:getWalletandcreateWalletgain an optionalonAuthRequiredparameter (defaultnil). Existing callers are unaffected.DefaultCrossmintWallets: newresolveEmailSignerhelper that rebuilds an email signer with the provided callback when needed.EVMEmailSigner,SolanaEmailSigner,StellarEmailSigner): inits narrowed tointernal;onAuthRequiredis typed@MainActor.OTPValidatorViewdisables input after successful verification to prevent double-submission.AppStateuses the newonAuthRequiredparameter style.CrossmintTEEOTPFlowTestscovering success, cancel, resend, no-callback error, and email forwarding. Setup extracted toCrossmintTEEOTPFlowFixture.