You are viewing a single comment's thread from:

RE: PR #285 Enhancing Steemit Wallet: RPC Node Failover System + Accessibility Improvements (PR #285)

in Steem Dev16 days ago

Update: PR #285 Review Outcome

Unfortunately, this PR was not able to be merged at this time. @ety001 provided the following review feedback with a recommendation to defer the RPC Node Selector feature to a post-stabilization phase.


Review Comment: RPC Node Selector Feature — Recommendation: Defer to Post-Stabilization Phase

After careful review, I recommend not merging this feature at the current stage of the refactoring. Here's the architectural rationale:

The core design principle of this refactored version is that the browser should never communicate directly with Steem RPC nodes. Instead, the Next.js API layer acts as the sole gateway: the browser only signs transactions locally with the user's private keys, and all network communication (queries, broadcasts, health checks) is proxied through the server-side API routes.

This PR introduces a user-selectable RPC node switcher, which implies a mental model where users "choose which node their browser talks to." However, in the current architecture, the browser does not talk to any node directly — it only talks to /api/*. The X-Steem-RPC header is effectively just a "preference hint" passed to the server, and this introduces concurrency risks, cache isolation issues, and inconsistencies between query and broadcast paths without clear user-visible benefits.

Technical Issues Identified

  • Concurrency race condition: withRpcOverride mutates a global singleton. Under concurrent requests, one request may overwrite the RPC URL while another is suspended mid-flight.
  • Broadcast routes ignore the header: All broadcast routes do not call applyRpcOverride, so user node selection is silently ignored for writes.
  • Cache cross-contamination: React Query keys do not include the RPC node URL. A cache entry from node A can be served to a user who explicitly selected node B.
  • login.ts lacks failover: Login signature verification calls SteemService without withFailover, so a single node outage blocks logins.
  • Environment variable mismatch risk: NEXT_PUBLIC_RPC_NODES (from client) and RPC_NODES (from server) can diverge, leading to silent fallback behavior.

Recommendation

  1. Do not merge PR #285 for the initial stable release. The default server-side failover mechanism (withFailover with left-to-right fallback) is sufficient for launch.
  2. Revisit this feature after the refactored version is live and stable. Once the new architecture has been running in production and the server-side RPC layer is battle-tested, we can properly explore user-defined node preferences — ideally implemented as a server-side configuration (e.g., per-session or per-user server setting) rather than a client-side selector, ensuring the browser still never holds RPC topology knowledge.

The code quality in this PR is otherwise good (tests, a11y fixes, AsyncLocalStorage usage), but the feature itself is architecturally premature for the current phase.


I appreciate the thorough review and agree that maintaining the architectural integrity of the refactored wallet takes priority. The accessibility fixes and foundational failover logic from this PR remain valuable, and I'll work on extracting those components for a follow-up submission while shelving the client-side node selector for a future phase.

Thanks to the review team for the constructive feedback.

Coin Marketplace

STEEM 0.04
TRX 0.32
JST 0.083
BTC 60990.63
ETH 1575.34
USDT 1.00
SBD 0.47