Refactor account utxos #1184
Refactor account utxos #1184
Conversation
DavidTranDucVL
force-pushed
the
refactor-account-utxos
branch
2 times, most recently
from
f8bd892 to
8a36cb3
Compare
DavidTranDucVL
force-pushed
the
refactor-account-utxos
branch
from
8a36cb3 to
c67ee9f
Compare
DavidTranDucVL
temporarily deployed
to
adalite-refactor-accoun-3yqri0
| const accountXpubs = await getAccountXpubs() | ||
| const stakingXpub = await getStakingXpub(cryptoProvider, accountIndex) | ||
| const stakingAddress = await myAddresses.getStakingAddress() | ||
| const {baseAddressBalance, nonStakingBalance, balance, tokenBalance} = await getBalance() | ||
| const utxos = await getUtxos() | ||
| const shelleyAccountInfo = await getStakingInfo(validStakepoolDataProvider) |
There was a problem hiding this comment.
these network calls (getStakingInfo, getStakingHistory, ...) within fetchAccountInfo should probably be prefixed "fetch" as well for the sake of consistency, and if they are not exported, I'd prefix them with an underscore for the sake of clarity given we don't have this implemented as TS classes to capture that with private methods
| // must be called before using other methods of the account object | ||
| async function loadCurrentState(validStakepoolDataProvider: StakepoolDataProvider) { | ||
| const [accountInfo, newUtxos] = await Promise.all([ | ||
| _fetchAccountInfo(validStakepoolDataProvider), |
There was a problem hiding this comment.
I'd put here a clarifying comment on why we want to keep account info and utxos in sync (to prevent the transaction history/balnce shown from being behind the app's set of utxos, which could lead to multiple transactions going through if the set of utxos was updated meanwhile.)
Also, I just realized that this logic doesn't strictly ensure that utxos are at most as old as the rest of the account info, even if we awaited the utxos before the account info, because the _fetchUtxos() call is actually coupled and depends on the same cached source of data as account info does - the transaction history call is used both for address discovery and address info retrieval :/ Moreover, if loadCurrentState() was called too frequently, more often than our current cache timeout of 5 seconds, the utxos can actually get ahead of the transaction history if they happen to be updated during that time.
I can see this problem being solved by refactoring our isSomeAddressUsed call in the blockchain explorer to use our backend call to check whether addresses are used, instead of transaction history and cache that one, as that's actually the call we use repeatedly in other depending calls. getTransactionHistory as a bonus would no longer have to chunk the requests by the gap limit as it would no longer be coupled with account discovery (though at the cost of few more calls to filter used addresses, but these are pretty lightweight anyway). These non-trivial dependencies between the calls/caching would deserve also some comment explaining them as I can see that being pretty confusing - either here or in blockchain explorer
Then the fetching of utxos ahead of the account info would actually give us the guarantees we want.
It's a bit tedious refactor, but I think it's for the better and I'd propose doing it before merging this PR
There was a problem hiding this comment.
ok not really, still the caching behind getAddressInfos call (needed e.g. for getBalance()) can cause it to get behind the utxos, this call should probably have a way to invalidate the address infos cache to prevent that from happening
There was a problem hiding this comment.
finally, we may relax the requirement of transaction history having to be younger than the utxos and at least make sure to have the balance shown in sync with the utxos and leave the scenarios described above as an edge-case which if happens by any chance, user still sees balance in sync with the utxos, feel free to think of other solutions/tradeoffs
refi93
added
needs work
on hold
and removed
needs review
Resolves issues #1181