Open
Conversation
Replace the dual fire-and-forget promise pattern with a sequential flow: createWallet → validateAndRenewAuthToken → pollForWalletStatus. This eliminates the race between renewPromise2 and polling that caused flaky 403 errors. Changes: - start(): remove renewPromise/renewPromise2, handleCreate closure, and closure-captured error variables. Token renewal is now a single awaited call after createWallet. - pollForWalletStatus(): replace setInterval with bounded while loop (max 60 attempts). Prevents callback stacking and adds timeout. - startReadOnly(): replace getWalletStatus fallback (needed auth) with retry loop on getReadOnlyAuthToken (no auth). - validateAndRenewAuthToken(): await the previously fire-and-forget renewAuthToken in else-if(usePassword). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughStart/read-only startup flows refactored: startup now uses bounded retry loops with explicit timeouts for wallet status and read-only auth-token acquisition; auth-token renewal now propagates errors and is performed on-demand; tests added/updated to cover retries, timeouts, and polling behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Wallet
participant WalletAPI
participant Storage
Client->>Wallet: start({pinCode, password})
Wallet->>WalletAPI: createWallet(payload)
alt create returns creating
Wallet->>Wallet: pollForWalletStatus() (bounded retry loop)
Wallet->>WalletAPI: getWalletStatus() (retries until ready)
else create returns ready
Wallet->>Wallet: (skip polling)
end
Wallet->>Wallet: validateAndRenewAuthToken(pinCode) (awaited when invoked)
Wallet->>Storage: onWalletReady() (persist state)
Wallet-->>Client: resolve (ready, auth token, address)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1058 +/- ##
===========================================
+ Coverage 71.03% 87.97% +16.93%
===========================================
Files 114 114
Lines 8910 8895 -15
Branches 2030 2019 -11
===========================================
+ Hits 6329 7825 +1496
+ Misses 2551 1042 -1509
+ Partials 30 28 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Exercises the creating→ready polling path with a brand-new wallet that the wallet-service has never seen, which is the exact path where the old fire-and-forget pattern raced. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__tests__/wallet/readOnlyWallet.test.ts (1)
181-211:⚠️ Potential issue | 🟡 MinorRestore fake timers even when the test fails.
jest.useRealTimers()only runs on the happy path here. If any assertion before Line 211 fails, later tests in this file inherit fake timers and can start failing for unrelated reasons.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/wallet/readOnlyWallet.test.ts` around lines 181 - 211, The test 'should time out if getReadOnlyAuthToken never succeeds' currently calls jest.useRealTimers() only on the happy path; wrap the body that uses jest.useFakeTimers() (the setup, timer advances and assertions referencing wallet.startReadOnly, mockCreateReadOnlyAuthToken, mockGetWalletStatus) in a try/finally so jest.useRealTimers() is always executed, or move jest.useRealTimers() to a file-level afterEach to restore real timers for every test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/integration/service-specific/start.test.ts`:
- Around line 125-130: The test claims to exercise the creating→ready path but
calls buildWalletInstance() with no seed, which triggers the
precalculated-wallet fallback in service-facade.helper and can skip first-time
creation; update the test to force a true new-wallet creation by calling
buildWalletInstance with an explicit unique seed or the helper's "force create"
parameter (e.g., buildWalletInstance({ seed: '<unique-seed>' }) or
buildWalletInstance({ forceCreate: true })) so the flow goes through
createWallet → validateAndRenewAuthToken → pollForWalletStatus and reliably
covers the race condition.
In `@__tests__/wallet/wallet.test.ts`:
- Around line 3601-3628: The test uses jest.useFakeTimers() but only calls
jest.useRealTimers() on the success path, so failures leave fake timers active;
wrap the timer restoration in a guaranteed cleanup (e.g., use a try/finally
inside the test or add an afterEach that calls jest.useRealTimers()) to ensure
jest.useRealTimers() always runs regardless of assertions—update the test
containing wallet.pollForWalletStatus() to restore real timers in finally or
rely on a test-level cleanup to avoid leaking fake timers.
In `@src/wallet/wallet.ts`:
- Around line 722-739: The loop in (method containing) walletApi.getWalletStatus
is bounded by attempts (MAX_WALLET_STATUS_POLL_ATTEMPTS) but each axios request
can itself block up to the HTTP timeout, so the total wait can exceed the
intended ~60s; change the polling to be time-bounded instead of attempt-bounded
by tracking elapsed time (e.g., start = Date.now()) and loop while (Date.now() -
start < MAX_WALLET_STATUS_POLL_DURATION_MS), calling walletApi.getWalletStatus
and breaking/throwing as you already do, or alternatively modify
walletApi.getWalletStatus to accept/forward a per-request timeout/AbortSignal so
each request respects a short timeout and keep the existing interval logic;
update references: walletApi.getWalletStatus, MAX_WALLET_STATUS_POLL_ATTEMPTS,
WALLET_STATUS_POLLING_INTERVAL, and WalletRequestError when implementing the
duration-based stop.
- Around line 1224-1234: The current loop catches errors from both
getReadOnlyAuthToken() and onWalletReady(), which hides permanent failures;
change the logic so the retry loop only wraps getReadOnlyAuthToken() (using
MAX_WALLET_STATUS_POLL_ATTEMPTS and WALLET_STATUS_POLLING_INTERVAL), and if
getReadOnlyAuthToken() succeeds break out and then call
onWalletReady(skipAddressFetch) once outside the retry loop so any errors from
onWalletReady (including getNewAddresses() or storage/auth setup) propagate
immediately; do not swallow or rethrow those errors as transient—only retry
failures from the auth token acquisition.
- Around line 483-485: The call to validateAndRenewAuthToken(pinCode) can
complete without actually setting this.authToken (renewAuthToken swallows
failures), so ensure startup fails fast if no token was obtained: either make
renewAuthToken propagate errors instead of swallowing them, or immediately after
await this.validateAndRenewAuthToken(pinCode) add a null-check for
this.authToken and throw a clear error (e.g., "missing auth token after
renewal") so subsequent calls (which use wallet.getAuthToken() and
walletServiceAxios building Authorization: Bearer ${...}) never send Bearer
null; update validateAndRenewAuthToken/renewAuthToken behavior accordingly to
guarantee a token is present or an exception is raised.
---
Outside diff comments:
In `@__tests__/wallet/readOnlyWallet.test.ts`:
- Around line 181-211: The test 'should time out if getReadOnlyAuthToken never
succeeds' currently calls jest.useRealTimers() only on the happy path; wrap the
body that uses jest.useFakeTimers() (the setup, timer advances and assertions
referencing wallet.startReadOnly, mockCreateReadOnlyAuthToken,
mockGetWalletStatus) in a try/finally so jest.useRealTimers() is always
executed, or move jest.useRealTimers() to a file-level afterEach to restore real
timers for every test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 542ed17a-15f7-4b0e-9920-f603c30cb2e9
📒 Files selected for processing (4)
__tests__/integration/service-specific/start.test.ts__tests__/wallet/readOnlyWallet.test.ts__tests__/wallet/wallet.test.tssrc/wallet/wallet.ts
Comment on lines
+722
to
739
| for (let attempt = 0; attempt < MAX_WALLET_STATUS_POLL_ATTEMPTS; attempt++) { | ||
| const data = await walletApi.getWalletStatus(this); | ||
|
|
||
| if (data.status.status === WS_STATUS_READY) { | ||
| return; | ||
| } | ||
| // Only possible states are 'ready', 'creating' and 'error'. If status | ||
| // is not ready or creating, we should throw an error. | ||
| if (data.status.status !== WS_STATUS_CREATING) { | ||
| throw new WalletRequestError('Error getting wallet status.', { cause: data.status }); | ||
| } | ||
|
|
||
| await new Promise(resolve => { | ||
| setTimeout(resolve, WALLET_STATUS_POLLING_INTERVAL); | ||
| }); | ||
| } | ||
| throw new WalletRequestError('Wallet status polling timed out.'); | ||
| } |
There was a problem hiding this comment.
The new poll cap is attempt-bounded, not time-bounded.
walletApi.getWalletStatus() (src/wallet/api/walletApi.ts:64-72) uses axiosInstance() with the default HTTP timeout (src/wallet/api/walletServiceAxios.ts:26-61) on every iteration. Under a slow or lossy connection, each attempt can block for the full request timeout before the 1s sleep, so this loop can still hang for minutes instead of roughly 60 seconds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallet.ts` around lines 722 - 739, The loop in (method containing)
walletApi.getWalletStatus is bounded by attempts
(MAX_WALLET_STATUS_POLL_ATTEMPTS) but each axios request can itself block up to
the HTTP timeout, so the total wait can exceed the intended ~60s; change the
polling to be time-bounded instead of attempt-bounded by tracking elapsed time
(e.g., start = Date.now()) and loop while (Date.now() - start <
MAX_WALLET_STATUS_POLL_DURATION_MS), calling walletApi.getWalletStatus and
breaking/throwing as you already do, or alternatively modify
walletApi.getWalletStatus to accept/forward a per-request timeout/AbortSignal so
each request respects a short timeout and keep the existing interval logic;
update references: walletApi.getWalletStatus, MAX_WALLET_STATUS_POLL_ATTEMPTS,
WALLET_STATUS_POLLING_INTERVAL, and WalletRequestError when implementing the
duration-based stop.
- Fail fast if authToken is null after renewal in start() - Move onWalletReady outside retry loop in startReadOnly so its errors propagate instead of being retried - Wrap fake timer tests in try/finally for cleanup - Add test for validateAndRenewAuthToken proactive renewal - Fix renewAuthToken mocks to set authToken (null-check) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/wallet.ts (1)
477-506:⚠️ Potential issue | 🟠 Major
waitReady: falsenow aborts the only remaining startup path.After removing the old fire-and-forget continuation, this early return means the non-blocking branch never renews the token, never polls, and never calls
onWalletReady().start({ waitReady: false })now leaves the wallet stuck inLoadingwith no path toREADY. Please keep the post-createWallet()flow in a single startup promise and onlyawaitit conditionally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 477 - 506, The early return when waitReady is false aborts the remaining startup flow; instead, compose the post-create flow (validateAndRenewAuthToken(pinCode), the authToken check, the status handling with pollForWalletStatus(), and the final await onWalletReady() plus clearSensitiveData()) into a single startup promise (e.g., startupPromise) and then await that promise only if waitReady is true; if waitReady is false, start startupPromise without awaiting it but attach a catch to log or handle errors to avoid unhandled rejections. Ensure you update the code paths around validateAndRenewAuthToken, pollForWalletStatus, onWalletReady, and clearSensitiveData so they execute in that single promise rather than being skipped by the early return.
♻️ Duplicate comments (1)
src/wallet/wallet.ts (1)
728-745:⚠️ Potential issue | 🟠 MajorThese retries still exceed the advertised 60s timeout.
Both loops are attempt-bounded, but each iteration awaits the HTTP call before the 1s sleep. Even normal request latency pushes the wall-clock timeout past 60 seconds, and slow responses can stretch it much further, so this still misses the acceptance criterion. Track an absolute deadline, or enforce a shorter per-request timeout, instead of only counting attempts.
Also applies to: 1227-1249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 728 - 745, The polling loop using MAX_WALLET_STATUS_POLL_ATTEMPTS and WALLET_STATUS_POLLING_INTERVAL can exceed the advertised 60s because each iteration awaits walletApi.getWalletStatus (which can be slow) plus the 1s sleep; change it to enforce an absolute deadline (e.g., compute const deadline = Date.now() + 60000) and before each iteration check Date.now() >= deadline and throw WalletRequestError('Wallet status polling timed out.') if expired, and/or wrap the walletApi.getWalletStatus(this) call in a per-request timeout (Promise.race with a timeout set to the remaining time until deadline) so slow HTTP responses can’t push the total wall-clock time beyond 60s; update both the loop around walletApi.getWalletStatus and the similar block at the other occurrence to use the same deadline+per-request-timeout approach and keep existing WS_STATUS_READY/WS_STATUS_CREATING checks and WalletRequestError usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/wallet/wallet.ts`:
- Around line 477-506: The early return when waitReady is false aborts the
remaining startup flow; instead, compose the post-create flow
(validateAndRenewAuthToken(pinCode), the authToken check, the status handling
with pollForWalletStatus(), and the final await onWalletReady() plus
clearSensitiveData()) into a single startup promise (e.g., startupPromise) and
then await that promise only if waitReady is true; if waitReady is false, start
startupPromise without awaiting it but attach a catch to log or handle errors to
avoid unhandled rejections. Ensure you update the code paths around
validateAndRenewAuthToken, pollForWalletStatus, onWalletReady, and
clearSensitiveData so they execute in that single promise rather than being
skipped by the early return.
---
Duplicate comments:
In `@src/wallet/wallet.ts`:
- Around line 728-745: The polling loop using MAX_WALLET_STATUS_POLL_ATTEMPTS
and WALLET_STATUS_POLLING_INTERVAL can exceed the advertised 60s because each
iteration awaits walletApi.getWalletStatus (which can be slow) plus the 1s
sleep; change it to enforce an absolute deadline (e.g., compute const deadline =
Date.now() + 60000) and before each iteration check Date.now() >= deadline and
throw WalletRequestError('Wallet status polling timed out.') if expired, and/or
wrap the walletApi.getWalletStatus(this) call in a per-request timeout
(Promise.race with a timeout set to the remaining time until deadline) so slow
HTTP responses can’t push the total wall-clock time beyond 60s; update both the
loop around walletApi.getWalletStatus and the similar block at the other
occurrence to use the same deadline+per-request-timeout approach and keep
existing WS_STATUS_READY/WS_STATUS_CREATING checks and WalletRequestError usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe22a3b4-9bf4-4578-93e6-203623081459
📒 Files selected for processing (3)
__tests__/wallet/readOnlyWallet.test.ts__tests__/wallet/wallet.test.tssrc/wallet/wallet.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/wallet/readOnlyWallet.test.ts
- tests/wallet/wallet.test.ts
- renewAuthToken now throws on failure instead of silently setting authToken=null. All callers already await it, so the non-throwing contract was no longer needed. - start() propagates the original createAuthToken error (e.g., "Auth invalid signature") instead of a generic "Auth token missing" message. - startReadOnly retries only WalletRequestError (server responded with non-200). Network/transport errors propagate immediately instead of being retried for 60s. - Timeout error preserves the last retry error as cause. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/wallet/wallet.ts (1)
113-114:⚠️ Potential issue | 🟠 MajorThe new 60s cap is still attempt-bounded, not wall-clock bounded.
Both loops call wallet-service APIs that inherit
axiosInstance()'s request timeout before the 1s sleep. Under slow responses, each iteration can burn the full HTTP timeout, so startup can still run well past 60 seconds.⏱️ Suggested direction
-const MAX_WALLET_STATUS_POLL_ATTEMPTS = 60; +const MAX_WALLET_STATUS_POLL_TIMEOUT_MS = 60_000; - for (let attempt = 0; attempt < MAX_WALLET_STATUS_POLL_ATTEMPTS; attempt++) { + const deadline = Date.now() + MAX_WALLET_STATUS_POLL_TIMEOUT_MS; + while (Date.now() < deadline) { // request... - await new Promise(resolve => { - setTimeout(resolve, WALLET_STATUS_POLLING_INTERVAL); - }); + const remaining = deadline - Date.now(); + if (remaining <= 0) { + break; + } + await new Promise(resolve => { + setTimeout(resolve, Math.min(WALLET_STATUS_POLLING_INTERVAL, remaining)); + }); }Apply the same deadline-based guard in
startReadOnly(), or pass a short per-request timeout into the wallet API calls.Also applies to: 724-740, 1219-1239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 113 - 114, The MAX_WALLET_STATUS_POLL_ATTEMPTS constant creates an attempt-bound loop that can exceed a real-time cap if each wallet-service call hits the HTTP timeout; update startReadOnly() and the other polling loops (the ones using MAX_WALLET_STATUS_POLL_ATTEMPTS) to enforce a wall-clock deadline instead of counting attempts by: compute a deadline (Date.now()+desiredMs) before the loop, check Date.now() against the deadline each iteration and break with a timeout error when exceeded, and/or pass an explicit short per-request timeout into the wallet service calls (via axios request config) so individual requests cannot block the total startup time; make these changes in the methods referencing MAX_WALLET_STATUS_POLL_ATTEMPTS and the startReadOnly() polling logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/wallet/wallet.ts`:
- Around line 113-114: The MAX_WALLET_STATUS_POLL_ATTEMPTS constant creates an
attempt-bound loop that can exceed a real-time cap if each wallet-service call
hits the HTTP timeout; update startReadOnly() and the other polling loops (the
ones using MAX_WALLET_STATUS_POLL_ATTEMPTS) to enforce a wall-clock deadline
instead of counting attempts by: compute a deadline (Date.now()+desiredMs)
before the loop, check Date.now() against the deadline each iteration and break
with a timeout error when exceeded, and/or pass an explicit short per-request
timeout into the wallet service calls (via axios request config) so individual
requests cannot block the total startup time; make these changes in the methods
referencing MAX_WALLET_STATUS_POLL_ATTEMPTS and the startReadOnly() polling
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4cc2c11-4ad1-4c67-abfc-687cc40b55d3
📒 Files selected for processing (3)
__tests__/wallet/readOnlyWallet.test.ts__tests__/wallet/wallet.test.tssrc/wallet/wallet.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/wallet/readOnlyWallet.test.ts
- tests/wallet/wallet.test.ts
The auth token endpoint requires the wallet to be in 'ready' state. Requesting it right after createWallet (while status may still be 'creating') caused start() to fail with "Error requesting auth token". Move validateAndRenewAuthToken to after pollForWalletStatus so the wallet is guaranteed ready before token renewal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. Remove explicit validateAndRenewAuthToken(pinCode) from start(). The axios interceptor already handles auth renewal on-demand when any authenticated API call is made (including during polling). authPrivKey and walletId are both set by that point. 2. Enrich createReadOnlyAuthToken error with HTTP status and response data. startReadOnly() now only retries on HTTP 400 (wallet still creating); other status codes (401, 403, 404) fail immediately instead of retrying for 60 seconds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pollForWalletStatus now retries WalletRequestError (transient server/auth errors) instead of crashing on the first failure. Non-WalletRequestError propagates immediately. Timeout error preserves the last transient error as cause. This mirrors the error classification already used by startReadOnly and closes a resilience gap introduced by the setInterval → for-loop migration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
createAuthToken and getWalletStatus threw generic messages
discarding the response status and body. Enrich both with
{ status, data } in the cause — matching what was already
done for createReadOnlyAuthToken. This gives callers (and
users) diagnostic info for 4xx failures instead of opaque
"Error requesting auth token" messages.
Also fix stale comment in start integration test that
still referenced the removed explicit auth call.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
Performance Impact: Wallet Startup Time BenchmarkTL;DRNo meaningful performance regression. Total wallet startup time is statistically equivalent between this branch and MethodologyA standalone benchmark script measures end-to-end
Tested against: ResultsWallet 1 (moderate history)
Wallet 2 (heavy history -- triggers polling on cold start)
InterpretationBoth wallets show the same pattern: the branch is faster at API but slower at Ready, with totals within noise. The reason is the auth token timing shift:
The total work is the same -- it just moved. The +169ms delta on the heavy wallet is within 1 standard deviation (stddev=127-171ms) and is not statistically significant. Polling pathThe heavy wallet triggered the polling path during warmup (cold start): 9022ms total, 4 polling attempts over ~5 seconds. This exercises the new bounded The Conclusion
The refactored auth flow trades a negligible, non-statistically-significant timing shift for deterministic, race-condition-free startup behavior. |
|
|
||
| throw new WalletRequestError('Error requesting read-only auth token.'); | ||
| throw new WalletRequestError('Error requesting read-only auth token.', { | ||
| cause: { status: response.status, data: response.data }, |
andreabadesso
approved these changes
Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Eliminates race conditions in the wallet-service
start()auth token flow that caused flaky 403 errors in CI integration tests. Replaces the dual fire-and-forget promise pattern with interceptor-driven on-demand auth, adds timeout bounds and transient error tolerance to polling, and fixes a logical contradiction instartReadOnly().Acceptance Criteria
start.test.ts) pass consistently without 403 flakespollForWalletStatustimes out after 60s instead of hanging indefinitelyWhy we're refactoring the auth token flow in
start()The problem in one sentence
The wallet service
start()method launches auth token renewal as background promises that race with wallet status polling, causing flaky 403 errors in CI.What's happening today
When
HathorWalletServiceWallet.start()runs, it needs to do two things:POST /wallet/init)These two operations have an ordering constraint: the token endpoint requires the wallet to already exist. For a brand-new wallet, the first token request will always fail because the wallet hasn't been created yet.
The current code tries to be clever about this by launching token renewal in the background while the wallet is being created, hoping it will finish by the time we need it. Here's the simplified flow:
The race condition
pollForWalletStatus()fires asetIntervalthat callsgetWalletStatus()every second. Each call needs an auth token. ButrenewPromise2(the token renewal) is running concurrently in the background. There are three possible outcomes:validateAndRenewAuthToken()is called again from inside the polling tick, racing withrenewPromise2. Both try to setthis.authToken. Usually one wins and things work.renewAuthToken()has a catch block that setsthis.authToken = nullwithout throwing. If this happens between another operation reading the token and using it, the request goes out with no token. The wallet-service returns 403 Forbidden.Outcome 3 is the flaky failure we see in CI. It depends on exact timing between the polling interval, the token endpoint response time, and JavaScript's microtask scheduling. It's not reproducible on demand, but it happens often enough to block CI.
Additional problems
pollForWalletStatuscan run forever. It usessetIntervalwith no maximum attempt count. If the wallet-service never returns 'ready', the test (or app) hangs indefinitely.setIntervalcallbacks can stack. IfgetWalletStatus()takes longer than the 1-second interval, multiple callbacks run concurrently, each trying to renew the auth token simultaneously.setIntervalasync callbacks swallow errors. WhengetWalletStatus()throws inside thesetIntervalcallback, the rejection is unhandled — the wrapping Promise never settles, and in Node.js 15+ this can terminate the process.startReadOnly()has a logical contradiction. When the read-only token request fails, the catch block callsgetWalletStatus()-- which requires an auth token we just failed to obtain.walletIdis overwritten 4+ times during startup (lines 455, 476, 526, 487), creating a window where concurrent token renewals might sign with a stale value.What we're changing
The new
start()flowReplace the concurrent fire-and-forget pattern with interceptor-driven on-demand auth:
Why this is better:
walletServiceAxios.ts) before each authenticated request. By the timepollForWalletStatusruns, bothwalletIdandauthPrivKeyare in memory, so the interceptor can obtain a token without the PIN.walletIdset once. No redundant overwrites.validateJWTExpireDate(). If the token is near expiry, it gets renewed before the request fires — no special handling needed for long polling windows."But won't this be slower?" The preemptive renewal (the first fire-and-forget) always failed for new wallets because the wallet didn't exist yet. We were paying the cost of a failing HTTP request for zero benefit. For existing wallets (the
WALLET_ALREADY_LOADEDpath), the interceptor obtains the token on the first API call, which adds one sequential round-trip. This is a small, acceptable cost for correctness.The new
pollForWalletStatus()Replace
setIntervalwith a boundedforloop with transient error tolerance:getWalletStatusin-flight at a time.setIntervalto clean up.WalletRequestError(server responded but said no) is retried; non-WalletRequestError(programming errors) fails fast. Permanent wallet error states ('error') also fail fast — the status check is outside the try-catch.The new
startReadOnly()Replace the catch block's
getWalletStatus()(which needs auth) with a retry loop ongetReadOnlyAuthToken()(which doesn't need auth), with error classification:The RO token endpoint returns 400 when the wallet is still 'creating'. We retry only that case. Non-400 HTTP errors (401, 403, 404) fail fast as permanent failures. Non-
WalletRequestError(network/transport) also fails fast.renewAuthTokennow throws on failureThe old
renewAuthToken()silently caught all errors and setthis.authToken = null. This was the root cause of the 403 race condition — a fire-and-forget renewal could null the token between another operation reading it and using it. The new code removes the try-catch entirely, letting errors propagate to the caller. All callers nowawaitthe result, so the non-throwing contract is no longer needed.Await fire-and-forget in
validateAndRenewAuthTokenThe
else if (usePassword)branch at line 1191 callsrenewAuthToken()withoutawait. This is a fire-and-forget that can silently null a valid token if the renewal fails. We addawaitto make it blocking. The latency cost is negligible since this branch only fires when the token is already valid (opportunistic refresh).What we're NOT changing
start(),startReadOnly(),pollForWalletStatus(),validateAndRenewAuthToken()keep the same signatures and return types.axiosInstanceinterceptor stays. The interceptor inwalletServiceAxios.tsthat callsvalidateAndRenewAuthToken()before authenticated requests is the mechanism we now rely on for startup auth — it was already doing this for all other API calls.Test coverage
We're adding unit tests for every edge case in the refactored flow:
Summary by CodeRabbit
Tests
Refactor