Page MenuHomePhabricator
Differential D9885

[Cashtab] Standardize use of wallet state parameters
ClosedPublic
Actions

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

Details

Reviewers
deadalnix
majcosta
Group Reviewers
Restricted Project
Commits
rABC74482297efa2: [Cashtab] Standardize use of wallet state parameters
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bytesofman created this revision.Aug 18 2021, 20:37
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 18 2021, 20:37
bytesofman requested review of this revision.Aug 18 2021, 20:37
Harbormaster completed remote builds in B16340: Diff 29434.Aug 18 2021, 20:39
bytesofman updated this revision to Diff 29435.Aug 18 2021, 22:26

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

Harbormaster completed remote builds in B16341: Diff 29435.Aug 18 2021, 22:29
bytesofman updated this revision to Diff 29436.Aug 18 2021, 23:44

Changing getWalletState to use return early instead of if else

bytesofman updated this revision to Diff 29437.Aug 18 2021, 23:45

Comment cleanup

Harbormaster completed remote builds in B16342: Diff 29436.Aug 18 2021, 23:46
Harbormaster completed remote builds in B16343: Diff 29437.
deadalnix accepted this revision.Aug 19 2021, 15:00
This revision is now accepted and ready to land.Aug 19 2021, 15:00
bytesofman updated this revision to Diff 29442.Aug 19 2021, 21:04

Bug fix in the update function removes need for previousWallet placeholder

Harbormaster completed remote builds in B16347: Diff 29442.Aug 19 2021, 21:07
bytesofman edited the summary of this revision. (Show Details)Aug 19 2021, 21:07
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
bytesofman updated this revision to Diff 29444.Aug 19 2021, 21:30

Stop exporting unused previousWallet, remove tautological unit tests

Harbormaster completed remote builds in B16348: Diff 29444.Aug 19 2021, 21:34
bytesofman updated this revision to Diff 29445.Aug 19 2021, 21:42

Refactor getWalletState for better readability

Harbormaster completed remote builds in B16349: Diff 29445.Aug 19 2021, 21:45
majcosta accepted this revision.Aug 19 2021, 22:41
This revision is now accepted and ready to land.Aug 19 2021, 22:41
Closed by commit rABC74482297efa2: [Cashtab] Standardize use of wallet state parameters (authored by Joey King <joseph.roy.king@gmail.com>). · Explain WhyAug 20 2021, 13:43
This revision was automatically updated to reflect the committed changes.
Joey King <joseph.roy.king@gmail.com> added a commit: rABC74482297efa2: [Cashtab] Standardize use of wallet state parameters.

Revision Contents
Changeset List

PathSize
web/
cashtab/
src/
components/
19 lines
Send/
22 lines
18 lines
__tests__/
15 lines
20 lines
__snapshots__/
Tokens/
190 lines
__tests__/
15 lines
__snapshots__/
Wallet/
39 lines
__mocks__/
__tests__/
14 lines
__snapshots__/
hooks/
2 lines
utils/
15 lines

Diff 29446

View Options

web/cashtab/src/components/App.js

Loading...
View Options

web/cashtab/src/components/Send/Send.js

Loading...
View Options

web/cashtab/src/components/Send/SendToken.js

Loading...
View Options

web/cashtab/src/components/Send/__tests__/Send.test.js

Loading...
View Options

web/cashtab/src/components/Send/__tests__/SendToken.test.js

Loading...
View Options

web/cashtab/src/components/Send/__tests__/__snapshots__/Send.test.js.snap

Loading...
View Options

web/cashtab/src/components/Send/__tests__/__snapshots__/SendToken.test.js.snap

Loading...
View Options

web/cashtab/src/components/Tokens/Tokens.js

Loading...
View Options

web/cashtab/src/components/Tokens/__tests__/Tokens.test.js

Loading...
View Options

web/cashtab/src/components/Tokens/__tests__/__snapshots__/Tokens.test.js.snap

Loading...
View Options

web/cashtab/src/components/Wallet/Wallet.js

Loading...
View Options

web/cashtab/src/components/Wallet/__mocks__/walletAndBalancesMock.js

Loading...
View Options

web/cashtab/src/components/Wallet/__tests__/Wallet.test.js

Loading...
View Options

web/cashtab/src/components/Wallet/__tests__/__snapshots__/Wallet.test.js.snap

Loading...
View Options

web/cashtab/src/hooks/useWallet.js

Loading...
View Options

web/cashtab/src/utils/cashMethods.js

Loading...