Related to T2722. In order to show the trailing zeros on tokenBalance, a new function has been added. As the optionalLocale paramater was not being used in any of the instances of formatBalance related to eTokens, the new function does not accept optionalLocale as a param. If this is a good way forward I will add the unit tests for the new function.
Details
Details
- Reviewers
bytesofman - Group Reviewers
Restricted Project - Commits
- rABCe23741199731: [Cashtab] Token balance should show full decimals
cd web/cashtab && npm start
navigate to eTokens page
observe that for each token balance, the decimals, including trailing zeros, are kept as part of the balance.
navigate to sendToken page
observe that for the token balance, the decimals, including trailing zeros, are kept as part of the balance.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/cashtab/src/utils/formatting.js | ||
|---|---|---|
| 74 ↗ | (On Diff #36070) | Is unformattedBalance a big number or a string? We should be using BigNumber here somewhere, token balances specifically can be greater than the the accurate mathematical Number restrictions of JS |
| 81 ↗ | (On Diff #36070) | We should be returning a string for token balances, as they can be larger than numbers JS is able to accurately process |
| web/cashtab/src/utils/formatting.js | ||
|---|---|---|
| 74 ↗ | (On Diff #36070) | In both cases of formatTokenBalance, unformattedBalance is passed as a Big Number. |
Comment Actions
Responding to review feedback, formatTokenBalance returns a string, and Number has been replaced with BigNumber. A comment has been added to the diff to indicate the formatTokenBalance is passed unformattedBalance as a BigNumber.
Comment Actions
Add some unit tests as well so I can see what kind of inputs and outputs we are working with
| web/cashtab/src/utils/formatting.js | ||
|---|---|---|
| 76 ↗ | (On Diff #36088) | could this realistically happen? It seems weird that we would just assume tokenDecimal=2 when that is almost guaranteed to be wrong. If we don't have tokenDecimal, then the token balance should not be displayed. |
Comment Actions
| web/cashtab/src/utils/formatting.js | ||
|---|---|---|
| 76 ↗ | (On Diff #36088) | I agree, initially I was trying to follow the function that previously handled token balance, but it makes more sense to return undefined than to use a placeholder value. |
Comment Actions
- Some comments in the function would be useful. It looks like unformattedBalance will always be a BigNumber. Add this as a comment
- The key unit test we want to see is that formatTokenBalance(100,8) will come out as a string with 8 decimal places, 100.00000000
Comment Actions
Responding to review feedback, comment has been added to indicate parameter types, and unit test has been added per feedback.
Comment Actions
This is solving the problem of eToken balances being rounded, but it is introducing a new problem of eToken balances being difficult to read.
Cashtab in current state:
This diff:
The string should still be locale formatted, i.e. comma separators for North American users.
Comment Actions
Responding to review feedback. Because toLocaleString does not work on a string, the initial unformattedTokenBalance must be converted back to a number in order to be formatted as a localeString. As far as I can tell, using new BigNumber(unformattedTokenBalance).toLocalteString(optionalLocale, {minimumFractionDigits: tokenBalance}) has no effect which was the catalyst for the creation of this new function originally. The workaround is, as stated, do the initial conversion of unformattedTokenBalance to a BigNumber, get the appropriate amount of decimal places with toFixed(tokenDecimal), and in the next step, convert that string into a number which maintains the decimals from the previous step, and finally setting that to localeString with the minimumFractionDigits option set to tokenDecimal. I tried to find a more elegant way to do this, but this has been my best attempt today. Additionally, with consideration to the new optionalLocale parameter, checks have been added along with fallback values in case the optionalLocale is not provided to the function.
| web/cashtab/src/utils/formatting.js | ||
|---|---|---|
| 78 ↗ | (On Diff #36112) |
in this case, use optionalLocale === 'en'
|
| 91 ↗ | (On Diff #36112) | Can delete this if..{} if you use default parameter setting recommended above |
| 94 ↗ | (On Diff #36112) | let locale = defaultLocale
if (navigator && navigator.language) {
locale = navigator.language
} |
Comment Actions
Responding to review feedback, optionalLocale is now defaultLocale, and is defined as 'en', if navigator.language is available, locale is set by the browser.