fix: decimal places displayed on token value on permit pages #25410
Conversation
jpuri
added
confirmation-redesign
team-confirmations
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
jpuri
force-pushed
the
permit_token_decimal_fix
branch
from
b3833a3 to
b0b36b8
Compare
…sk-extension into permit_token_decimal_fix
Builds ready [ac15aa4]
Page Load Metrics (54 ± 4 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25410 +/- ##
===========================================
+ Coverage 69.60% 69.61% +0.01%
===========================================
Files 1364 1364
Lines 48173 48189 +16
Branches 13291 13297 +6
===========================================
+ Hits 33527 33542 +15
- Misses 14646 14647 +1 ☔ View full report in Codecov by Sentry. |
| @@ -42,9 +44,23 @@ const TypedSignInfo: React.FC = () => { | |||
|
|
|||
| const isPermit = isPermitSignatureRequest(currentConfirmation); | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
We have similar logic within useBalanceChanges in fetchErc20Decimals and it seems we have to account for base 10 and base 16 responses, plus checking it's a finite value.
Is it worth re-using that function and moving it to a util or even creating a hook such as useTokenDecimals?
There was a problem hiding this comment.
Writing a new hook could be useful, a small limitation here is that we are conditionally getting this for only permit types.
There was a problem hiding this comment.
Even if we don't make a hook or util, do we not have to match the support for base 16 responses or NaN values for the sake of stability?
There was a problem hiding this comment.
This value is not used for signatures that are not permit.
| @@ -65,7 +68,10 @@ const PermitSimulation: React.FC = () => { | |||
| paddingInline={2} | |||
| textAlign={TextAlign.Center} | |||
| > | |||
| {value} | |||
| {formatNumber( | |||
| value / Math.pow(10, tokenDecimals), | |||
There was a problem hiding this comment.
Rather than doing this math inline, is it worth including this within the formatNumber util or a new one?
We seem to also do this within dataTree.
Maybe a combined function such as applyDecimals?
There was a problem hiding this comment.
I think it will not be very useful to do division inside format number to get actual value of tokens. It is not a lot of calculation thus I I left it inline.
There was a problem hiding this comment.
It is indeed brief, but I'd argue it still limits the readability a little, plus introduces duplication.
It would be both the division and the pow call also.
There was a problem hiding this comment.
It looks bit like over-engineering to me to add a util method for this.
There was a problem hiding this comment.
It's certainly not a blocker so we can review in future if we develop additional usages.
Builds ready [673f415]
Page Load Metrics (48 ± 4 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [c5539fb]
Page Load Metrics (57 ± 7 ms)
Bundle size diffs
|
| if (value === undefined) { | ||
| return value; | ||
| } | ||
| const formatter = new Intl.NumberFormat('en-US', { |
There was a problem hiding this comment.
in the future, we may want to support formats based on the user's locale settings
There was a problem hiding this comment.
I've since looked into the code and I'd like to surface 2 other formatNumber/formatAccount* methods I've found used in our repo. Each have their own nuances. We utilize Intl.NumberFormat for each of the methods, but differently.
app/scripts/controllers/push-platform-notifications/utils/get-notification-data.ts:
- uses abbreviation / notation (K, M, B, T)
- uses ellipsis
- does not support locales; Uses en-US
ui/pages/confirmations/components/simulation-details/formatAmount.ts:
- neither abbreviated nor uses ellipsis
- displays all left numbers if value is >= 0
- rounds decimals based on desired precision
- supports locales which if I'm not mistaken only utilizes the benefit of commas vs decimals. Symbols are handled outside of the method.
since the related issue ticket mentions:
This is already done for simulations UI that exists on transactions confirmations in case there's logic we can re-use from there.
I'd suggest we extract the simulation details formatAmount method to be reusable across the repo. We could update the method to specify the precision number / significant decimal places. This suggestion would apply more cohesion across our app, reduce formatNumber/formatAmount* variants, and support i18n locale decimals/commas.
There was a problem hiding this comment.
regarding the last suggestion to remove formatNumber in favor of reusing the simulation's formatAmount method, I see you are doing in in a subsequent PR here https://github.com/MetaMask/metamask-extension/pull/25438/files#diff-27e9fac2551026bbb4a89ea07db94a27210316d346fed5e3234e696601e425b5L21. 👍🏼
other comment to support locale still applies
|
metamaskbot
added
the
release-12.2.0
Builds ready [ed96b06]
Page Load Metrics (141 ± 157 ms)
Bundle size diffs
|
Description
fix decimal places displayed on token value on permit pages
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2648
Manual testing steps
Screenshots/Recordings
Pre-merge author checklist
Pre-merge reviewer checklist