Skip to content

stores: no node calls until a wallet is picked - #4450

Open
TheSeydiCharyyev wants to merge 1 commit into
ZeusLN:masterfrom
TheSeydiCharyyev:fix/no-connection-before-wallet-selection
Open

stores: no node calls until a wallet is picked#4450
TheSeydiCharyyev wants to merge 1 commit into
ZeusLN:masterfrom
TheSeydiCharyyev:fix/no-connection-before-wallet-selection

Conversation

@TheSeydiCharyyev

Copy link
Copy Markdown

Description

Relates to issue: #4016

With Display > Select wallet on startup enabled, the wallet list opens without activating a wallet, but the previously used wallet was still contacted in the background.

Wallet.tsx handles the setting correctly — it returns before fetchData(). The requests come from somewhere else: BalanceStore sets up a reaction on the whole settings object in its constructor and fetches balances whenever hasCredentials() is true. During startup getSettings() runs several times, and each call assigns a freshly parsed settings object, so the reaction fires again and again. updateNodeProperties() has already loaded the credentials of nodes[selectedNode] by then, so those fetches go to the last used wallet.

Measured on an emulator with a local HTTP server standing in for the node: after a restart, with the wallet list open and nothing selected, the server received 10 requests — five /v1/balance/blockchain + /v1/balance/channels pairs, then quiet. It is a burst during startup, not polling. With this change it receives none, and selecting a wallet still connects normally.

The change

SettingsStore now carries walletSelectionPending.

It is raised in updateNodeProperties() — the same place the node credentials are loaded — so the latch is up before anything can react to them. My first attempt set it from Wallet.tsx instead, next to the navigation, and that was too late: the reaction had already fired five times with credentials by the time the view got there, which cut the requests from 10 to 6 rather than to 0.

It is cleared in setConnectingStatus(true), which every wallet activation path already calls, so new wallets created or imported from the startup list are covered too. Wallets.tsx clears it explicitly in the one branch that skips connecting because the node is already active.

The BalanceStore reaction returns early while the latch is set.

Scope

Only wallets whose credentials are a macaroon or an access key were affected, since hasCredentials() checks those two. An LNDHub wallet has neither, so it never triggered this — I reproduced against LNDHub first and saw no requests at all.

The node process itself never started: startLnd() runs inside fetchData(), which the existing guard already skips. What leaked were the balance requests.

Note on the test mock

stores/SettingsStore.test.ts mocked MigrationUtils without migrateSwapHostsToBoltz and migrateOlympusHostsToZeusLsp, so getSettings() threw into its own catch and never reached updateNodeProperties(). I added them — without that the new tests would have passed without exercising anything.

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Configuration change
  • Locales update
  • Quality assurance
  • Other

Checklist

  • I've run yarn run tsc and made sure my code compiles correctly
  • I've run yarn run lint and made sure my code didn't contain any problematic patterns
  • I've run yarn run prettier and made sure my code is formatted correctly
  • I've run yarn run test and made sure all of the tests pass

Testing

If you modified or added a utility file, did you add new unit tests?

  • No, I'm a fool
  • Yes
  • N/A

Eight new tests: three in a new stores/BalanceStore.test.ts covering the reaction gate, and five in stores/SettingsStore.test.ts covering when the latch is raised and cleared. I checked each of them fails with the fix removed.

I have tested this PR on the following platforms (please specify OS version and phone model/VM):

  • Android — Android 16 (API 36), Pixel 9 Pro AVD, x86_64 emulator
  • iOS

I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):

On-device

  • LDK Node
  • Embedded LND

Remote

  • LND (REST) — pointed at a local stub server, which is what counts the requests
  • LND (Lightning Node Connect)
  • Core Lightning (CLNRest)
  • Nostr Wallet Connect
  • LndHub — local stub, used to confirm this path is not affected

Locales

  • I've added new locale text that requires translations
  • I'm aware that new translations should be made on the ZEUS Transfix page and not directly to this repo

Third Party Dependencies and Packages

  • Contributors will need to run yarn after this PR is merged in
  • 3rd party dependencies have been modified:
    • verify that package.json and yarn.lock have been properly updated
    • verify that dependencies are installed for both iOS and Android platforms

Other:

  • Changes were made that require an update to the README
  • Changes were made that require an update to onboarding

With Select wallet on startup enabled, the wallet list opens without activating a wallet, but balances were still fetched from the previously used one. BalanceStore reacts to the whole settings object and fetches whenever hasCredentials() is true. During startup getSettings() runs several times and each call assigns a freshly parsed settings object, so the reaction fired repeatedly with the credentials of the last used wallet. On an emulator this sent 10 requests to that wallet while the selection screen was open; with this change it sends none.

SettingsStore now carries walletSelectionPending. It is raised in updateNodeProperties, the same place node credentials are loaded, so the latch is up before anything can react to them. It is cleared by setConnectingStatus(true), which every wallet activation path already calls, and explicitly in the Wallets branch that skips connecting because the node is already active. The BalanceStore reaction returns early while it is set.

Only wallets whose credentials are a macaroon or an access key were affected, since hasCredentials() checks those two. The node process itself never started: startLnd() runs inside fetchData(), which the existing guard already skips.

The SettingsStore test mock was missing migrateSwapHostsToBoltz and migrateOlympusHostsToZeusLsp, so getSettings() threw into its catch and never reached updateNodeProperties. Without adding them the new tests would have passed without exercising anything.
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