Page MenuHomePhabricator
Differential D12759

[Cashtab] [no BigNumber in indexedDb p1/2] Store etoken balances as strings
ClosedPublic
Actions

Authored by bytesofman on Dec 4 2022, 23:57.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABCbbfe2ff4213b: [Cashtab] [no BigNumber in indexedDb p1/2] Store etoken balances as strings
Summary

T2836, T2614

Stop storing token balances as BigNumber. Store as string. Do all calculations as BigNumber.

This prevents type loss when saved in and retrieved from indexedDb

Note: A hard migration is not required here. Cashtab still supports loading the old type. Wallets will eventually migrate to the new type as they are used.

Test Plan

On the master branch, create a few wallets with some tokens
Check out this diff
npm start
Activate one of the wallets
Wait to see Token balances stored as string in the dev console
Check wallet state in local storage to confirm token balances are saved as a string
Send a token tx
Burn a token
Confirm no dev log errors on SendToken page
Confirm no dev log error son CreateToken page
Create a token
Switch to another one of your wallets with token balance
Check dev log for This wallet has token balance stored as a BigNumber
Wait to see Token balances stored as string in the dev console
Switch back to original wallet
Check dev log for This wallet has token balance stored as a string

Diff Detail

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

Event Timeline

bytesofman created this revision.Dec 4 2022, 23:57
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 4 2022, 23:57
bytesofman requested review of this revision.Dec 4 2022, 23:57
bytesofman mentioned this in D12758: [Cashtab] Store token balances as string instead of BigNumber.Dec 4 2022, 23:58
bytesofman mentioned this in D12756: [Cashtab] Store token balances as string and not BigNumber p1/2.
Harbormaster completed remote builds in B21367: Diff 36968.Dec 5 2022, 00:00
bytesofman added a child revision: D12760: [Cashtab] [no BigNumber in indexedDb p2/2] Remove debug logging.Dec 5 2022, 00:01
emack requested changes to this revision.Dec 5 2022, 01:38
emack added a subscriber: emack.

Whilst the wallet state is all ok, the token balances are no longer being rendered on the token list view after balance conversion to string.

image.png (661×1 px, 154 KB)

This is due to /Home/TokenList.js (lines 15-17) calling formatTokenBalance() and passing in the raw token.balance param, which is now a string instead of a bignumber. That util function has a !BigNumber.isBigNumber() which will now fail. Either remove that check or format it as bignumber before passing into formatTokenBalance().

This revision now requires changes to proceed.Dec 5 2022, 01:38
bytesofman updated this revision to Diff 36977.Dec 5 2022, 04:06

Passing token.balance as a string to formatTokenBalance in TokenList.js

bytesofman added a comment.Dec 5 2022, 04:07
In D12759#284131, @emack wrote:

Whilst the wallet state is all ok, the token balances are no longer being rendered on the token list view after balance conversion to string.

image.png (661×1 px, 154 KB)

This is due to /Home/TokenList.js (lines 15-17) calling formatTokenBalance() and passing in the raw token.balance param, which is now a string instead of a bignumber. That util function has a !BigNumber.isBigNumber() which will now fail. Either remove that check or format it as bignumber before passing into formatTokenBalance().

Good catch. Opted for passing it as a BigNumber since, for now, we still want the wallet to work whether or not token.balance is a string or a BigNumber

Harbormaster completed remote builds in B21373: Diff 36977.Dec 5 2022, 04:09
emack requested changes to this revision.Dec 5 2022, 04:42

Looks like the SendToken component is also not rendering the token balance either.

image.png (180×305 px, 5 KB)

See line 476 for the same issue:

balance={token.balance}

just wrap it inline as a bignumber

This revision now requires changes to proceed.Dec 5 2022, 04:42
bytesofman updated this revision to Diff 36980.Dec 5 2022, 04:51

Wrapping another instance of token.balance in BigNumber initialization

Harbormaster completed remote builds in B21375: Diff 36980.Dec 5 2022, 04:55
emack requested changes to this revision.Dec 5 2022, 06:23

encountering a bug where importing an existing wallet on a fresh browser session (use incognito) throws the following exception. I had a look through Home.js and it wasn't obvious what was causing this. Seems to be implying parsedTxHistory is undefined when being passed to <TxHistory>
Note: This issue is not present in prod.

image.png (897×1 px, 180 KB)

web/cashtab/src/utils/cashMethods.js
894 ↗(On Diff #36980)

at the last step of the test plan I'm not seeing this console message when switching back to the original wallet. There's plenty of Token balances stored as string though.

image.png (467×470 px, 88 KB)

This revision now requires changes to proceed.Dec 5 2022, 06:23
bytesofman added a comment.Dec 5 2022, 11:19
In D12759#284158, @emack wrote:

encountering a bug where importing an existing wallet on a fresh browser session (use incognito) throws the following exception. I had a look through Home.js and it wasn't obvious what was causing this. Seems to be implying parsedTxHistory is undefined when being passed to <TxHistory>
Note: This issue is not present in prod.

image.png (897×1 px, 180 KB)

I ran into this yesterday trying to test T2822, and created T2837 to catch it.

It's a weird condition. I think it's related to parsedTxHistory not being ready "in time" in some cases. I was able to repeat it in a new incognito window both at cashtab.com and at localhost:3000/, then it stopped happening. So, I do not think it is related to this diff. The component that the app is trying but failing to .map in this case is txs in <TxHistory>, which is related to parsedTxHistory and not tokens

bytesofman updated this revision to Diff 36991.Dec 5 2022, 11:22

Correcting logic error in if check

bytesofman added inline comments.Dec 5 2022, 11:23
web/cashtab/src/utils/cashMethods.js
894 ↗(On Diff #36980)

oof yeah my logic check is off there. Pushed up a correction.

Harbormaster completed remote builds in B21380: Diff 36991.Dec 5 2022, 11:28
emack accepted this revision.Dec 5 2022, 11:49
This revision is now accepted and ready to land.Dec 5 2022, 11:49
Closed by commit rABCbbfe2ff4213b: [Cashtab] [no BigNumber in indexedDb p1/2] Store etoken balances as strings (authored by bytesofman). · Explain WhyDec 5 2022, 12:37
This revision was automatically updated to reflect the committed changes.
bytesofman added a commit: rABCbbfe2ff4213b: [Cashtab] [no BigNumber in indexedDb p1/2] Store etoken balances as strings.
bytesofman mentioned this in D12770: [Cashtab] Prevent .map error in TxHistory.Dec 6 2022, 00:31
bytesofman mentioned this in rABC31595faa0358: [Cashtab] Prevent .map error in TxHistory.Dec 6 2022, 13:22

Revision Contents
Changeset List

PathSize
web/
cashtab/
src/
components/
Home/
3 lines
__tests__/
__snapshots__/
Send/
6 lines
__tests__/
__snapshots__/
hooks/
12 lines
utils/
__mocks__/
88 lines
20 lines
7 lines

Diff 36996

View Options

web/cashtab/src/components/Home/TokenList.js

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

web/cashtab/src/hooks/useWallet.js

Loading...
View Options

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

Loading...
View Options

web/cashtab/src/utils/cashMethods.js

Loading...
View Options

web/cashtab/src/utils/chronik.js

Loading...