Skip to content
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

Merged
merged 26 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9e57e6e
fix: formatting of deadline in permit signature page
jpuri Jun 13, 2024
5ee78f2
fix
jpuri Jun 14, 2024
343b38a
fix
jpuri Jun 14, 2024
a15aaa1
fix
jpuri Jun 14, 2024
4e3dc68
fix
jpuri Jun 14, 2024
244eb3d
Merge branch 'develop' of github.com:MetaMask/metamask-extension into…
jpuri Jun 18, 2024
b6ddf18
Update
jpuri Jun 18, 2024
5960490
Merge branch 'develop' into permit_deadline
jpuri Jun 19, 2024
b0b36b8
merge
jpuri Jun 19, 2024
17b55f0
Merge branch 'permit_deadline' into permit_token_decimal_fix
jpuri Jun 19, 2024
448e558
update
jpuri Jun 19, 2024
a0b91ef
Merge branch 'permit_token_decimal_fix' of github.com:MetaMask/metama…
jpuri Jun 19, 2024
ac15aa4
update
jpuri Jun 19, 2024
435bd22
merge
jpuri Jun 23, 2024
a4568ae
Merge branch 'permit_deadline' of github.com:MetaMask/metamask-extens…
jpuri Jun 23, 2024
5c745db
merge
jpuri Jun 23, 2024
5ef63d3
update
jpuri Jun 24, 2024
673f415
Merge branch 'permit_deadline' into permit_token_decimal_fix
jpuri Jun 24, 2024
cb7eb4c
merge
jpuri Jun 25, 2024
c5539fb
Merge branch 'develop' into permit_token_decimal_fix
jpuri Jun 25, 2024
fabd406
Merge branch 'develop' into permit_token_decimal_fix
jpuri Jun 29, 2024
71a77d5
Merge branch 'develop' into permit_token_decimal_fix
jpuri Jul 1, 2024
e9735f6
Merge branch 'develop' into permit_token_decimal_fix
jpuri Jul 2, 2024
5edce91
Merge branch 'develop' into permit_token_decimal_fix
jpuri Jul 2, 2024
5ffc6bb
Merge branch 'develop' into permit_token_decimal_fix
jpuri Jul 2, 2024
ed96b06
Merge branch 'develop' into permit_token_decimal_fix
jpuri Jul 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/e2e/tests/confirmations/signatures/permit.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ async function assertInfoValues(driver: Driver) {
css: '.name__value',
text: '0x5B38D...eddC4',
});
const value = driver.findElement({ text: '3000' });
const value = driver.findElement({ text: '3,000' });
const nonce = driver.findElement({ text: '0' });
const deadline = driver.findElement({ text: '02 August 1971, 16:53' });

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ exports[`TypedSignInfo correctly renders permit sign type 1`] = `
class="mm-box mm-text mm-text--body-md mm-box--color-inherit"
style="white-space: pre-wrap;"
>
3000
3,000
</p>
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ exports[`PermitSimulation renders component correctly 1`] = `
<p
class="mm-box mm-text mm-text--body-md mm-text--text-align-center mm-box--padding-inline-2 mm-box--color-text-default mm-box--background-color-background-alternative mm-box--rounded-xl"
>
3000
30.00
</p>
</div>
<div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ describe('PermitSimulation', () => {
},
};
const mockStore = configureMockStore([])(state);
const { container } = renderWithProvider(<PermitSimulation />, mockStore);
const { container } = renderWithProvider(
<PermitSimulation tokenDecimals={2} />,
mockStore,
);
expect(container).toMatchSnapshot();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ import {
import { SignatureRequestType } from '../../../../../types/confirm';
import useTokenExchangeRate from '../../../../../../../components/app/currency-input/hooks/useTokenExchangeRate';
import { IndividualFiatDisplay } from '../../../../simulation-details/fiat-display';
import { formatNumber } from '../../../utils';
import { ConfirmInfoSection } from '../../../../../../../components/app/confirm/info/row/section';

const PermitSimulation: React.FC = () => {
const PermitSimulation: React.FC<{
tokenDecimals: number;
}> = ({ tokenDecimals }) => {
const t = useI18nContext();
const currentConfirmation = useSelector(
currentConfirmationSelector,
Expand Down Expand Up @@ -61,7 +64,10 @@ const PermitSimulation: React.FC = () => {
paddingInline={2}
textAlign={TextAlign.Center}
>
{value}
{formatNumber(
value / Math.pow(10, tokenDecimals),
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

@matthewwalsh0 matthewwalsh0 Jun 27, 2024

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

tokenDecimals,
)}
</Text>
</Box>
<Name value={verifyingContract} type={NameType.ETHEREUM_ADDRESS} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ import {
} from '../../../../../../../test/data/confirmations/typed_sign';
import TypedSignInfo from './typed-sign';

jest.mock('../../../../../../store/actions', () => {
return {
getTokenStandardAndDetails: jest.fn().mockResolvedValue({ decimals: 2 }),
};
});

describe('TypedSignInfo', () => {
it('renders origin for typed sign data request', () => {
const state = {
Expand Down
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';

Expand All @@ -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';
Expand All @@ -26,6 +27,7 @@ const TypedSignInfo: React.FC = () => {
const useTransactionSimulations = useSelector(
selectUseTransactionSimulations,
);
const [decimals, setDecimals] = useState<number>(0);

if (!currentConfirmation?.msgParams) {
return null;
Expand All @@ -38,9 +40,23 @@ const TypedSignInfo: React.FC = () => {

const isPermit = isPermitSignatureRequest(currentConfirmation);

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 NaN values for the sake of stability?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 && (
<>
Expand All @@ -64,6 +80,7 @@ const TypedSignInfo: React.FC = () => {
<ConfirmInfoRowTypedSignData
data={currentConfirmation.msgParams?.data as string}
isPermit={isPermit}
tokenDecimals={decimals}
/>
</ConfirmInfoRow>
</ConfirmInfoSection>
Expand Down
24 changes: 23 additions & 1 deletion ui/pages/confirmations/components/confirm/row/dataTree.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
ConfirmInfoRowDate,
ConfirmInfoRowText,
} from '../../../../../components/app/confirm/info/row';
import { formatNumber } from '../utils';

type ValueType = string | Record<string, TreeData> | TreeData[];

Expand All @@ -22,9 +23,11 @@ export type TreeData = {
export const DataTree = ({
data,
isPermit = false,
tokenDecimals = 0,
}: {
data: Record<string, TreeData> | TreeData[];
isPermit?: boolean;
tokenDecimals?: number;
}) => (
<Box width={BlockSize.Full}>
{Object.entries(data).map(([label, { value, type }], i) => (
Expand All @@ -42,6 +45,7 @@ export const DataTree = ({
isPermit={isPermit}
value={value}
type={type}
tokenDecimals={tokenDecimals}
/>
}
</ConfirmInfoRow>
Expand All @@ -54,14 +58,32 @@ const DataField = ({
isPermit,
type,
value,
tokenDecimals,
}: {
label: string;
isPermit: boolean;
type: string;
value: ValueType;
tokenDecimals: number;
}) => {
if (typeof value === 'object' && value !== null) {
return <DataTree data={value} isPermit={isPermit} />;
return (
<DataTree
data={value}
isPermit={isPermit}
tokenDecimals={tokenDecimals}
/>
);
}
if (isPermit && label === 'value') {
return (
<ConfirmInfoRowText
text={formatNumber(
parseInt(value, 10) / Math.pow(10, tokenDecimals),
tokenDecimals,
)}
/>
);
}
if (isPermit && label === 'deadline') {
return <ConfirmInfoRowDate date={parseInt(value, 10)} />;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,17 @@ import {
ConfirmInfoRow,
ConfirmInfoRowText,
} from '../../../../../../components/app/confirm/info/row';

import { DataTree } from '../dataTree';
import { parseSanitizeTypedDataMessage } from '../../../../utils';
import { DataTree } from '../dataTree';

export const ConfirmInfoRowTypedSignData = ({
data,
isPermit,
tokenDecimals,
}: {
data: string;
isPermit?: boolean;
tokenDecimals?: number;
}) => {
const t = useI18nContext();

Expand All @@ -35,7 +36,11 @@ export const ConfirmInfoRowTypedSignData = ({
<ConfirmInfoRowText text={primaryType} />
</ConfirmInfoRow>
<Box style={{ marginLeft: -8 }}>
<DataTree data={sanitizedMessage.value} isPermit={isPermit} />
<DataTree
data={sanitizedMessage.value}
isPermit={isPermit}
tokenDecimals={tokenDecimals}
/>
</Box>
</Box>
);
Expand Down
46 changes: 28 additions & 18 deletions ui/pages/confirmations/components/confirm/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,39 @@ import {
unapprovedPersonalSignMsg,
} from '../../../../../test/data/confirmations/personal_sign';
import { SignatureRequestType } from '../../types/confirm';
import { getConfirmationSender } from './utils';
import { formatNumber, getConfirmationSender } from './utils';

describe('getConfirmationSender()', () => {
test("returns the sender address from a signature if it's passed", () => {
const testCurrentConfirmation =
genUnapprovedContractInteractionConfirmation() as TransactionMeta;
const { from } = getConfirmationSender(testCurrentConfirmation);
describe('confirm - utils', () => {
describe('getConfirmationSender()', () => {
test("returns the sender address from a signature if it's passed", () => {
const testCurrentConfirmation =
genUnapprovedContractInteractionConfirmation() as TransactionMeta;
const { from } = getConfirmationSender(testCurrentConfirmation);

expect(from).toEqual(CONTRACT_INTERACTION_SENDER_ADDRESS);
});
expect(from).toEqual(CONTRACT_INTERACTION_SENDER_ADDRESS);
});

test("returns the sender address from a transaction if it's passed", () => {
const { from } = getConfirmationSender(
unapprovedPersonalSignMsg as SignatureRequestType,
);
test("returns the sender address from a transaction if it's passed", () => {
const { from } = getConfirmationSender(
unapprovedPersonalSignMsg as SignatureRequestType,
);

expect(from).toEqual(PERSONAL_SIGN_SENDER_ADDRESS);
});
expect(from).toEqual(PERSONAL_SIGN_SENDER_ADDRESS);
});

test('returns no sender address if no confirmation is passed', () => {
const testCurrentConfirmation = undefined;
const { from } = getConfirmationSender(testCurrentConfirmation);
test('returns no sender address if no confirmation is passed', () => {
const testCurrentConfirmation = undefined;
const { from } = getConfirmationSender(testCurrentConfirmation);

expect(from).toEqual(undefined);
});
});

expect(from).toEqual(undefined);
describe('formatNumber()', () => {
test('formats number according to decimal places passed', () => {
expect(formatNumber(123456, 2)).toEqual('123,456.00');
expect(formatNumber(123456, 0)).toEqual('123,456');
expect(formatNumber(123456, 7)).toEqual('123,456.0000000');
});
});
});
11 changes: 11 additions & 0 deletions ui/pages/confirmations/components/confirm/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

@digiwand digiwand Jul 1, 2024

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
};