-
Notifications
You must be signed in to change notification settings - Fork 34
fix(wallets-sdk): add default HTTP timeout + gate preAuth on prepareOnly #1802
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
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 | ||
| "@crossmint/common-sdk-base": patch | ||
| --- | ||
|
|
||
| fix: add default 30s HTTP timeout to ApiClient and gate preAuthIfNeeded on prepareOnly in wallet.send() | ||
|
|
||
| - ApiClient.makeRequest() now uses `AbortSignal.timeout(30_000)` by default so requests that never receive a response will reject after 30 seconds instead of hanging indefinitely. Callers can override by passing their own `signal` in `RequestInit`. Timeout rejections are wrapped in a typed `ApiRequestTimeoutError`. | ||
| - `wallet.send()` no longer calls `preAuthIfNeeded()` when `prepareOnly` is `true`, preventing unnecessary signer pre-authorization that could stall the call before the HTTP request fires. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { ApiClient, ApiClientError, ApiRequestTimeoutError } from "./ApiClient"; | ||
|
|
||
| class TestApiClient extends ApiClient { | ||
| get commonHeaders(): HeadersInit { | ||
| return { "x-test": "true" }; | ||
| } | ||
| get baseUrl(): string { | ||
| return "https://api.example.com"; | ||
| } | ||
|
|
||
| // Expose makeRequest indirectly through the public HTTP methods | ||
| } | ||
|
|
||
| describe("ApiClient - timeout handling", () => { | ||
| const originalFetch = globalThis.fetch; | ||
|
|
||
| afterEach(() => { | ||
| globalThis.fetch = originalFetch; | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it("should reject with ApiRequestTimeoutError when fetch times out", async () => { | ||
| const timeoutError = new DOMException("The operation was aborted due to timeout", "TimeoutError"); | ||
| globalThis.fetch = vi.fn().mockRejectedValue(timeoutError); | ||
|
|
||
| const client = new TestApiClient(); | ||
|
|
||
| await expect(client.get("/slow-endpoint", {})).rejects.toThrow(ApiRequestTimeoutError); | ||
| await expect(client.get("/slow-endpoint", {})).rejects.toThrow('API request to "/slow-endpoint" timed out'); | ||
| }); | ||
|
|
||
| it("should include the path in the ApiRequestTimeoutError", async () => { | ||
| const timeoutError = new DOMException("The operation was aborted due to timeout", "TimeoutError"); | ||
| globalThis.fetch = vi.fn().mockRejectedValue(timeoutError); | ||
|
|
||
| const client = new TestApiClient(); | ||
|
|
||
| try { | ||
| await client.get("/my/custom/path", {}); | ||
| expect.fail("Expected ApiRequestTimeoutError"); | ||
| } catch (error) { | ||
| expect(error).toBeInstanceOf(ApiRequestTimeoutError); | ||
| expect((error as ApiRequestTimeoutError).path).toBe("/my/custom/path"); | ||
| } | ||
| }); | ||
|
|
||
| it("should use caller-supplied signal instead of the default timeout", async () => { | ||
| const mockResponse = new Response(JSON.stringify({ ok: true }), { status: 200 }); | ||
| globalThis.fetch = vi.fn().mockResolvedValue(mockResponse); | ||
|
|
||
| const client = new TestApiClient(); | ||
| const customController = new AbortController(); | ||
|
|
||
| await client.get("/endpoint", { signal: customController.signal }); | ||
|
|
||
| expect(globalThis.fetch).toHaveBeenCalledWith( | ||
| expect.any(String), | ||
| expect.objectContaining({ | ||
| signal: customController.signal, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it("should apply a default AbortSignal.timeout(30_000) when no signal is provided", async () => { | ||
| const timeoutSpy = vi.spyOn(AbortSignal, "timeout"); | ||
| const mockResponse = new Response(JSON.stringify({ ok: true }), { status: 200 }); | ||
| globalThis.fetch = vi.fn().mockResolvedValue(mockResponse); | ||
|
|
||
| const client = new TestApiClient(); | ||
| await client.get("/endpoint", {}); | ||
|
|
||
| expect(timeoutSpy).toHaveBeenCalledWith(30_000); | ||
| }); | ||
|
|
||
| it("should re-throw non-timeout errors as-is", async () => { | ||
| const networkError = new Error("Network failure"); | ||
| globalThis.fetch = vi.fn().mockRejectedValue(networkError); | ||
|
|
||
| const client = new TestApiClient(); | ||
|
|
||
| await expect(client.get("/endpoint", {})).rejects.toThrow("Network failure"); | ||
| await expect(client.get("/endpoint", {})).rejects.not.toBeInstanceOf(ApiRequestTimeoutError); | ||
| }); | ||
|
|
||
| it("should re-throw non-timeout DOMExceptions as-is", async () => { | ||
| const abortError = new DOMException("The operation was aborted", "AbortError"); | ||
| globalThis.fetch = vi.fn().mockRejectedValue(abortError); | ||
|
|
||
| const client = new TestApiClient(); | ||
|
|
||
| await expect(client.get("/endpoint", {})).rejects.toThrow(DOMException); | ||
| await expect(client.get("/endpoint", {})).rejects.not.toBeInstanceOf(ApiRequestTimeoutError); | ||
| }); | ||
|
|
||
| it("should still throw ApiClientError on 5xx responses", async () => { | ||
| const mockResponse = new Response("Internal Server Error", { | ||
| status: 500, | ||
| statusText: "Internal Server Error", | ||
| }); | ||
| globalThis.fetch = vi.fn().mockResolvedValue(mockResponse); | ||
|
|
||
| const client = new TestApiClient(); | ||
|
|
||
| await expect(client.get("/endpoint", {})).rejects.toThrow(ApiClientError); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -508,7 +508,11 @@ export class Wallet<C extends Chain> { | |
| ...(options?.transactionType != null ? { transactionType: options.transactionType } : {}), | ||
| }); | ||
|
|
||
| await this.preAuthIfNeeded(); | ||
| if (!options?.prepareOnly) { | ||
| await this.preAuthIfNeeded(); | ||
| } else { | ||
| await this.#signerInitialization; | ||
| } | ||
| const walletSigner = this.requireSigner(); | ||
|
Comment on lines
+511
to
516
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.
A minimal fix is to await initialization in the Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 511-514
Comment:
**`#signerInitialization` race skipped for `prepareOnly`**
`preAuthIfNeeded()` starts with `await this.#signerInitialization`, which is the only place that waits for `initDefaultSigner()` to finish auto-assembling `this.#signer`. By skipping `preAuthIfNeeded()` entirely for `prepareOnly`, `requireSigner()` on line 514 can be reached while `initDefaultSigner()` is still running — leaving `this.#signer === undefined` and causing `requireSigner()` to throw "This wallet is read-only" even though the signer would be available moments later. The tests don't expose this because `createMockWallet` calls `await wallet.useSigner(signer)` before any test runs, bypassing `initDefaultSigner()` entirely.
A minimal fix is to await initialization in the `prepareOnly` branch before calling `requireSigner()`:
```
if (!options?.prepareOnly) {
await this.preAuthIfNeeded();
} else {
await this.#signerInitialization; // ensure auto-assembly completes
}
const walletSigner = this.requireSigner();
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
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. Good catch — confirmed this is a real race. Fixed in 9726fd4 — added if (!options?.prepareOnly) {
await this.preAuthIfNeeded();
} else {
await this.#signerInitialization;
}
const walletSigner = this.requireSigner(); |
||
|
|
||
| let signer: string; | ||
|
|
||
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.
The 30-second timeout is hardcoded with no way to adjust it per
ApiClientinstance. Callers can override it per-request by passing their ownsignal, but there is no way to say "this client should always time out in 10 s" without wrapping every call. Consider accepting an optionaldefaultTimeoutMsin theApiClientconstructor so subclasses can configure it without touching call sites.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.
Agree this would be a nice enhancement, but treating it as out of scope for this bug-fix PR to keep the change minimal. The per-request
signaloverride is sufficient for callers who need a different timeout today. AdefaultTimeoutMsproperty onApiClientcould be added in a follow-up if subclasses need instance-level configuration.