fix(wallets-sdk): add default HTTP timeout + gate preAuth on prepareOnly#1802
fix(wallets-sdk): add default HTTP timeout + gate preAuth on prepareOnly#1802devin-ai-integration[bot] wants to merge 2 commits into
Conversation
Co-Authored-By: jorge@paella.dev <jorge@paella.dev>
Original prompt from jorge@paella.dev
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: 9726fd4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Prompt To Fix All 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.
---
This is a comment left during a code review.
Path: packages/common/base/src/apiClient/ApiClient.ts
Line: 30
Comment:
**Hardcoded timeout with no override at the client level**
The 30-second timeout is hardcoded with no way to adjust it per `ApiClient` instance. Callers can override it per-request by passing their own `signal`, but there is no way to say "this client should always time out in 10 s" without wrapping every call. Consider accepting an optional `defaultTimeoutMs` in the `ApiClient` constructor so subclasses can configure it without touching call sites.
```suggestion
signal: init.signal ?? AbortSignal.timeout(this.defaultTimeoutMs ?? 30_000),
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(wallets-sdk): add default HTTP timeo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!options?.prepareOnly) { | ||
| await this.preAuthIfNeeded(); | ||
| } | ||
| const walletSigner = this.requireSigner(); |
There was a problem hiding this 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();
Prompt To Fix With AI
This 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.There was a problem hiding this comment.
Good catch — confirmed this is a real race. preAuthIfNeeded() awaits this.#signerInitialization (line 1404), and by skipping the entire call for prepareOnly we'd let requireSigner() run before initDefaultSigner() completes.
Fixed in 9726fd4 — added await this.#signerInitialization in the else branch:
if (!options?.prepareOnly) {
await this.preAuthIfNeeded();
} else {
await this.#signerInitialization;
}
const walletSigner = this.requireSigner();| response = await fetch(this.buildUrl(path), { | ||
| ...init, | ||
| headers: { ...this.commonHeaders, ...init.headers }, // commonHeaders intentionally first, in case sub class wants to override | ||
| signal: init.signal ?? AbortSignal.timeout(30_000), |
There was a problem hiding this comment.
The 30-second timeout is hardcoded with no way to adjust it per ApiClient instance. Callers can override it per-request by passing their own signal, but there is no way to say "this client should always time out in 10 s" without wrapping every call. Consider accepting an optional defaultTimeoutMs in the ApiClient constructor so subclasses can configure it without touching call sites.
| signal: init.signal ?? AbortSignal.timeout(30_000), | |
| signal: init.signal ?? AbortSignal.timeout(this.defaultTimeoutMs ?? 30_000), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/apiClient/ApiClient.ts
Line: 30
Comment:
**Hardcoded timeout with no override at the client level**
The 30-second timeout is hardcoded with no way to adjust it per `ApiClient` instance. Callers can override it per-request by passing their own `signal`, but there is no way to say "this client should always time out in 10 s" without wrapping every call. Consider accepting an optional `defaultTimeoutMs` in the `ApiClient` constructor so subclasses can configure it without touching call sites.
```suggestion
signal: init.signal ?? AbortSignal.timeout(this.defaultTimeoutMs ?? 30_000),
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
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 signal override is sufficient for callers who need a different timeout today. A defaultTimeoutMs property on ApiClient could be added in a follow-up if subclasses need instance-level configuration.
Co-Authored-By: jorge@paella.dev <jorge@paella.dev>
🔥 Smoke Test Results❌ Status: Failed Statistics
Test DetailsThis is a non-blocking smoke test. Full regression tests run separately. |
|
Reviews (2): Last reviewed commit: "fix: await #signerInitialization in prep..." | Re-trigger Greptile |
Description
Fixes two SDK-side bugs that, in combination, cause
wallet.send()to hang indefinitely (reported via Pylon #6935 — Pálpito).Bug 1 — No HTTP timeout on ApiClient:
makeRequest()used barefetch()with noAbortController. If a response was lost in transit, the Promise would never resolve or reject. Now appliesAbortSignal.timeout(30_000)by default. Callers can still override by passing their ownsignalinRequestInit. Timeout rejections are caught and rethrown as a typedApiRequestTimeoutError(extendsError, notApiClientError, since there is no HTTP response to attach).Bug 2 —
preAuthIfNeeded()runs on prepareOnly sends:wallet.send()unconditionally calledpreAuthIfNeeded(), which triggers signer initialization/recovery andNonCustodialSigner.ensureAuthenticated(). WhenprepareOnly: true, the caller only wants to persist the transaction without broadcasting — signer pre-auth is unnecessary and can itself stall. Now gated behindif (!options?.prepareOnly). Theelsebranch still awaitsthis.#signerInitializationso thatrequireSigner()on the next line never races againstinitDefaultSigner().Audit note (out of scope)
The same preAuth-before-prepareOnly pattern exists in
EVMWallet.sendTransaction,EVMWallet.signMessage,EVMWallet.signTypedData,SolanaWallet.sendTransaction, andStellarWallet.sendTransaction. These are not fixed in this PR to keep scope tight — follow-up ticket needed.wallet.approve()also callspreAuthIfNeeded()but approval inherently requires signer auth, so no gate is needed there.Human review checklist
ApiRequestTimeoutErrorextends plainErrorrather thanApiClientError— intentional since there's no HTTP status/body to attach. Confirm this is the right hierarchy.defaultTimeoutMsproperty was suggested but deferred to keep this PR minimal.)#signerInitializationrace fix (else { await this.#signerInitialization; }) was flagged by Greptile and addressed — but note the existing tests usecreateMockWalletwhich callswallet.useSigner()directly, bypassinginitDefaultSigner(). The race path is not directly exercised in unit tests. Worth a manual sanity check or integration test follow-up.Test plan
ApiClient.test.ts— new file, 7 cases):makeRequest()rejects withApiRequestTimeoutErrorafter mocked timeout; error includes the request path; caller-supplied signal takes precedence over default;AbortSignal.timeout(30_000)is applied when no signal provided; non-timeout errors (network failures,AbortErrorDOMExceptions) pass through unchanged; 5xx still throwsApiClientError.wallet.test.ts— newpreAuthIfNeeded gatingdescribe block, 2 cases):wallet.send({ prepareOnly: true })does not callpreAuthIfNeeded;wallet.send()without the flag does call it.pnpm test:vitest— all 11 test suites green, no regressions. Required CI checks (lint, build & test) pass.Package updates
@crossmint/common-sdk-base: patch (newApiRequestTimeoutErrorexport,makeRequesttimeout logic)@crossmint/wallets-sdk: patch (prepareOnly gate inwallet.send()).changeset/fix-http-timeout-preauth-gate.mdLink to Devin session: https://crossmint.devinenterprise.com/sessions/f411cdd969cd4f07988fc20d3d08a850
Requested by: @jorge2393