Page MenuHomePhabricator

[Cashtab] Load from cache on startup
ClosedPublic

Authored by josephroyking on Thu, Apr 15, 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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested changes to this revision.Mon, Apr 19, 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.Mon, Apr 19, 08:17
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.

Responding to code review feedback

Fabien requested changes to this revision.Mon, Apr 19, 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.Mon, Apr 19, 19:15

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

Fabien requested changes to this revision.Tue, Apr 20, 07:45

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

web/cashtab/src/hooks/useWallet.js
142 ↗(On Diff #28214)

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.Tue, Apr 20, 07:45

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 ↗(On Diff #28214)

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

Implementing isValidStoredWallet in useWallet.js

This revision is now accepted and ready to land.Tue, Apr 20, 17:54