Page MenuHomePhabricator

[Cashtab] Standardize use of wallet state parameters
ClosedPublic

Authored by bytesofman on Aug 18 2021, 20:37.

Details

Summary

T1737

Cashtab switched from keeping distinct state parameters in state to keeping all state parameters in an object called wallet.state. This was done to dramatically speed up loading times by loading cached wallet state from local storage instead of polling the API every time the app opened.

Because this was done with active users with wallets that did not have this state parameter, some kind of migration system had to be devised. This was rolled out incrementally as some screens did not exist until after the migration (Tokens.js).

This diff standardizes how the newer wallet.state parameters are used.

  1. Screens are simply not rendered until an unmigrated wallet has migrated (at this point, very unlikely anyone would have an unmigrated wallet)
  2. This simplifies and standardizes validation checks across the Send.js, SendToken.js, Tokens.js, and Wallet.js screens
  3. Bug fix in update function that made wallet temporarily invalid every time its state was updated

Some unit tests were removed because this diff changes how an invalid wallet is handled. Previously, rendering for invalid wallets was handled on a screen by screen basis. Now, the app will lock with a spinner until a valid wallet is loaded.

This refactor is necessary to prevent 2 sources of truth for wallet state. Another diff is still necessary to completely remove use of legacy wallet state parameters (they are still used for incoming transaction notifications).

Test Plan

npm start

  1. Send an XEC tx, observe spinner lock and unlock with no flash of different screens
  2. Send an eToken tx, observe spinner lock and unlock with no flash of different screens
  3. Receive an XEC tx, observe received notification, observe brief UI lock as utxo set updates with no flash of different screens
  4. Receive an eToken tx, observe receivd notification, observe brief UI lock as utxo set updates with no flash of different screen
  5. Switch wallets and send a tx, send a few txs, try to break it

Diff Detail

Repository
rABC Bitcoin ABC
Branch
wallet-state-single-source-truth
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 16343
Build 32549: Build Diffcashtab-tests
Build 32548: arc lint + arc unit

Event Timeline

Replacing confusing 2-level if-then cascade with unit-tested function to get wallet state

Changing getWalletState to use return early instead of if else

This revision is now accepted and ready to land.Aug 19 2021, 15:00

Bug fix in the update function removes need for previousWallet placeholder

majcosta requested changes to this revision.Aug 19 2021, 21:19
majcosta added a subscriber: majcosta.
majcosta added inline comments.
web/cashtab/src/components/Tokens/Tokens.js
29 ↗(On Diff #29442)

weird comma placement

web/cashtab/src/hooks/useWallet.js
1382 ↗(On Diff #29442)

is this still necessary?

grepping gives me:

[marco@marco-pc cashtab]$ git grep previousWallet && git grep previousWsCashAddress && git grep previousWsSlpAddress
src/hooks/useWallet.js:    const previousWallet = usePrevious(wallet);
src/hooks/useWallet.js:            const previousWsCashAddress = previousWallet.Path145.legacyAddress;
src/hooks/useWallet.js:            const previousWsSlpAddress = previousWallet.Path245.legacyAddress;
src/hooks/useWallet.js:            const previousWsCashAddress = previousWallet.Path145.legacyAddress;
src/hooks/useWallet.js:                        addr: previousWsCashAddress,
src/hooks/useWallet.js:                    `Unsubscribed from BCH address at ${previousWsCashAddress}`,
src/hooks/useWallet.js:            const previousWsSlpAddress = previousWallet.Path245.legacyAddress;
src/hooks/useWallet.js:                        addr: previousWsSlpAddress,
src/hooks/useWallet.js:                    `Unsubscribed from SLP address at ${previousWsSlpAddress}`,

same for other instances of previousWallet in components/Wallet/Wallet.js

web/cashtab/src/utils/__tests__/cashMethods.test.js
11 ↗(On Diff #29442)

remove

126–142 ↗(On Diff #29442)

these tests are basically checking whether if statements work in javascript at this point. I think we can assume they do. :)

web/cashtab/src/utils/cashMethods.js
166–176 ↗(On Diff #29442)

now you could easily happy path this, if (weird shit) return the empty scaffolding, and just end the function with return wallet.state

This revision now requires changes to proceed.Aug 19 2021, 21:19

Stop exporting unused previousWallet, remove tautological unit tests

Refactor getWalletState for better readability

This revision is now accepted and ready to land.Aug 19 2021, 22:41