Page MenuHomePhabricator
Differential D9424

[Cashtab] Load from cache on startup
ClosedPublic
Actions

Authored by bytesofman on Apr 15 2021, 14:28.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCf5e03e05bbb5: [Cashtab] Load from cache on startup
Summary

Building on D9393, this diff now adds utxo information to the cached wallet state. On app start, if the wallet has a cached state and utxo set is unchanged, the app pulls this in rather than querying server for full history.

Because BigNumber type information is lost in the caching to IndexedDb, a helper function is added to convert cached token utxo info back to what is calculated in the app. This function includes a unit test.

Test Plan

npm test

npm start
Switch between wallets and check for any strange behavior
Load time should be improved after you are reloading a wallet that has cached its utxo info

Diff Detail

Repository
rABC Bitcoin ABC
Branch
add-utxo-details-to-state
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 15537
Build 30994: Build Diffcashtab-tests
Build 30993: arc lint + arc unit

Event Timeline

bytesofman created this revision.Apr 15 2021, 14:28
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 15 2021, 14:28
bytesofman requested review of this revision.Apr 15 2021, 14:28
Harbormaster completed remote builds in B15522: Diff 28196.Apr 15 2021, 14:31
Fabien requested changes to this revision.Apr 19 2021, 08:17
Fabien added a subscriber: Fabien.
Fabien added inline comments.
web/cashtab/src/components/Wallet/Wallet.js
214 ↗(On Diff #28196)

Please avoid this kind of unrelated layout changes in the diff. This brings no value and distract from the actual changes to review

web/cashtab/src/hooks/useWallet.js
153 ↗(On Diff #28196)

If I understand correctly this has to be updated each time something is added to the wallet state, which is prone to errors. Is there a more generic way to ensure that all members of an object are non null ?

156 ↗(On Diff #28196)

Is there a future use for wallet.state ? Otherwise wallet.state.tokens seems to be enough

163 ↗(On Diff #28196)

are the same

172 ↗(On Diff #28196)

Remove else clause. In fact if there was no console logging this could be much simpler: return !_.isEqual(utxos, cachedUtxos);

This revision now requires changes to proceed.Apr 19 2021, 08:17
bytesofman added inline comments.Apr 19 2021, 14:26
web/cashtab/src/components/Wallet/Wallet.js
214 ↗(On Diff #28196)

noted -- will take this out in amend and take care to avoid in future.

web/cashtab/src/hooks/useWallet.js
153 ↗(On Diff #28196)

Yes, this is a bit overkill. The specific case I'm trying to catch here is cashtab users who are opening this new diff's version for the first time. The first time they load the app, the wallet object will not have the info necessary to populate without calling hydrateUtxos.

But, I figured in any event all of the data needs to be there in order to populate without calling hydrateUtxos, so might as well confirm this. I haven't seen cases in testing where one of these items is randomly missing, only the specific case where an older wallet loading this diff for the first time would, by design, not have the newer fields added in this diff.

It probably makes sense to have something like an isWalletStateValid function, could have more general use later and appropriate here. Will amend.

153 ↗(On Diff #28196)

Doing some research -- there are ways to make this check shorter, but I think it makes the code a bit harder to understand without offering any performance benefit. I think moving this logic into a described function (which may need to be expanded as more info is added to the wallet state in local storage) is the best approach.

156 ↗(On Diff #28196)

Function should convert a cached wallet state with invalid BigNumber items to a valid wallet state with the correct BigNumber types. It would be possible to limit this only to the token fields, but then the logic to insert this into wallet.state would need to go back into useWallet.js.

I think there's a good chance other fields (like txHistory) will need this same treatment, and I might come across other adjustments that need to be made to allow the wallet to treat the cached state like the from-API live state -- so I keeping the logic in this function makes sense.

I will update the function name to reflect this, right now it is named at the hyper-specific case of token type conversion (which is all it does now).

172 ↗(On Diff #28196)

Good point this is much better. console.log was just for my testing/debugging and doesn't need to be in the production app.

bytesofman updated this revision to Diff 28211.Apr 19 2021, 14:26

Responding to code review feedback

Harbormaster completed remote builds in B15535: Diff 28211.Apr 19 2021, 14:29
Fabien requested changes to this revision.Apr 19 2021, 19:15
Fabien added inline comments.
web/cashtab/src/hooks/useWallet.js
142 ↗(On Diff #28211)

if (isValidStoredWallet(wallet))

web/cashtab/src/utils/cashMethods.js
134 ↗(On Diff #28211)

just return the bool:

return walletStateFromStorage &&
        walletStateFromStorage.state &&
        walletStateFromStorage.state.balances &&
        walletStateFromStorage.state.utxos &&
        walletStateFromStorage.state.hydratedUtxoDetails &&
        walletStateFromStorage.state.parsedTxHistory &&
        walletStateFromStorage.state.slpBalancesAndUtxos &&
        walletStateFromStorage.state.tokens
This revision now requires changes to proceed.Apr 19 2021, 19:15
bytesofman updated this revision to Diff 28214.Apr 19 2021, 21:28

Modified isValidStoredWallet to return bool (required some changes, is now a more specific validation function)

Harbormaster completed remote builds in B15537: Diff 28214.Apr 19 2021, 21:31
Fabien requested changes to this revision.Apr 20 2021, 07:45

So the previous content of the if statement was not a bool ?

web/cashtab/src/hooks/useWallet.js
142

I don't get it, what is the purpose of isValidStoredWallet if it's only used in tests ? Shouldn't it be called here ?

This revision now requires changes to proceed.Apr 20 2021, 07:45
bytesofman added a comment.Apr 20 2021, 13:44
In D9424#213950, @Fabien wrote:

So the previous content of the if statement was not a bool ?

In testing, returning the previous content would actually return the wallet object. Some distinction between how JS processes that request in an if() vs simply returning it, or maybe this distinction just highlighted some laziness in my previous bool. The updated version is more specific, checking that wallet exists and is an object (vs before checking that it exists, with meeting the other conditions implying that it is an object).

web/cashtab/src/hooks/useWallet.js
142

Yup, this is just a somewhat embarrassing oversight on my part ;)

bytesofman updated this revision to Diff 28216.Apr 20 2021, 13:46

Implementing isValidStoredWallet in useWallet.js

Harbormaster completed remote builds in B15539: Diff 28216.Apr 20 2021, 13:49
Fabien accepted this revision.Apr 20 2021, 17:54
This revision is now accepted and ready to land.Apr 20 2021, 17:54
Closed by commit rABCf5e03e05bbb5: [Cashtab] Load from cache on startup (authored by Joey King <joseph.roy.king@gmail.com>). · Explain WhyApr 20 2021, 17:59
This revision was automatically updated to reflect the committed changes.
Joey King <joseph.roy.king@gmail.com> added a commit: rABCf5e03e05bbb5: [Cashtab] Load from cache on startup.

Revision Contents
Changeset List

PathSize
web/
cashtab/
src/
hooks/
49 lines
utils/
__mocks__/
__tests__/
23 lines
28 lines

Diff 28214

View Options

web/cashtab/src/hooks/useWallet.js

Loading...
View Options

web/cashtab/src/utils/__mocks__/mockCachedUtxos.js

Loading...
View Options

web/cashtab/src/utils/__mocks__/mockStoredWallets.js

Loading...
View Options

web/cashtab/src/utils/__tests__/cashMethods.test.js

Loading...
View Options

web/cashtab/src/utils/cashMethods.js

Loading...