stores: no node calls until a wallet is picked - #4450
Open
TheSeydiCharyyev wants to merge 1 commit into
Open
Conversation
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.
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
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.
Description
Relates to issue: #4016
With
Display > Select wallet on startupenabled, the wallet list opens without activating a wallet, but the previously used wallet was still contacted in the background.Wallet.tsxhandles the setting correctly — it returns beforefetchData(). The requests come from somewhere else:BalanceStoresets up a reaction on the wholesettingsobject in its constructor and fetches balances wheneverhasCredentials()is true. During startupgetSettings()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 ofnodes[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/channelspairs, 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
SettingsStorenow carrieswalletSelectionPending.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 fromWallet.tsxinstead, 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.tsxclears it explicitly in the one branch that skips connecting because the node is already active.The
BalanceStorereaction 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 insidefetchData(), which the existing guard already skips. What leaked were the balance requests.Note on the test mock
stores/SettingsStore.test.tsmockedMigrationUtilswithoutmigrateSwapHostsToBoltzandmigrateOlympusHostsToZeusLsp, sogetSettings()threw into its owncatchand never reachedupdateNodeProperties(). I added them — without that the new tests would have passed without exercising anything.This pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn't contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
Eight new tests: three in a new
stores/BalanceStore.test.tscovering the reaction gate, and five instores/SettingsStore.test.tscovering 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):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
On-device
Remote
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther: