Page MenuHomePhabricator
Differential D13085

[Cashtab] [Alias] pt 6.5 - Optimize getAllTxHistory to only make API calls for uncached tx history pages
ClosedPublic
Actions

Authored by emack on Feb 1 2023, 06:06.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC0ea770622849: [Cashtab] [Alias] pt 6.5 - Optimize getAllTxHistory to only make API calls for…
Summary

T2551

Depends on D13065

This diff implements the alias caching component to minimize API calls for alias tx history retrieval.

Overview

  • upon load of Alias.js, it checks whether cached Alias objects exist. If it doesn't, then execute a full alias tx history retrieval.
  • If cached Alias objects do exist, verify whether a refresh is required by comparing the cached and onchain tx counts.
  • If a partial tx history refresh is required, retrieve the additional txs entries and concat them to the cached txs entries.
  • There are now two alias caching objects, one being aliasTxHistory which stores an array of txs objects, and the other being aliases which is the list of aliases and their addresses.
  • the updateAliases() function in useWallets updates the aliases cache object, while the updateCachedAliasTxHistory() function in useWallets updates the aliasTxHistory cache object
  • getAllTxHistory() in chronik.js has been overloaded with an optional customStartPage which is used to indicate whether it should retrieve all tx history or a partial one starting from that page supplied.

[Cashtab] [Alias] pt 1 - Create scaffold for new Identity component
[Cashtab] [Alias] pt 2 - Upgrade sendXec() to handle alias registration
[Cashtab] [Alias] pt 3 - Implement isAliasAvailable function
[Cashtab] [Alias] pt 4 - Implement isAddressRegistered function
[Cashtab] [Alias] pt 5 - Implement getAddressFromAlias function
[Cashtab] [Alias] pt 6.1 - Get latest alias tx count from payment address
[Cashtab] [Alias] pt 6.1.1 - Apply Promise.All approach for alias history retrieval
[Cashtab] [Alias] pt 6.2 - Implement getAliasesFromLocalForage
[Cashtab] [Alias] pt 6.3 - Implement updateAliases
[Cashtab] [Alias] pt 6.4 - Update getAliases() to extract both alias and address
[Cashtab] [Alias] pt 6.5 - Optimize getAllTxHistory to only make API calls for uncached tx history pages
[Cashtab] [Alias] pt 6.6 - Render list of Aliases owned by active wallet in Alias.js
[Cashtab] [Alias] pt 6.7 - Deprecate caching related mocks, @TODOs and reviewer logs
[Cashtab] [Alias] pt 7 - Mitigate edge cases for registration records (same rego in same block, validate emoji char counts)
[Cashtab] [Alias] pt 8 - Enable alias lookup for Send XEC component
[Cashtab] [Alias] pt 9 - Enable alias lookup for Send Token component
[Cashtab] [Alias] pt 10 - Upgrade tx history to recognize alias registration txs
[Cashtab] [Alias] pt 11 - Set final registration fees and remove residual dev logs

Test Plan
  • npm test
  • npm start
  • check IndexedDB keyvaluepairs and ensure aliasTxHistory and aliases cache objects do not exist
  • navigate to the Alias page for the first time, refresh IndexedDB and verify the above cache objects have now been created and the array length in aliasTxHistory and the totalPaymentTxCount param in aliases both match the transaction count on explorer.e.cash. Also verify in the console log a full tx history retrieval was triggered
  • register a few new aliases then navigate away from and then back to the Alias page. Verify the console log is indicating partial tx history retrieval required and partial tx history retrieval starting from page x, rather than a full tx retrieval
  • verify in IndexedDB that the cached tx history prior to the partial tx history retrieval has not been lost and is incremented by the new aliases you just registered
  • navigate away from and back to the Alias page and validate via the console log that the cached and onchain tx counts are in sync and no tx retrieval is triggered
  • attempt to register one of the newly registered aliases and ensure the alias duplication error notification is displayed and it is not broadcasted

new test cases for changes to cashtabCache:

  • open the alias page in a fresh new browser with no existing cache and validate the previous undefined exception in isAddressRegistered does not appear and the console log is displaying invalid cashtabCache detected, initializing to currency.defaultCashtabCache.
  • validate a valid cashtabCache detected log is displayed in console after refresh.
  • open a different browser which has previously visited a version of cashtab before this cashtabCache update and validate invalid cashtabCache detected, initializing to currency.defaultCashtabCache is displayed in console, followed by valid cashtabCache detected after refresh.
  • register a new alias, refresh the page, and ensure cashtabCache is still valid via console log. In localforage keypairs ensure aliases in aliasCache has been updated with your new alias and paymentTxHistory and totalPaymentTxCount params incremented correctly.
  • create a new wallet, switch to it, navigate to the Alias page and ensure no errors in the console log.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emack created this revision.Feb 1 2023, 06:06
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 1 2023, 06:06
emack requested review of this revision.Feb 1 2023, 06:06
Harbormaster completed remote builds in B21905: Diff 37777.Feb 1 2023, 06:09
bytesofman requested changes to this revision.Feb 1 2023, 17:04
  1. Rebase required up the stack, can't arc patch anymore
  2. Let's move away from using BigInt for tx count, normal js number is ok
  3. Other comments
