Skip to content

Add websocket transaction confirmation - #4859

Draft
ponderingdemocritus wants to merge 1 commit into
nextfrom
ponderingdemocritus/starknet-ws-rpc-scope
Draft

Add websocket transaction confirmation#4859
ponderingdemocritus wants to merge 1 commit into
nextfrom
ponderingdemocritus/starknet-ws-rpc-scope

Conversation

@ponderingdemocritus

Copy link
Copy Markdown
Contributor

Adds an internal transaction confirmation strategy for packages/provider that can use Starknet websocket transaction-status subscriptions when a wsUrl is configured, while preserving polling as the default and auto fallback path.

The provider constructor now accepts either the legacy retry config or an options object with retry and transaction confirmation settings.

The websocket waiter subscribes to transaction status, treats PRE_CONFIRMED, ACCEPTED_ON_L2, and ACCEPTED_ON_L1 as success states, fetches the receipt once, and leaves existing revert/background confirmation handling intact.

Validation: CI=1 pnpm --filter @bibliothecadao/provider test, pnpm --filter @bibliothecadao/provider build, pnpm run format, and pnpm run knip.

@vercel

vercel Bot commented Jun 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

3 Skipped Deployments
Project Deployment Actions Updated (UTC)
eternum Ignored Ignored Jun 11, 2026 10:31pm
eternum-docs Ignored Ignored Preview Jun 11, 2026 10:31pm
eternum-mobile Ignored Ignored Preview Jun 11, 2026 10:31pm

Request Review

@claude

claude Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ponderingdemocritus's task —— View job


PR Review: Add websocket transaction confirmation

Tasks:

  • Gather context and read changed files
  • Review API consistency and public interface
  • Review type safety
  • Review breaking changes and cross-package impact
  • Review build & distribution
  • Post final review

Summary

This PR adds an optional WebSocket-based transaction confirmation strategy to @bibliothecadao/provider, with polling as the default/fallback. The implementation is well-structured and the backward-compatibility story is solid. A few issues are worth addressing before merging.


Breaking Changes / Backward Compatibility

No breaking changes detected. The fourth constructor argument accepts either the old RetryConfig shape or the new EternumProviderOptions shape. The discriminator at index.ts:499-505 uses "retryConfig" in value || "transactionConfirmation" in value, which is correct and preserves the legacy path. The existing tests at execute-and-check-transaction.l2-gas.test.ts:78-90 confirm this.


API Consistency

ws is a runtime dependency, not a dev dependency. package.json:30 lists ws: ^8.19.0 as a production dependency. Since it is only used as a dynamic fallback in transaction-confirmation.ts:336 for Node.js environments where globalThis.WebSocket is absent, this adds bundle weight to browser consumers unnecessarily. Consider moving ws to peerDependencies or optionalDependencies, and document that browser environments must provide globalThis.WebSocket or a websocketFactory.

WebSocketLike uses any in listener signatures. transaction-confirmation.ts:19 has:

addEventListener?: (event: string, listener: (...args: any[]) => void) => void;

This leaks any into the public type. The individual event shapes are already well-typed in attachWebSocketHandler, so narrowing to (...args: unknown[]) => void here is achievable and would improve strictness.

TX_WAIT_RETRY_INTERVAL_MS and TX_WAIT_SUCCESS_STATES are re-exported from index.ts:31-32 but only used internally. If these are not part of the intended public API, they should stay unexported. If they are intentional public API, they should be documented. Currently they are exported from index.ts line 65 only for createTransactionReceiptWaiter, but the constants themselves are not re-exported at the package boundary — which is fine, but the internal imports in index.ts:31-32 of these constants from transaction-confirmation.ts are unused since the waiter now encapsulates them. The console log at index.ts:1739-1740 still references them for informational purposes, so this is okay but worth noting.


Type Safety

isEternumProviderOptions relies on duck typing that may over-match. index.ts:505:

return "retryConfig" in value || "transactionConfirmation" in value;

A RetryConfig that accidentally has a retryConfig key would be misidentified. The RetryConfig type uses fields maxRetries, baseDelayMs, maxDelayMs, jitterFactor — none of which overlap, so this is safe in practice, but it's brittle. A more robust check could verify that the value does NOT contain maxRetries (a key exclusive to RetryConfig).

resolveRequest coerces message.id to Number (index.ts analogue at transaction-confirmation.ts:269). String IDs like "1" correctly round-trip, but fractional-number IDs (e.g. 1.5) are theoretically possible per JSON-RPC and would fail silently. In practice Starknet nodes use integer IDs, so this is low risk.

