-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: decimal places displayed on token value on permit pages #25410
Changes from all commits
9e57e6e
5ee78f2
343b38a
a15aaa1
4e3dc68
244eb3d
b6ddf18
5960490
b0b36b8
17b55f0
448e558
a0b91ef
ac15aa4
435bd22
a4568ae
5c745db
5ef63d3
673f415
cb7eb4c
c5539fb
fabd406
71a77d5
e9735f6
5edce91
5ffc6bb
ed96b06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import React from 'react'; | ||
import React, { useEffect, useState } from 'react'; | ||
import { useSelector } from 'react-redux'; | ||
import { isValidAddress } from 'ethereumjs-util'; | ||
|
||
|
@@ -11,6 +11,7 @@ import { | |
} from '../../../../../../components/app/confirm/info/row'; | ||
import { useI18nContext } from '../../../../../../hooks/useI18nContext'; | ||
import { currentConfirmationSelector } from '../../../../../../selectors'; | ||
import { getTokenStandardAndDetails } from '../../../../../../store/actions'; | ||
import { SignatureRequestType } from '../../../../types/confirm'; | ||
import { isPermitSignatureRequest } from '../../../../utils'; | ||
import { selectUseTransactionSimulations } from '../../../../selectors/preferences'; | ||
|
@@ -26,6 +27,7 @@ const TypedSignInfo: React.FC = () => { | |
const useTransactionSimulations = useSelector( | ||
selectUseTransactionSimulations, | ||
); | ||
const [decimals, setDecimals] = useState<number>(0); | ||
|
||
if (!currentConfirmation?.msgParams) { | ||
return null; | ||
|
@@ -38,9 +40,23 @@ const TypedSignInfo: React.FC = () => { | |
|
||
const isPermit = isPermitSignatureRequest(currentConfirmation); | ||
|
||
useEffect(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have similar logic within Is it worth re-using that function and moving it to a util or even creating a hook such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if we don't make a hook or util, do we not have to match the support for base 16 responses or There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This value is not used for signatures that are not permit. |
||
(async () => { | ||
if (!isPermit) { | ||
return; | ||
} | ||
const { decimals: tokenDecimals } = await getTokenStandardAndDetails( | ||
verifyingContract, | ||
); | ||
setDecimals(parseInt(tokenDecimals ?? '0', 10)); | ||
})(); | ||
}, [verifyingContract]); | ||
|
||
return ( | ||
<> | ||
{isPermit && useTransactionSimulations && <PermitSimulation />} | ||
{isPermit && useTransactionSimulations && ( | ||
<PermitSimulation tokenDecimals={decimals} /> | ||
)} | ||
<ConfirmInfoSection> | ||
{isPermit && ( | ||
<> | ||
|
@@ -64,6 +80,7 @@ const TypedSignInfo: React.FC = () => { | |
<ConfirmInfoRowTypedSignData | ||
data={currentConfirmation.msgParams?.data as string} | ||
isPermit={isPermit} | ||
tokenDecimals={decimals} | ||
/> | ||
</ConfirmInfoRow> | ||
</ConfirmInfoSection> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,3 +17,14 @@ export const getConfirmationSender = ( | |
|
||
return { from }; | ||
}; | ||
|
||
export const formatNumber = (value: number, decimals: number) => { | ||
if (value === undefined) { | ||
return value; | ||
} | ||
const formatter = new Intl.NumberFormat('en-US', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the future, we may want to support formats based on the user's locale settings There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've since looked into the code and I'd like to surface 2 other
since the related issue ticket mentions:
I'd suggest we extract the simulation details There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
minimumFractionDigits: decimals, | ||
maximumFractionDigits: decimals, | ||
}); | ||
return formatter.format(value); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks bit like over-engineering to me to add a util method for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's certainly not a blocker so we can review in future if we develop additional usages.