web/cashtab/src/components/Alias/Alias.js
91 ↗(On Diff #37777)

While using BigInt is technically prudent, I'm going to make the call that we can assume that we will never have 9 quadrillion transactions. Just use standard JS number for txCount.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isSafeInteger

98 ↗(On Diff #37777)

Getting onchainAliasTxCount should be its own function with unit tests

so, lines 98-138 here

126 ↗(On Diff #37777)

const txsCountOnLastPage = txs.length;

128 ↗(On Diff #37777)

if (txsCountOnLastPage === 25) {

130 ↗(On Diff #37777)

onchainAliasTxCount = numPages * 25;

134 ↗(On Diff #37777)

remove BigInt

149 ↗(On Diff #37777)

remove BigInt

web/cashtab/src/utils/chronik.js
55 ↗(On Diff #37777)

export const getAllTxHistory = async (chronik, hash160, customStartPage=0) => {

to set a default value if the function is called without a specified customStartPage

59 ↗(On Diff #37777)

remove this if/else loop and instead work with the default customStartPage = 0

66 ↗(On Diff #37777)

No BigInt

89 ↗(On Diff #37777)

function should be designed so we don't need if (customStartPage); customStartPage is just 0 if not set

197 ↗(On Diff #37777)

was this added to catch a specific type of tx? if so what tx?

This revision now requires changes to proceed.Feb 1 2023, 17:04
emack updated this revision to Diff 37811.Feb 3 2023, 03:02

rebase

Harbormaster completed remote builds in B21932: Diff 37811.Feb 3 2023, 03:05
bytesofman requested changes to this revision.Feb 3 2023, 23:10

Ran npm start

Local storage has items aliasTxHistory and aliases

Navigated to Alias screen

dev console:

cached Alias Tx Count:  84n
bundle.js:2012 onchain numPages:  4
bundle.js:2032 onchain Alias Tx Count:  90n
bundle.js:2037 partial tx history retrieval required
bundle.js:18492 partial tx history retrieval starting from page 3
bundle.js:2105 Does this active wallet have an onchain alias? : false

What cache items changed to:

image.png (362×511 px, 56 KB)

vs before when count was 84 and aliases seemed like the right kind of object.

refresh on alias page gests this in dev console:

image.png (128×511 px, 18 KB)

also bumps length of aliasTxHistory from 15 to 16, with the 16th entry being undefined

So actions here:

  1. See inline comments
  2. Some troubleshooting to handle issue above
  3. aliases cache stuff should go in the cashtabCache object, which is designed to hold everything cache related that needs to be shared between wallets
This revision now requires changes to proceed.Feb 3 2023, 23:10
emack edited the summary of this revision. (Show Details)Feb 6 2023, 22:14
emack updated this revision to Diff 37834.Feb 7 2023, 00:17
emack marked 12 inline comments as done.
  • Created standalone getOnchainAliasTxCount() function in chronik.js. As per tg chat, this has now been further split into two functions, one to retrieve the two pages and one (calculateAliasTxCount) to parse the total onchain tx count which can now be unit tested with txHistoryLastPageResponse mocks.
  • Updated getAllTxHistory() to use default customStartPage params throughout the function
  • Fixed the undefined entry in the aliasTxHistory cache object. Root cause was allTxHistory in Alias.js being initialized as an undefined variable rather than an empty array
  • Fixing the undefined bug above also means there is no further need for the outputScript validation check at the top of getAliasAndAddresses() function
  • Added logic to remove duplicate payment tx history entries in Alias.js by traversing through the partially retrieved alias txs and only pushing unique entries to the cache object. (the edge case of duplicate regos with different txids will be handled later)
  • Fixed the uncaught exception when refreshing the alias component. Root cause was the wallet input being undefined in Alias.js being passed to isAddressRegistered() when the page is loaded or reloaded directly, even though Alias.js is retrieving wallet object from ContextValue. Have added an edge case check that redirects the user to wallet to active the wallet object first.
  • merged aliasTxHistory into the same aliases object and then migrated it to the existing cashtabCache object and updated references throughout stack. The aliasCache is now structured as:
cashtabCache{ 
	aliasCache{
		aliases[...],
		paymentTxHistory[...],
		totalPaymentTxCount,
	},
	...
}
  • Moved from BigInt usage to Number
emack edited the summary of this revision. (Show Details)Feb 7 2023, 22:29
bytesofman requested changes to this revision.Feb 7 2023, 23:18
bytesofman added inline comments.
web/cashtab/src/components/Alias/Alias.js
112 ↗(On Diff #37834)

calculate this as a variable before calling the function, for better code readability

206 ↗(On Diff #37834)

why was this needed?

refreshing the page or navigating to alias directly should still be ok since wallet is coming from context.

EDIT: nm I think I found out the issue this was trying to solve. Should be fixed by adding wallet.name to this useEffect loop containing the isAddressRegistered function. Without this, that loop is only running on page load -- it should actually be running when any of the parameters it depends on change (and there are many of them).

Whenever you have a useEffect loop that contains parameters defined outside the loop, react will give you some haunted type bugs if those parameters are not listed as dependencies in the useEffect loop.

217 ↗(On Diff #37834)

Replace [] with wallet.name

We don't want to run this every time the wallet state changes. We want to run it when wallet is defined (this should happen on load. you'll want to test with console.log statements. Will fire when the page loads, then will fire again when wallet goes from undefined to the active wallet. So, you probably want something at the beginning fo this useEffect block like if( typeof wallet === 'undefined') {return}

web/cashtab/src/utils/chronik.js
55 ↗(On Diff #37834)

25 should be a param in Ticker.js, not hardcoded

Should also be an input param to the calculateAliasTxCount function so you can have unit tests with other figures.

We may find out later that 25 is not the best value to use here.

350 ↗(On Diff #37834)

Fun new complication here but we should do it the right way

We have a function isValidCashtabCache in validation.js that is called when Cashtab is loaded. This is where we should be parsing for the existence of aliasCache and, if it's not there, creating it.

This revision now requires changes to proceed.Feb 7 2023, 23:18
emack edited the test plan for this revision. (Show Details)Feb 9 2023, 13:43
emack updated this revision to Diff 37864.Feb 9 2023, 13:46
emack marked 4 inline comments as done.
  • fixed the alias.js refreshing exception by adding wallet.name as an useEffect loop param and adding check to only run this block when wallet is defined
  • moved logic calculating pages to traverse to outside of the await getAllTxHistory() call in Alias.js
  • moved 25 txs param into Ticker.js as currency.chronikTxsPerPage
  • updated cashtabCaching logic where isValidCashtabCache will now validate the aliasCache object and defaultCashtabCache in Ticker.js now updated with the expected structure of aliasCache. See additional test cases above to test this.
  • updated unit tests for isValidCashtabCache in validation.test.js to factor for new aliasCache params
Harbormaster completed remote builds in B21967: Diff 37864.Feb 9 2023, 13:48
bytesofman requested changes to this revision.Feb 9 2023, 18:05

Nice -- this works much better for me in testing and gets rid of the useHistory complication. Some nits related to cache and local storage.

web/cashtab/src/hooks/useWallet.js
1221 ↗(On Diff #37864)

This 'works.' But it will also nuke the cached tokenInfoById.

See the parseInvalidSettingsForMigration in validation.js for an example of how this is handled with settings. Instead of setting the default, you want to call a function that looks at the cache object, finds what is missing, and sticks it in.

At this point, it would make this diff more complicated, and it could be added later -- so no need to add it here. But need to create this as a task.

1228 ↗(On Diff #37864)

Need to set currency.defaultCashtabCache in localForage, not just state. Right now this fails the test plan as loading a wallet without cached aliases throws the invalid cashtabCache detected comment in the dev console, but does not update localForage.

This revision now requires changes to proceed.Feb 9 2023, 18:05
emack edited the summary of this revision. (Show Details)Feb 9 2023, 22:25
emack edited the summary of this revision. (Show Details)
emack updated this revision to Diff 37870.Feb 9 2023, 23:50
emack marked 2 inline comments as done.

setting aliasCache to localforage upon invalidation of the cashtabCache object. Tested via initial load of master cashtab and then applying this diff on top.

Harbormaster completed remote builds in B21971: Diff 37870.Feb 9 2023, 23:53
bytesofman accepted this revision.Feb 10 2023, 00:09
This revision is now accepted and ready to land.Feb 10 2023, 00:09
emack added a child revision: D13124: [Cashtab] [Alias] pt 6.6 - Render list of Aliases owned by active wallet in Alias.js.Feb 10 2023, 14:28
emack updated this revision to Diff 37945.Feb 17 2023, 23:10

rebase

Harbormaster completed remote builds in B22028: Diff 37945.Feb 17 2023, 23:12
Closed by commit rABC0ea770622849: [Cashtab] [Alias] pt 6.5 - Optimize getAllTxHistory to only make API calls for… (authored by emack). · Explain WhyFeb 22 2023, 04:06
This revision was automatically updated to reflect the committed changes.
emack added a commit: rABC0ea770622849: [Cashtab] [Alias] pt 6.5 - Optimize getAllTxHistory to only make API calls for….

Revision Contents
Changeset List

PathSize
web/
cashtab/
src/
components/
Alias/
159 lines
__tests__/
__snapshots__/
2 lines
Common/
6 lines
hooks/
24 lines
utils/
__mocks__/
2458 lines
320 lines
__tests__/
19 lines
22 lines
138 lines
28 lines

Diff 37987

View Options

web/cashtab/src/components/Alias/Alias.js

Loading...
View Options

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

Loading...
View Options

web/cashtab/src/components/Common/Ticker.js

Loading...
View Options

web/cashtab/src/hooks/useWallet.js

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

web/cashtab/src/utils/chronik.js

Loading...
View Options

web/cashtab/src/utils/validation.js

Loading...