Skip to content

Treat uninitialized wallets as unverified in isConnected - #583

Draft
L03TJ3 with Copilot wants to merge 4 commits into
masterfrom
copilot/prevent-failed-xdc-wallet-init
Draft

L03TJ3 with Copilot wants to merge 4 commits into
masterfrom
copilot/prevent-failed-xdc-wallet-init

Conversation

Copilot AI commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

If XdcAdminWallet fails to initialize (for example, its RPC is rate-limited), it never gets an identityContract. MultiWallet.isConnected checks every wallet that has an Identity contract, so that one broken wallet makes the whole check throw Cannot read properties of undefined (reading 'methods'). As a result, Celo /verify/topwallet fails in production even for accounts Celo has verified.

Change: Web3Wallet.isConnected

  • If identityContract is missing, log a warning (address, wallet name, network ID) and return false. This replaces the TypeError.
if (!this.identityContract) {
  log.warn('isConnected: identity contract unavailable, wallet not initialized', { address, wallet: this.name, networkId: this.networkId })
  return false
}
  • Verification semantics are unchanged: a positive result on any Identity chain is still enough, and an unavailable wallet never counts as verified.
  • BaseAdminWallet.topWallet also calls celoWallet.isConnected. If the Celo wallet itself is uninitialized, Base top-ups are now refused instead of throwing.
  • Out of scope: a live RPC error from any one wallet still rejects the whole MultiWallet.isConnected check, as before. This PR does not add RPC fallback or retries.

Tests: blockchain/__tests__/multiWallet.test.js (new)

Both tests leave XDC uninitialized and build the MultiWallet with stubbed Identity contracts:

  • Celo verifies the account: the check returns true, and the XDC warning is logged.
  • No wallet verifies the account: the check returns false, so no top-up is authorized.

Reviewer note (not addressed here)

The MultiWallet constructor makes the first wallet it visits the mainWallet. JS orders integer-like object keys from lowest to highest, so with {122, ...{42220, 8453, 50}} that is XDC (50), not Fuse (122). This contradicts the // "main" wallet goes first comment. If confirmed, isVerified, lastAuthenticated and getAuthenticationPeriod currently run against XDC and would still fail when XDC is down. This likely needs its own issue.

Summary by Sourcery

Make wallet initialization resilient to transient RPC failures while ensuring unavailable wallets are treated as unverified and do not disrupt healthy-chain verification.

New Features:

  • Add bounded wallet initialization retries with exponential backoff and single failure alerting.
  • Support HTTP RPC endpoint failover when an endpoint is rate-limited.
  • Treat wallets without an available Identity contract as unverified instead of throwing during connection checks.

Bug Fixes:

  • Prevent an unavailable wallet from blocking verification or authorizing top-ups when another Identity chain can verify the account.
  • Prevent stale or partially initialized wallet state from being used after initialization failures.
  • Sanitize RPC credentials and API keys from initialization failure alerts.

Enhancements:

  • Improve wallet initialization recovery and ensure failed initialization can be retried later without caching a failed result.

Tests:

  • Add coverage for uninitialized-wallet verification, initialization retries and recovery, partial initialization failures, RPC endpoint fallback, and sanitized alerts.

…erification check

Co-authored-by: L03TJ3 <6606028+L03TJ3@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix wallet initialization to avoid breaking Celo top-ups Treat uninitialized wallets as unverified in isConnected Oct 27, 2026
Copilot AI requested a review from L03TJ3 October 27, 2026 15:58

L03TJ3 commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

@copilot Please extend this fix so failed wallets can actually recover, keeping it simple:

  • Use the existing multi-RPC provider in transport.js for chain/admin wallet initialization, rather than selecting only the first configured HTTP endpoint. Reuse the existing mechanism, no new fallback framework.
  • Retry failed initialization independently per wallet/chain, with a bounded delay/backoff. Allow 3 total attempts (initial attempt + 2 retries). Do not permanently cache a failed _readyPromise, and do not block healthy chains while another chain retries. Avoid overlapping initialization attempts or duplicating setup on retry.
  • After the third failed attempt, send one Slack notification using the existing alert helper/pattern. Include the chain/wallet and a sanitized error; do not send RPC credentials. Leave that wallet unavailable without taking down other chains or repeatedly spamming Slack.
  • Keep the missing-contract guard and existing verification rules from this PR.

Please add focused tests for endpoint fallback, recovery on a later initialization attempt, and three failures producing one Slack alert while a healthy chain remains usable. Keep this scoped; no broad refactor.

…itized Slack alert

Co-authored-by: L03TJ3 <6606028+L03TJ3@users.noreply.github.com>

Copilot AI commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Please extend this fix so failed wallets can actually recover, keeping it simple:

  • Use the existing multi-RPC provider in transport.js for chain/admin wallet initialization, rather than s...

Done in 192b78f: wallets now use the multi-RPC provider, and a failed wallet retries its init up to 3 times before sending one Slack alert. The new tests pass. The existing wallet tests against a local chain have not run: I couldn't start a chain here (the Solidity compiler download is blocked), and CI on this branch is waiting for maintainer approval.

  • Endpoint fallback: getWeb3TransportProvider now creates the provider with HttpProviderFactory.create(...) from transport.js, so every comma-separated RPC endpoint is used, not just the first.
  • Transport bug fixed: the provider's rate-limit handling called endpoints.splice(endpoints.indexOf(item, 1)), which removed the wrong endpoints, often the healthy ones. It now removes only the rate-limited endpoint for 60s, and never the last one left. The new fallback test fails on the old code.
  • Retries: initialize() now makes up to 3 attempts with 5s and 10s backoff, configurable via the initRetryDelay option. Each wallet retries on its own schedule. Callers that ask while init is running share the same attempt.
    • A failed run is not kept: a later .ready call starts a fresh 3-attempt run.
    • Each attempt resets its accounts and the EIP-1559 check result, so a retry doesn't set them up twice.
    • A failed attempt now throws instead of returning false.
  • Slack: one alert after the 3rd failure, with wallet name, network ID, network and an error message stripped down to protocol and host (no RPC credentials, paths or query strings). It isn't re-sent by later runs until init succeeds. The per-attempt alert inside init() is gone. The failed wallet resolves false, and other chains keep working.
  • Kept: the missing-contract guard in isConnected and the existing verification rules.

Tests in the new blockchain/__tests__/walletInit.test.js cover:

  • fallback from a rate-limited endpoint to a healthy one;
  • recovery on the 2nd attempt, with no overlapping attempts and no alert;
  • three failures sending exactly one sanitized Slack alert, while Celo is ready, and isConnected passes, before XDC finishes retrying.

Decisions for you:

  • Startup delay: MultiWallet.ready still waits for all wallets, so when a chain is down, server startup is delayed by that chain's retry time (about 15s plus attempt time). Healthy wallets finish setting up independently.
  • KMS errors: errors before the RPC calls (for example KMS setup) are now retried and then leave the wallet unavailable, instead of crashing the server process.

@L03TJ3
L03TJ3 requested a review from sirpy October 27, 2026 16:51

This branch has not been deployed

No deployments
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.

Prevent failed XDC wallet initialization from breaking Celo top-ups

2 participants