completedSubscriptions can grow unboundedly. transaction-confirmation.ts:160 — if a success notification arrives but waitForTransactionStatus is never called, the completed subscription is stored but never evicted (the only eviction is inside matchesCompletedSubscription when it's found). Over the life of a connection with many transactions this is a minor memory leak. A Map with a TTL or bounded size cap would be defensive.

PRE_CONFIRMED is cast with as TransactionFinalityStatus. transaction-confirmation.ts:6-8:

export const TX_WAIT_SUCCESS_STATES: TransactionFinalityStatus[] = [
  "PRE_CONFIRMED" as TransactionFinalityStatus,
  ...

This suggests PRE_CONFIRMED is not in the upstream starknet type's union. That's expected given the library lag, but the cast silences a type error rather than extending the type. A comment explaining this divergence would help future maintainers.


Error Handling

WebSocket connection is cached across calls but never re-established on permanent failure. In WebSocketTransactionReceiptWaiter.getConnection() at transaction-confirmation.ts:144-153, a failed connection promise clears this.connection and re-throws. The next call creates a new connection — this is correct. However, in FallbackTransactionReceiptWaiter, the fallback catches the error and switches to polling for that single call, but subsequent calls to the same waiter will retry the WebSocket again (since connection was cleared). This is probably the desired behavior but is worth confirming: after a flappy connection, every waitForTransactionReceipt call will attempt a new WebSocket connection before falling back. This could cause latency spikes.

unsubscribe errors are silently swallowed (transaction-confirmation.ts:198-200). This is intentional per the comment, but if subscription cleanup is a chargeable or rate-limited resource on the node, silent failure could be a problem. A debug-level log would help observability.


Build & Distribution

ws in dependencies increases the production bundle. See above — for browser consumers this pulls in the ws package unnecessarily. The dynamic import("ws") at transaction-confirmation.ts:336 is already behind a globalThis.WebSocket check, but bundlers may still include it in the output depending on tree-shaking configuration.

Test relies on private class name via .constructor.name (execute-and-check-transaction.l2-gas.test.ts:89, 111):

expect((provider as any).transactionReceiptWaiter.constructor.name).toBe("PollingTransactionReceiptWaiter");

Class names are not guaranteed to survive minification, and accessing private internals by name is fragile. Exposing a narrow readonly kind: string or instanceof check would be more resilient.


Minor / Nits

  • flushConnectionSetup in tests (transaction-confirmation.test.ts:61-64) awaits Promise.resolve() twice to flush the microtask queue. This is a fragile pattern — if the implementation adds an async step the flush may need updating. A vi.waitFor (already used elsewhere in the test) is more robust.
  • The WebSocketLike type (transaction-confirmation.ts:13-23) is exported but WebSocketTransactionReceiptWaiter (the class that uses it) is not. Downstream consumers who provide a custom websocketFactory will need WebSocketLike to type their factory return value — the export is correct and necessary.

Overall Assessment

The core design is sound: clean strategy pattern, clear fallback, proper cleanup with finally, and good test coverage of success and failure paths. The main actionable items are:

  1. Move ws to peerDependencies or optionalDependencies to avoid bundling it for browser consumers.
  2. Add a bounded cleanup mechanism for completedSubscriptions to prevent a slow memory leak.
  3. Replace .constructor.name assertions in tests with a more minification-safe check.
  4. Address the any in WebSocketLike.addEventListener for stricter type safety.

@claude

claude Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ponderingdemocritus's task —— View job


PR Review: Add websocket transaction confirmation

Tasks:

  • Gather context on changed files
  • Review transaction-confirmation.ts (new file)
  • Review index.ts changes
  • Review package.json and lock file changes
  • Review test files
  • Compile findings

Summary

This PR adds a WebSocket-based transaction confirmation strategy to packages/provider with clean fallback to polling. The implementation is well-structured, tested, and focused. No secrets or credentials are present. Below are findings organized by severity.


Security & Secrets

No issues found. No secrets, API keys, or private keys are committed. The wsUrl is consumed as a runtime config parameter (not hardcoded), and WebSocket construction is properly abstracted through the websocketFactory pattern. The dynamic ws module import (await import("ws")) is acceptable here as it's a fallback for Node.js environments only.

The ...any[] type in attachWebSocketHandler (transaction-confirmation.ts:348) is a minor surface—but given it is purely for internal event handler routing with no external exposure, it poses no practical risk.


Reliability & Correctness

Medium — WebSocket connection is never explicitly closed

WebSocketTransactionReceiptWaiter lazily creates and caches a single WebSocketJsonRpcConnection (transaction-confirmation.ts:124). The connection is created once and never torn down, even if the socket closes or the provider is discarded. This means:

  • After a close event, rejectActiveWork rejects pending subscriptions, but this.connection is not reset to undefined here. Subsequent waitForTransactionReceipt calls will await the same resolved (but now dead) promise and try to subscribe on a closed socket, throwing immediately.
  • Contrast with the catch path at transaction-confirmation.ts:146-149 which does reset this.connection on open failure — the same reset is missing on socket close.

Recommendation: In attachMessageHandlers, on the close event, also set this.connection = undefined (from the parent WebSocketTransactionReceiptWaiter) so the next call re-establishes the connection.


Low — resolveTransactionStatus silently ignores revert/failure statuses

transaction-confirmation.ts:291 only resolves on TX_WAIT_SUCCESS_STATES. If a transaction arrives with REJECTED or REVERTED finality status, the pending subscription is neither resolved nor rejected — it will hang until the socket closes or times out (TRANSACTION_CONFIRM_TIMEOUT_MS = 10_000). The fallback in FallbackTransactionReceiptWaiter will eventually catch this, but in "websocket" mode (no fallback) the caller waits the full timeout before getting an error.

Recommendation: Explicitly reject the pending subscription when a terminal failure status (REJECTED) is received.


Low — Request ID counter is per-connection, not per-class

nextRequestId starts at 1 on each new WebSocketJsonRpcConnection instance (transaction-confirmation.ts:157). If the connection is re-created (once the reconnect issue above is addressed), IDs restart from 1, which is fine. This is noted only for awareness — it is correct as-is.


Multi-Environment Consistency

No issues. The wsUrl is opt-in via the constructor options object and defaults gracefully to polling when absent. Existing callers passing a RetryConfig directly are handled by isEternumProviderOptions/resolveEternumProviderOptions (index.ts:498-524), preserving full backwards compatibility across Katana, Sepolia, and Mainnet configs.


Dependency Management

Low — ws added as a runtime dependency, not devDependency

package.json:30 adds "ws": "^8.19.0" to dependencies. However, ws is only used in the dynamic Node.js fallback path (transaction-confirmation.ts:336). In browser environments this import never runs. This is acceptable for an isomorphic package, but it adds ~70KB to Node.js consumers and should be documented. @types/ws is correctly in devDependencies.

The pnpm-lock.yaml change is minimal (+6/-1 lines), consistent with adding ws and @types/ws to the package.


Test Coverage

The test file (transaction-confirmation.test.ts) covers the key paths:

  • WebSocket subscription happy path with object-style params ✅
  • WebSocket subscription with array-style params ✅
  • Polling fallback when no wsUrl configured ✅
  • Polling override when mode: "polling"
  • Auto fallback to polling on socket failure ✅

Missing test cases:

  • What happens when waitForTransactionReceipt is called on a closed/dead socket after initial use (the reconnect issue above)
  • "websocket" mode with a REJECTED transaction status (the hang scenario above)
  • Concurrent waitForTransactionReceipt calls on the same waiter instance (single shared connection behavior)

Code Quality

  • isEternumProviderOptions type guard (index.ts:498-505) uses "retryConfig" in value || "transactionConfirmation" in value — this correctly differentiates from a plain RetryConfig since neither key exists on RetryConfig. Solid.
  • The fallback waiter at index.ts:1747-1748 creates a new PollingTransactionReceiptWaiter on every call if this.transactionReceiptWaiter is undefined. Since the constructor always sets transactionReceiptWaiter via createTransactionReceiptWaiter, this path is only reachable in tests or subclasses. Low risk, but slightly surprising — the ?? expression could be replaced with a pre-created fallback stored at construction time.
  • TX_WAIT_SUCCESS_STATES casts "PRE_CONFIRMED" as TransactionFinalityStatus (transaction-confirmation.ts:6). The cast is needed because starknet.js's enum doesn't yet include PRE_CONFIRMED as a typed value. A comment explaining why the cast is needed would reduce future confusion.

Verdict

Approve with minor suggestions. The core design is sound, the implementation is clean, and the backwards-compatibility handling is correct. The primary concern is the missing connection reset on socket close which can cause silent hangs for subsequent transactions after a connection drop. The rejection of failure statuses is a secondary reliability issue worth addressing before wider rollout on Mainnet.

@github-actions

Copy link
Copy Markdown
Contributor

Failed to generate code suggestions for PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant