Repository navigation
fix(client): Bitcoin connector detection and reconnect correctness - #80
Merged
Merged
Conversation
`getInternalProvider` returned as soon as `binancew3w` existed, so a build that injects `binancew3w` without a `bitcoin` provider resolved undefined and never reached the `window.unisat.isBinance` fallback. The LI.FI widget's own detection accepted both paths, so the two disagreed and Binance could be listed while this connector could not drive it. Same shape as the BitKeep fix in #77, and the widget is removing its duplicate detection in favour of asking the connector. `unisat()` still rejects `isBinance`, so the two cannot claim the same injection.
🦋 Changeset detectedLatest commit: f509fbf The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
3 tasks done
`connect()` took no parameters, so it ignored `isReconnecting`, and it read accounts through `getAccounts()` — which calls the interactive `bitcoin:connect`. `reconnect()` runs on every mount and `isAuthorized()` returns true whenever the connected shim is set, so a returning user with a locked MetaMask got an unsolicited prompt on page load, and a rejection surfaced as `UserRejectedRequestError`. Read the session the wallet already restored when reconnecting, and keep the interactive path for a user-initiated connect. This is the same treatment `unisat`, `okx`, `binance`, `bitget` and `onekey` already have. This matters now because lifinance/widget#878 makes `metamask()` a default Bitcoin connector, which would have exposed every integrator to the prompt.
…bsence Two problems with the passive reconnect path. `wallet.accounts` is filled by a session lookup the wallet does not await, so it reads empty from registration until that round trip resolves. `reconnect` polls only for the provider, which appears the moment the wallet enters the registry, so a wallet registering after the app mounts reported no session and nothing recovered it — the `bitcoin:events` subscription is created after the length check, so the later change event reached no listener. Poll the accounts too, for up to a second: one extension round trip, and `reconnect` already spends up to five seconds finding the provider. `ConnectorNotConnectedError` was thrown inside the `try` whose `catch` rethrows everything as `UserRejectedRequestError`, so a returning user with no session was told they had rejected a request they never saw. Throw it outside, as `unisat` and `okx` do. The earlier tests could not catch either: the fake wallet hardcoded `accounts`, which is the field whose timing is the bug. It now restores asynchronously, and a fourth case covers the absent session.
chybisov
changed the title
`disconnect()` awaited `connector.disconnect()` before deleting the connection, so a throw skipped the delete, the listener cleanup and the state update alike. `xverse`, `oyl` and `leather` all throw `ProviderNotFoundError` once their provider is gone, and the surviving connection was then promoted to `current`: the store reported an account that could never sign, and every later disconnect hit the same dead entry. Detach it regardless and rethrow after the state settles, so the caller still learns it failed. Three follow-ups in the MetaMask connector, all on the reconnect path: - An account that cannot be parsed now drops out of the poll instead of ending it, which is the half-initialized case the poll exists to survive. - A failure while reconnecting keeps its own error type. Only the interactive path may report `UserRejectedRequestError`, since nothing is shown to the user otherwise. - `isAuthorized()` requires a restored account, as `unisat`, `okx`, `binance`, `bitget` and `onekey` do. The storage shim alone sent every returning user through the poll, even after revoking the site or locking the wallet. It reads the session rather than calling `getAccounts`, so it stays passive.
`isAuthorized()` had started requiring a restored account, to match the other
connectors. For MetaMask that is wrong: the wallet fills `accounts` from a
session lookup it does not await, and `reconnect` calls `isAuthorized()` in
the same tick the wallet registers. It returned false on every reload, so
`reconnect` skipped MetaMask and the wait added to `connect()` was never
reached — reconnect was broken for the one wallet this work is about. Gate on
the shim and the provider, and let `connect({ isReconnecting: true })` decide
by waiting for the session.
The test that was supposed to cover that branch never reached it: the storage
stub resolved `undefined`, so `isAuthorized()` returned at the shim gate, and
it passed identically with the branch deleted. The stub now reports the shim,
and the restoring-session case fails if the accounts read comes back.
`disconnect()` also clears the connector's shim when its `disconnect()`
throws. The connector throws before writing its own, so the store said
disconnected while storage still said connected, and the next reconnect would
have restored what the user disconnected.
The poll window moves to 1500ms with an accurate rationale: the provider
lookup in `reconnect` returns as soon as the wallet registers, so this is the
only meaningful wait inside the 5s `connect` budget.
Three defects that `metamask()` becoming a default connector makes reachable for every widget user. A `change` event mapped accounts through `toAccount` unguarded, while the reconnect poll next to it already skips what it cannot parse. `toAccount` throws on a missing `publicKey` or an address it cannot classify, and the throw escaped into MetaMask's own emitter: bigmi never learned the account had changed and kept signing against the previous address. `onAccountsChanged` treated only an empty raw list as a disconnect, then emitted `change` with the payment-filtered list. A Taproot-only selection filters to nothing, and an empty array is not nullish, so the connection kept `status: 'connected'` with no usable address and every route request failed. Filter first, and disconnect when nothing is left. `onDisconnect` left the connected shim in storage. `isAuthorized` gates on that shim, so a user who revoked the site inside MetaMask was reported authorized on every later load and paid for a session poll that could never succeed. Clear it.
The parse guard and the payment-filter disconnect interacted badly. Skipping
accounts `toAccount` cannot read left an empty list, which the disconnect
rule then took to mean the wallet had reported nothing — so a single `change`
event carrying only a half-initialized account disconnected the user and
deleted the connected shim, leaving the next page load unable to reconnect
either. That is the exact state the reconnect poll exists to ride out.
Treat a non-empty batch that parses to nothing as transient and ignore the
event. An empty batch, or one that parses to no payment address, still
disconnects.
`onDisconnect` also no longer depends on its storage write: a blocked
`localStorage` threw before `emit('disconnect')`, leaving the store connected
with no usable address — the state the payment-filter fix targets.
An empty interactive selection read `accounts[0].address` and threw a TypeError, which the catch relabelled as `UserRejectedRequestError` — so a user who simply holds no Bitcoin account saw raw JavaScript text and was told they had rejected something. The reconnect path already classified that state correctly; the interactive one now does too. A reconnect that finds no session left the connected shim in place, and `reconnect` swallows the error, so a site revoked while the tab was closed never self-healed: every later load paid the full poll and sat in `reconnecting`. Clear the shim there. `onDisconnect` never released the events subscription — only `disconnect` did, and this connector now reaches `onDisconnect` from `onAccountsChanged` too. A later connect would see a truthy `unsubscribe`, skip reattaching, and stay bound to a wallet object MetaMask had replaced, so account switches stopped reaching the widget. The poll guarded each account but not the `accounts` getter itself. A getter that throws while the wallet initializes ended the poll on its first tick, which is exactly the case the poll exists to survive.
…ow load Two regressions from the previous commit. Moving `getAccounts()` out of the `try` dropped the `UserRejectedRequestError` mapping from the one call a user can actually reject, so a decline surfaced as MetaMask's raw error and consumers lost the documented contract. The surviving `try` then wrapped only the subscribe and the storage writes, so a blocked `localStorage` in an embedded iframe told the user they had rejected a connection that had in fact succeeded. Wrap just the interactive call, as `binance` does, and let the storage write fail quietly. Clearing the connected shim when a reconnect found no session assumed the poll could tell "no session" from "no answer yet". It cannot: the wallet restores once per load, un-awaited, and swallows a failure, and a cold MV3 service worker takes seconds. A returning user with a valid session was signed out for good. Keep the shim — a genuine revoke still clears it through `onDisconnect` — and spend more of the budget `reconnect` already allows. No test covered the interactive rejection mapping, which is why the first regression went unnoticed; there is one now, and it fails without the fix.
chybisov
added
the
release-preview
Contributor
|
📦 Preview published under the Install the exact version(s) — npm i @bigmi/client@0.0.0-preview-f509fbf
npm i @bigmi/react@0.0.0-preview-f509fbf |
github-actions
Bot
removed
the
release-preview
Merged
chybisov
added a commit
to lifinance/widget
that referenced
this pull request
Sep 21, 2026
Moves to `@bigmi/client` 0.10.4 and `@bigmi/react` 0.9.4, now that lifinance/bigmi#80 is merged and published. `0.10.4` carries what this branch depends on. The BitKeep and Binance detection fall-throughs matter most: asking the connector instead of re-deriving from `window` would otherwise narrow those two wallets rather than preserve them. It also stops a default `metamask()` connector prompting on page load, and keeps the store consistent when a wallet's own `disconnect()` throws. `@bigmi/core` stays on 0.9.2, which already carries the rejection mapping.
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
1 participant
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.
What
binance()'sgetInternalProviderreturned as soon aswindow.binancew3wexisted:A build that injects
binancew3wwithout abitcoinprovider therefore resolvedundefinedand never reached thewindow.unisat.isBinancefallback.Why it matters
The LI.FI widget's own detection accepted both paths (
binancew3w?.bitcoin || unisat?.isBinance), so the two disagreed: the widget listed a Binance wallet this connector could not resolve, and connecting bounced with no message.lifinance/widget#878 removes that duplicated detection and asks the connector instead, so this narrowing would otherwise become a visible regression — Binance disappearing from the list on those builds. Same shape as the BitKeep fix in #77.
unisat()continues to rejectisBinance, so the two connectors still cannot claim the same injection — covered by a test.Verified
binance.spec.ts, 4 tests, red before the change on thebinancew3w-without-bitcoincaseunisat.spec.ts5 tests andbitget.spec.ts2 tests still pass, confirming no connector collisionpnpm check,check:types,check:circular-deps,knip:check,testall clean