Skip to content

Commit 9911443

Browse files
authored
fix: simulation Fiat precision and Fiat flickers different value before decimals are applied (#13371)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** - Fix fiat precision. We should show expected digits instead of 0s. - Replaces areas where value is coerced to number - Fix related FiatDisplay test - invalid test where we called toBeDefined instead of toBeTruthy from queryByText - removed useFiatFormatter mock - fix invalid showFiatInTestnets mock - fix hideFiatForTestnet test related mocks since in useHideFiatForTestnet ```TEST_NETWORK_IDS.includes(chainId) && !showFiatInTestnets;``` was returning true in all cases as chainId was undefined. - Update FiatDisplay value to replace $100.00 → $100 and $100.2 → $100.20. useFiatFormatter formatted as expected. - Fixes fiat flickering issue while data is still being fetched - adds placeholder element while token details are pending ## **Related issues** Fixes: #13387 ## **Manual testing steps** 1. Go to https://develop.d3bkcslj57l47p.amplifyapp.com/ 2. Trigger Permit Batch ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <img width="320" src="https://github.com/user-attachments/assets/30dd5d66-91a3-4fc1-8a6a-c5ede4134c8c"> ### **After example** <img width="320" src="https://github.com/user-attachments/assets/c5ad4c15-bed8-4428-a7f0-bbcf37f5c9c0"> ### **Real after with hidden value if "Unlimited" is shown w/ some token icons still loading ** <img width="320" src="https://github.com/user-attachments/assets/b8452419-254a-46d8-93af-fd083535e949"> ### **Before flickering issue ** https://github.com/user-attachments/assets/52178518-04ed-49fd-8568-d40d1b6c0637 ### **After** https://github.com/user-attachments/assets/2ef9fc1a-bd0f-457b-bf20-9e5250b1df97 ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
1 parent 9798e95 commit 9911443

File tree

11 files changed

+187
-147
lines changed

11 files changed

+187
-147
lines changed

app/components/UI/SimulationDetails/FiatDisplay/FiatDisplay.test.tsx

Lines changed: 84 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -1,82 +1,67 @@
11
import React from 'react';
2+
import BigNumber from 'bignumber.js';
23
import { merge } from 'lodash';
4+
import { CHAIN_IDS } from '@metamask/transaction-controller';
35
import renderWithProvider from '../../../../util/test/renderWithProvider';
46
import { backgroundState } from '../../../../util/test/initial-root-state';
5-
67
import { IndividualFiatDisplay, TotalFiatDisplay } from './FiatDisplay';
7-
import { FIAT_UNAVAILABLE } from '../types';
8-
import useFiatFormatter from './useFiatFormatter';
98
import { mockNetworkState } from '../../../../util/test/network';
10-
import { CHAIN_IDS } from '@metamask/transaction-controller';
11-
12-
jest.mock('./useFiatFormatter');
13-
14-
const mockInitialState = {
15-
engine: {
16-
backgroundState,
17-
},
18-
};
9+
import { FIAT_UNAVAILABLE, FiatAmount } from '../types';
1910

20-
const mockStateWithTestnet = merge({}, mockInitialState, {
21-
engine: {
22-
backgroundState: {
23-
NetworkController: {
24-
...mockNetworkState({
25-
chainId: CHAIN_IDS.SEPOLIA,
26-
id: 'sepolia',
27-
nickname: 'Sepolia',
28-
ticker: 'ETH',
29-
}),
30-
},
11+
const mockStateWithTestnet = merge(
12+
{
13+
engine: {
14+
backgroundState,
3115
},
3216
},
33-
});
34-
35-
const mockStateWithShowingFiatOnTestnets = merge({}, mockStateWithTestnet, {
36-
engine: {
37-
backgroundState: {
38-
PreferencesController: {
39-
showFiatInTestnets: true,
17+
{
18+
engine: {
19+
backgroundState: {
20+
CurrencyRateController: {
21+
currentCurrency: 'USD',
22+
},
23+
NetworkController: {
24+
...mockNetworkState({
25+
chainId: CHAIN_IDS.SEPOLIA,
26+
id: 'sepolia',
27+
nickname: 'Sepolia',
28+
ticker: 'ETH',
29+
}),
30+
},
4031
},
4132
},
33+
settings: {
34+
showFiatOnTestnets: true,
35+
},
4236
},
43-
});
37+
);
4438

45-
const mockStateWithHidingFiatOnTestnets = merge({}, mockStateWithTestnet, {
46-
engine: {
47-
backgroundState: {
48-
PreferencesController: {
49-
showFiatInTestnets: false,
50-
},
51-
},
39+
const mockStateWithHideFiatOnTestnets = merge({}, mockStateWithTestnet, {
40+
settings: {
41+
showFiatOnTestnets: false,
5242
},
5343
});
5444

5545
describe('FiatDisplay', () => {
56-
const mockUseFiatFormatter = jest.mocked(useFiatFormatter);
57-
58-
beforeEach(() => {
59-
jest.resetAllMocks();
60-
mockUseFiatFormatter.mockReturnValue((value: number) => `$${value}`);
61-
});
62-
6346
describe('IndividualFiatDisplay', () => {
6447
it.each([
6548
[FIAT_UNAVAILABLE, 'Not Available'],
66-
[100, '$100'],
67-
[-100, '$100'],
68-
])('when fiatAmount is %s it renders %s', (fiatAmount, expected) => {
69-
const { queryByText } = renderWithProvider(
70-
<IndividualFiatDisplay fiatAmount={fiatAmount} />,
71-
{ state: mockStateWithShowingFiatOnTestnets },
49+
[new BigNumber(100), '$100'],
50+
[new BigNumber(-100), '$100'],
51+
[new BigNumber(-100.5), '$100.50'],
52+
[new BigNumber('987543219876543219876.54321'), '$987,543,219,87...'],
53+
])('when fiatAmount is %s it renders %s', async (fiatAmount, expected) => {
54+
const { findByText } = renderWithProvider(
55+
<IndividualFiatDisplay fiatAmount={fiatAmount as BigNumber} />,
56+
{ state: mockStateWithTestnet },
7257
);
73-
expect(queryByText(expected)).toBeDefined();
58+
expect(await findByText(expected)).toBeTruthy();
7459
});
7560

7661
it('does not render anything if hideFiatForTestnet is true', () => {
7762
const { queryByText } = renderWithProvider(
7863
<IndividualFiatDisplay fiatAmount={100} />,
79-
{ state: mockStateWithHidingFiatOnTestnets },
64+
{ state: mockStateWithHideFiatOnTestnets },
8065
);
8166
expect(queryByText('100')).toBe(null);
8267
});
@@ -86,20 +71,55 @@ describe('FiatDisplay', () => {
8671
it.each([
8772
[[FIAT_UNAVAILABLE, FIAT_UNAVAILABLE], 'Not Available'],
8873
[[], 'Not Available'],
89-
[[100, 200, FIAT_UNAVAILABLE, 300], 'Total = $600'],
90-
[[-100, -200, FIAT_UNAVAILABLE, -300], 'Total = $600'],
91-
])('when fiatAmounts is %s it renders %s', (fiatAmounts, expected) => {
92-
const { queryByText } = renderWithProvider(
93-
<TotalFiatDisplay fiatAmounts={fiatAmounts} />,
94-
{ state: mockStateWithShowingFiatOnTestnets },
95-
);
96-
expect(queryByText(expected)).toBeDefined();
97-
});
74+
[
75+
[
76+
new BigNumber(100),
77+
new BigNumber(200),
78+
FIAT_UNAVAILABLE,
79+
new BigNumber(300),
80+
],
81+
'Total = $600',
82+
],
83+
[
84+
[
85+
new BigNumber(-100),
86+
new BigNumber(-200),
87+
FIAT_UNAVAILABLE,
88+
new BigNumber(-300.2),
89+
],
90+
'Total = $600.20',
91+
],
92+
[
93+
[
94+
new BigNumber(new BigNumber('987543219876543219876.54321')),
95+
new BigNumber(-200),
96+
FIAT_UNAVAILABLE,
97+
new BigNumber(-300.2),
98+
],
99+
'Total = $987,543,219,876,543,219,376.34',
100+
],
101+
])(
102+
'when fiatAmounts is %s it renders %s',
103+
async (fiatAmounts, expected) => {
104+
const { findByText } = renderWithProvider(
105+
<TotalFiatDisplay fiatAmounts={fiatAmounts as FiatAmount[]} />,
106+
{ state: mockStateWithTestnet },
107+
);
108+
109+
expect(await findByText(expected)).toBeTruthy();
110+
},
111+
);
98112

99113
it('does not render anything if hideFiatForTestnet is true', () => {
114+
const mockFiatAmounts = [
115+
new BigNumber(100),
116+
new BigNumber(200),
117+
new BigNumber(300),
118+
] as unknown as FiatAmount[];
119+
100120
const { queryByText } = renderWithProvider(
101-
<TotalFiatDisplay fiatAmounts={[100, 200, 300]} />,
102-
{ state: mockStateWithHidingFiatOnTestnets },
121+
<TotalFiatDisplay fiatAmounts={mockFiatAmounts} />,
122+
{ state: mockStateWithHideFiatOnTestnets },
103123
);
104124
expect(queryByText('600')).toBe(null);
105125
});

app/components/UI/SimulationDetails/FiatDisplay/FiatDisplay.tsx

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/* eslint-disable react/prop-types */
22
import React from 'react';
33
import { StyleSheet, ViewProps } from 'react-native';
4+
import { BigNumber } from 'bignumber.js';
45
import { useStyles } from '../../../hooks/useStyles';
56
import Text, {
67
TextColor,
@@ -10,6 +11,7 @@ import { strings } from '../../../../../locales/i18n';
1011
import useFiatFormatter from './useFiatFormatter';
1112
import { FIAT_UNAVAILABLE, FiatAmount } from '../types';
1213
import useHideFiatForTestnet from '../../../hooks/useHideFiatForTestnet';
14+
import { shortenString } from '../../../../util/notifications';
1315

1416
const styleSheet = () =>
1517
StyleSheet.create({
@@ -33,10 +35,10 @@ const FiatNotAvailableDisplay: React.FC = () => {
3335
);
3436
};
3537

36-
export function calculateTotalFiat(fiatAmounts: FiatAmount[]): number {
38+
export function calculateTotalFiat(fiatAmounts: FiatAmount[]): BigNumber {
3739
return fiatAmounts.reduce(
38-
(total: number, fiat) => total + (fiat === FIAT_UNAVAILABLE ? 0 : fiat),
39-
0,
40+
(total: BigNumber, fiat) => total.plus(fiat === FIAT_UNAVAILABLE ? new BigNumber(0) : new BigNumber(fiat)),
41+
new BigNumber(0)
4042
);
4143
}
4244

@@ -48,11 +50,13 @@ export function calculateTotalFiat(fiatAmounts: FiatAmount[]): number {
4850
*/
4951

5052
interface IndividualFiatDisplayProps extends ViewProps {
51-
fiatAmount: FiatAmount;
53+
fiatAmount: BigNumber | FiatAmount;
54+
shorten?: boolean;
5255
}
5356

5457
export const IndividualFiatDisplay: React.FC<IndividualFiatDisplayProps> = ({
5558
fiatAmount,
59+
shorten = true,
5660
}) => {
5761
const hideFiatForTestnet = useHideFiatForTestnet();
5862
const { styles } = useStyles(styleSheet, {});
@@ -65,11 +69,20 @@ export const IndividualFiatDisplay: React.FC<IndividualFiatDisplayProps> = ({
6569
if (fiatAmount === FIAT_UNAVAILABLE) {
6670
return <FiatNotAvailableDisplay />;
6771
}
68-
const absFiat = Math.abs(fiatAmount);
72+
const absFiat = new BigNumber(fiatAmount).abs();
73+
74+
const absFiatFormatted = shorten
75+
? shortenString(fiatFormatter(absFiat), {
76+
truncatedCharLimit: 15,
77+
truncatedStartChars: 15,
78+
truncatedEndChars: 0,
79+
skipCharacterInEnd: true,
80+
})
81+
: fiatFormatter(absFiat);
6982

7083
return (
7184
<Text {...sharedTextProps} style={styles.base}>
72-
{fiatFormatter(absFiat)}
85+
{absFiatFormatted}
7386
</Text>
7487
);
7588
};
@@ -92,12 +105,12 @@ export const TotalFiatDisplay: React.FC<{
92105
return null;
93106
}
94107

95-
return totalFiat === 0 ? (
108+
return totalFiat.eq(0) ? (
96109
<FiatNotAvailableDisplay />
97110
) : (
98111
<Text {...sharedTextProps} style={styles.base}>
99112
{strings('simulation_details.total_fiat', {
100-
currency: fiatFormatter(Math.abs(totalFiat)),
113+
currency: fiatFormatter(totalFiat.abs()),
101114
})}
102115
</Text>
103116
);

app/components/UI/SimulationDetails/FiatDisplay/useFiatFormatter.test.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { renderHook } from '@testing-library/react-hooks';
22
import I18n from '../../../../../locales/i18n';
33
import { selectCurrentCurrency } from '../../../../selectors/currencyRateController';
44
import useFiatFormatter from './useFiatFormatter';
5+
import { BigNumber } from 'bignumber.js';
56

67
jest.mock('react-redux', () => ({
78
useSelector: jest.fn((selector) => selector()),
@@ -33,9 +34,10 @@ describe('useFiatFormatter', () => {
3334
const { result } = renderHook(() => useFiatFormatter());
3435
const formatFiat = result.current;
3536

36-
expect(formatFiat(1000)).toBe('$1,000.00');
37-
expect(formatFiat(500.5)).toBe('$500.50');
38-
expect(formatFiat(0)).toBe('$0.00');
37+
expect(formatFiat(new BigNumber('987543219876543219876.54321'))).toBe('$987,543,219,876,543,219,876.54');
38+
expect(formatFiat(new BigNumber(1000))).toBe('$1,000');
39+
expect(formatFiat(new BigNumber(500.5))).toBe('$500.50');
40+
expect(formatFiat(new BigNumber(0))).toBe('$0');
3941
});
4042

4143
it('should use the current locale and currency from the mocked functions', () => {
@@ -56,8 +58,9 @@ describe('useFiatFormatter', () => {
5658
const formatFiat = result.current;
5759

5860
// Testing the fallback formatting for an unknown currency
59-
expect(formatFiat(1000)).toBe('1000 storj');
60-
expect(formatFiat(500.5)).toBe('500.5 storj');
61-
expect(formatFiat(0)).toBe('0 storj');
61+
expect(formatFiat(new BigNumber('98754321987654321987654321'))).toBe('98754321987654321987654321 storj');
62+
expect(formatFiat(new BigNumber(1000))).toBe('1000 storj');
63+
expect(formatFiat(new BigNumber(500.5))).toBe('500.5 storj');
64+
expect(formatFiat(new BigNumber(0))).toBe('0 storj');
6265
});
6366
});

app/components/UI/SimulationDetails/FiatDisplay/useFiatFormatter.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import { useSelector } from 'react-redux';
22
import { selectCurrentCurrency } from '../../../../selectors/currencyRateController';
33
import I18n from '../../../../../locales/i18n';
4+
import { BigNumber } from 'bignumber.js';
45

5-
type FiatFormatter = (fiatAmount: number) => string;
6+
type FiatFormatter = (fiatAmount: BigNumber) => string;
67

78
/**
89
* Returns a function that formats a fiat amount as a localized string.
@@ -11,23 +12,29 @@ type FiatFormatter = (fiatAmount: number) => string;
1112
*
1213
* ```
1314
* const formatFiat = useFiatFormatter();
14-
* const formattedAmount = formatFiat(1000);
15+
* const formattedAmount = formatFiat(new BigNumber(1000));
1516
* ```
1617
*
1718
* @returns A function that takes a fiat amount as a number and returns a formatted string.
1819
*/
1920
const useFiatFormatter = (): FiatFormatter => {
2021
const fiatCurrency = useSelector(selectCurrentCurrency);
2122

22-
return (fiatAmount: number) => {
23+
return (fiatAmount: BigNumber) => {
24+
const hasDecimals = !fiatAmount.isInteger();
25+
2326
try {
2427
return new Intl.NumberFormat(I18n.locale, {
2528
style: 'currency',
2629
currency: fiatCurrency,
27-
}).format(fiatAmount);
30+
minimumFractionDigits: hasDecimals ? 2 : 0,
31+
// string is valid parameter for format function
32+
// for some reason it gives TS issue
33+
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/format#number
34+
}).format(fiatAmount.toFixed() as unknown as number);
2835
} catch (error) {
2936
// Fallback for unknown or unsupported currencies
30-
return `${fiatAmount} ${fiatCurrency}`;
37+
return `${fiatAmount.toFixed()} ${fiatCurrency}`;
3138
}
3239
};
3340
};

app/components/UI/SimulationDetails/types.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ export enum AssetType {
1212
* Describes an amount of fiat.
1313
*/
1414
export const FIAT_UNAVAILABLE = null;
15-
export type FiatAmountAvailable = number;
15+
export type FiatAmountAvailable = number | string;
1616
export type FiatAmount = FiatAmountAvailable | typeof FIAT_UNAVAILABLE;
1717

1818
/**

app/components/UI/SimulationDetails/useSimulationMetrics.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -330,7 +330,7 @@ describe('useSimulationMetrics', () => {
330330
const balanceChange = {
331331
...BALANCE_CHANGE_MOCK,
332332
amount: new BigNumber(isNegative ? -1 : 1),
333-
fiatAmount,
333+
fiatAmount: fiatAmount as number,
334334
};
335335

336336
expectUpdateTransactionMetricsCalled(

app/components/UI/SimulationDetails/useSimulationMetrics.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ function getProperties(changes: BalanceChange[], prefix: string) {
172172
function getSensitiveProperties(changes: BalanceChange[], prefix: string) {
173173
const fiatAmounts = changes.map((change) => change.fiatAmount);
174174
const totalFiat = calculateTotalFiat(fiatAmounts);
175-
const totalValue = totalFiat ? Math.abs(totalFiat) : undefined;
175+
const totalValue = totalFiat ? totalFiat.abs().toNumber() : undefined;
176176

177177
return getPrefixProperties({ total_value: totalValue }, prefix);
178178
}

0 commit comments

Comments
 (0)