Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Background wallet/node refactor: wallet-as-plugin, recover failed connections, clean up and remove unused or redundant code #397

Merged
merged 33 commits into from
Sep 6, 2021

Conversation

pinheadmz
Copy link
Contributor

@pinheadmz pinheadmz commented Jul 28, 2021

DON'T PANIC

Sorry this is a big one.

The original goal of this PR was to restore the wallet-as-a-plugin configuration when the hsd full node is local (as opposed to remote RPC mode) which is the default. This ended up leading to a ton of refactoring in the node/wallet background services as well as node and wallet actions. There are so many pitfalls and footguns left open for the user that I wanted to cover. Along the way I noticed a good deal of redundant code and unused code and unnecessary code and I cleaned that all up as well. Reviewers may want to read each commit separately (I did a few git rebases so the PR should makes sense like this).

New features:

  • By default, hsd node is run locally with wallet as plugin
  • User can switch connection to remote full node, in which case wallet is run as WalletNode (independent process)

Things we want to be able to do without crashing and without node or wallet state appearing wrong in the UI:

  • In any configuration, reload the UI (ctrl-R or change a UI file in dev mode)
  • In any configuration, shut down and restart the application
  • When switching to remote RPC mode, test the connection FIRST before switching (this includes "correct network" test)
  • In any configuration, switch networks. If remote node is on wrong network, allow user to recover by switching to internal node or re-setting remote node options
  • If remote node drops the connection, warn user immediately and allow them to recover by reconfiguring

Additional notes to Bob devs:

  • The "master" network is now set by the WALLET. The value in the network switcher is set by the wallet, because we might not know anything about the remote full node, so we need to think of Bob from the perspective of the WALLET -- that is attached (one way or another) to some full node, which is secondary.

  • UI reloads will wipe out the redux store but not interrupt the background services. That means anything that is "dispatched to main window" from the background must be sent again if the UI is refreshed. You can see in the service files in the "start" functions that if the service is already running, I just dispatch that service's relevant properties and return.

  • Node details are dispatched by the node service, wallet details are dispatched by the wallet service. I separated the concerns.

  • I removed a bunch of lines like "ensure client" when a client isn't actually used in a function. I hope I didn't pull out anything critical but it seemed like there were a lot of unnecessary checks probably left over from even earlier architectures of Bob.

  • Both wallet and node services use http clients as well as direct library function calls, I think a good long-term goal would be to get rid of any unnecessary local loopback HTTP calls but a lot of time they are necessary because the hsd RPC modules do extra processing that we'd just have to duplicate in Bob.

  • After switching internal/external connections, a user may need to rescan the wallet (especially if the internal node is out of sync) or simply refresh the UI.

  • The remote RPC settings are global but could be saved in the Bob db for each network separately

  • isRunning has a more general meaning now: Basically in either configuration, the START event switches this value to true and the STOP event switches it to false. In essence, it refers to the wallet instead of the node. Since an invalid or broken connection sends a STOP event, isRunning: false should always mean "no connection" -- remote node is not connected and internal node is not running.

Demo

https://youtu.be/_87mXSjbxrw

pinheadmz added 30 commits July 16, 2021 12:38

Verified

This commit was signed with the committer’s verified signature.
rithvikvibhu Rithvik Vibhu

Verified

This commit was signed with the committer’s verified signature.
rithvikvibhu Rithvik Vibhu
With this set, its easy to start up a local "remote" RPC node:

hsd --network=regtest --memory=true --port=10001 --http-port=10002 \
    --brontide-port=10003 --no-wallet --no-dns --only=127.0.0.1
@pinheadmz
Copy link
Contributor Author

@chikeichan I'm happy to just rebase on master branch any time as well, might be cleaner than these github merges like pinheadmz@7d7f8ca but whatever works for you

@chikeichan
Copy link
Contributor

i normally squash and merge anyways, just didn't want to ping you for a simple rebase.

...but i did miss a trailing comma in app/background/node/client.js:29 and app/background/wallet/client.js:62 in my merge commit tho, if you could push that changes up before i merge 😓

@pinheadmz
Copy link
Contributor Author

@chikeichan gotcha, fixed.

@nlydv
Copy link
Contributor

nlydv commented Aug 23, 2021

I've been dealing with some persistent problems with Bob lately where certain actions would result in a blank white UI that would force me to restart the app. Clicking domain manager in the sidebar consistently led to that (making the manager unusable). This would also usually occur as soon as I'd log into an alternative wallet/account (i.e. not the first option in the dropdown list).

All I was able to determine was that the bug would only occur when connected to a remote RPC node.
Building this PR on macOS, seems to have (as far as I can tell) solved whatever was leading to those issues.

So from an end-user perspective, I'd give it a positive review.

@chikeichan chikeichan merged commit 4ba29ad into kyokan:master Sep 6, 2021
@pinheadmz pinheadmz deleted the walletplugin branch July 1, 2022 15:13
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.

None yet

3 participants