-
Notifications
You must be signed in to change notification settings - Fork 35
fix: cache auth status in NonCustodialSigner to prevent repeated OTP prompts #1774
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
base: main
Are you sure you want to change the base?
Changes from all commits
ae7b9ba
58e3a22
e07c352
cefc913
a89e580
e9a8e56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| "@crossmint/wallets-sdk": patch | ||
| --- | ||
|
|
||
| fix: cache auth status in NonCustodialSigner to prevent repeated OTP prompts on React Native | ||
|
|
||
| Added a timestamp-based cache (10-min TTL) in `handleAuthRequired()` that short-circuits | ||
| the `get-status` round-trip to the frame when the signer was recently confirmed as ready. | ||
| This prevents unnecessary OTP re-prompts caused by frame cache expiry or JWT token refresh. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,9 +13,26 @@ import { validateAPIKey, WithLoggerContext } from "@crossmint/common-sdk-base"; | |
| import type { SignerOutputEvent } from "@crossmint/client-signers"; | ||
| import { walletsLogger } from "../../logger"; | ||
|
|
||
| // Client-side TTL for caching a successful "ready" status from the frame. | ||
| // Avoids redundant get-status round-trips that can trigger unnecessary OTP | ||
| // prompts when the frame's own cache has expired or the JWT was refreshed. | ||
| const AUTH_CACHE_TTL_MS = 10 * 60 * 1000; // 10 minutes | ||
|
|
||
| export abstract class NonCustodialSigner implements SignerAdapter { | ||
| public readonly type: "email" | "phone"; | ||
| private _needsAuth = true; | ||
| private _lastAuthSuccessTimestamp = 0; | ||
|
|
||
| /** | ||
| * Resets the client-side auth cache so the next operation will re-check | ||
| * signer status with the frame. Subclasses should call this when they | ||
| * receive an auth-related error during signing (e.g. frame was silently | ||
| * reloaded and lost its master secret). | ||
| */ | ||
| protected invalidateAuthCache(): void { | ||
| this._needsAuth = true; | ||
| this._lastAuthSuccessTimestamp = 0; | ||
| } | ||
| private _authPromise: { | ||
| promise: Promise<void>; | ||
| resolve: () => void; | ||
|
|
@@ -124,6 +141,16 @@ export abstract class NonCustodialSigner implements SignerAdapter { | |
| ); | ||
| } | ||
|
|
||
| // Skip the get-status round-trip if we recently confirmed the signer is ready. | ||
| // This prevents unnecessary OTP prompts caused by frame cache expiry or JWT refresh. | ||
| const timeSinceLastAuth = Date.now() - this._lastAuthSuccessTimestamp; | ||
| if (!this._needsAuth && timeSinceLastAuth < AUTH_CACHE_TTL_MS) { | ||
| walletsLogger.info("get-status: skipping, recently authenticated", { | ||
| timeSinceLastAuthMs: timeSinceLastAuth, | ||
| }); | ||
| return; | ||
| } | ||
|
Comment on lines
+144
to
+152
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If the WebView is silently reloaded (or the frame loses its master secret for any reason) within the 10-minute window, this check will keep short-circuiting and all subsequent Because // Example: expose a reset helper so subclasses can call it on sign failure
protected invalidateAuthCache(): void {
this._needsAuth = true;
this._lastAuthSuccessTimestamp = 0;
}Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/wallets/src/signers/non-custodial/ncs-signer.ts
Line: 133-141
Comment:
**No path to invalidate the cache after a sign failure**
If the WebView is silently reloaded (or the frame loses its master secret for any reason) within the 10-minute window, this check will keep short-circuiting and all subsequent `sign()` calls will fail — with no mechanism to break out of that stuck state.
Because `_lastAuthSuccessTimestamp` and `_needsAuth` are both `private`, subclass sign methods cannot reset them on failure. A retry-with-auth pattern would require either making these accessible to subclasses (e.g., `protected`), or exposing a `protected invalidateAuthCache()` helper that subclasses can call when they receive an auth-related error from the frame. Without this, users are stuck for up to 10 minutes until the TTL expires.
```typescript
// Example: expose a reset helper so subclasses can call it on sign failure
protected invalidateAuthCache(): void {
this._needsAuth = true;
this._lastAuthSuccessTimestamp = 0;
}
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point from the cold-blooded code reviewer 🦎 — this is a real gap. Added a |
||
|
|
||
| // Determine if we need to authenticate the user via OTP or not | ||
| walletsLogger.info("get-status: sending request"); | ||
| const startTime = Date.now(); | ||
|
|
@@ -156,9 +183,11 @@ export abstract class NonCustodialSigner implements SignerAdapter { | |
|
|
||
| if (signerResponse.signerStatus === "ready") { | ||
| this._needsAuth = false; | ||
| this._lastAuthSuccessTimestamp = Date.now(); | ||
| return; | ||
| } else { | ||
| this._needsAuth = true; | ||
| this._lastAuthSuccessTimestamp = 0; | ||
| } | ||
|
|
||
| walletsLogger.info("Auth required, initiating OTP flow", { needsAuth: this._needsAuth }); | ||
|
|
@@ -258,6 +287,7 @@ export abstract class NonCustodialSigner implements SignerAdapter { | |
|
|
||
| if (response?.status === "success" && response.signerStatus === "ready") { | ||
| this._needsAuth = false; | ||
| this._lastAuthSuccessTimestamp = Date.now(); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -301,12 +331,14 @@ export abstract class NonCustodialSigner implements SignerAdapter { | |
| } catch (err) { | ||
| walletsLogger.error("complete-onboarding: error", { error: err }); | ||
| this._needsAuth = true; | ||
| this._lastAuthSuccessTimestamp = 0; | ||
| this._authPromise?.reject(err as Error); | ||
| throw err; | ||
| } | ||
|
|
||
| if (response?.status === "success") { | ||
| this._needsAuth = false; | ||
| this._lastAuthSuccessTimestamp = Date.now(); | ||
| // We call onAuthRequired again so the needsAuth state is updated for the dev | ||
| if (this.config.onAuthRequired != null) { | ||
| await this.config.onAuthRequired( | ||
|
|
@@ -324,6 +356,7 @@ export abstract class NonCustodialSigner implements SignerAdapter { | |
|
|
||
| walletsLogger.error("complete-onboarding: OTP validation failed", { status: response?.status }); | ||
| this._needsAuth = true; | ||
| this._lastAuthSuccessTimestamp = 0; | ||
| const errorMessage = response?.status === "error" ? response.error : "Failed to validate encrypted OTP"; | ||
| const error = new Error(errorMessage); | ||
| this._authPromise?.reject(error); | ||
|
|
@@ -357,6 +390,7 @@ export abstract class NonCustodialSigner implements SignerAdapter { | |
| }); | ||
|
|
||
| if (response?.status === "error") { | ||
| this.invalidateAuthCache(); | ||
| throw new Error(response.error || "Failed to export private key"); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no tests for
NonCustodialSigneranywhere in the repo, and this new cache has several observable branches worth exercising: first call always reachesget-status, second call within TTL is short-circuited, call after TTL expiry re-issues the request, and a non-"ready"get-statusresponse resets the timestamp to 0. Covering these cases would make the behavior easier to verify and regression-proof.Prompt To Fix With AI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged, oh wise lizard 🐍 — you're right that there are no existing tests for
NonCustodialSignerin the repo. Adding a full test suite for this abstract class (which depends on iframe/WebView handshake connections) is a larger effort that goes beyond the scope of this bug fix. The caching behavior is exercised end-to-end via the open-signer companion PR's cache tests. That said, adding unit tests for the auth cache branches would be a good follow-up — just not in this PR.