Skip to content

Commit

Permalink
chore(runway): cherry-pick fix: add nativeAsset key to staked ETH ass…
Browse files Browse the repository at this point in the history 
…et multichain (#12851)

## **Description**

1. What is the reason for the change?

There was a crash when clicking on the Staked Ethereum asset due to it
missing a key `nativeAsset` which is required in some component. This
was a regression from code refactoring.

Separately, there was a currency switching bug that was introduced with
the use of a different fiat conversion method. I have switched to a
method that does not throw error for currency switching and this has
been tested with Vince

Separately, there were some issues with the Token list asset balances
being slightly off. This was noticed when the balance we use for the
Asset Detail staked Ethereum asset showed up differently than in the
Token List. Some of this has been addressed, but there are still issues
that I've noticed where we lose precision when getting the fiat total of
a token asset or native asset. I left a comment in the code about this,
and maybe it could be revisited it later.

2. What is the improvement/solution?

We added the `nativeAsset` key back to the staked ethereum asset

We added the weiToFiat method to replace the Intl.NumberFormat method 

We removed precision loss from multichain.ts asset fiatBalance
calculations, however there is another place(s) where fiat balances are
recalculated again i.e.
[deriveBalanceFromAssetMarketDetails.ts](https://github.com/MetaMask/metamask-mobile/pull/12851/files#diff-283c109c8093bbb5d8e06be79bcbeac928b8353633e55dc7944594a0e6888b49)

## **Related issues**

Fixes: #12680
Fixes: #12856
Closes: https://consensyssoftware.atlassian.net/browse/STAKE-912

## **Manual testing steps**

- Import the staking test wallet in MM
- Click on the staked ethereum asset
- The link should not crash the application more info here on testing
this #12680
- Currency switching should not crash the application more info here on
testing this #12856

## **Screenshots/Recordings**

### **Before**

### **After**


https://github.com/user-attachments/assets/c2525b70-8d04-4606-861b-3206ebd557f4


https://github.com/user-attachments/assets/a8c4010e-3589-4171-803a-4d07a962e6a6

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
  • Loading branch information
@nickewansmith
nickewansmith committed Jan 10, 2025
1 parent cfdb096 commit 002e75a
Show file tree
Hide file tree
Showing 7 changed files with 143 additions and 68 deletions.
2 changes: 1 addition & 1 deletion app/components/UI/AssetOverview/AssetOverview.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ const AssetOverview: React.FC<AssetOverviewProps> = ({
}
} else {
mainBalance = `${balance} ${asset.isETH ? asset.ticker : asset.symbol}`;
secondaryBalance = exchangeRate ? asset.balanceFiat : '';
secondaryBalance = asset.balanceFiat || '';
}

let currentPrice = 0;
Expand Down
16 changes: 16 additions & 0 deletions app/components/UI/AssetOverview/__snapshots__/AssetOverview.test.tsx.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2237,6 +2237,22 @@ exports[`AssetOverview should render correctly when portfolio view is enabled 1`
}
}
>
<Text
accessibilityRole="text"
style={
{
"color": "#141618",
"fontFamily": "EuclidCircularB-Medium",
"fontSize": 16,
"fontWeight": "500",
"letterSpacing": 0,
"lineHeight": 24,
}
}
testID="fiat-balance-test-id"
>
1500
</Text>
<Text
accessibilityRole="text"
style={
Expand Down
6 changes: 3 additions & 3 deletions app/components/UI/Stake/hooks/useBalance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { useSelector } from 'react-redux';
import { selectSelectedInternalAccountFormattedAddress } from '../../../../selectors/accountsController';
import { selectAccountsByChainId } from '../../../../selectors/accountTrackerController';
import {
selectConversionRate,
selectCurrencyRates,
selectCurrentCurrency,
} from '../../../../selectors/currencyRateController';
import { selectChainId } from '../../../../selectors/networkController';
Expand All @@ -20,9 +20,9 @@ const useBalance = () => {
const selectedAddress = useSelector(
selectSelectedInternalAccountFormattedAddress,
);
const conversionRate = useSelector(selectConversionRate) ?? 1;
const currentCurrency = useSelector(selectCurrentCurrency);

const currencyRates = useSelector(selectCurrencyRates);
const conversionRate = currencyRates?.ETH?.conversionRate ?? 1;
const rawAccountBalance = selectedAddress
? accountsByChainId[chainId]?.[selectedAddress]?.balance
: '0';
Expand Down
30 changes: 15 additions & 15 deletions app/components/UI/Tokens/__snapshots__/index.test.tsx.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,7 @@ exports[`Tokens render matches snapshot 1`] = `
"iconUrl": "",
"name": "Bat",
"symbol": "BAT",
"tokenFiatAmount": NaN,
"tokenFiatAmount": 0,
},
]
}
Expand All @@ -991,7 +991,7 @@ exports[`Tokens render matches snapshot 1`] = `
"iconUrl": "",
"name": "Bat",
"symbol": "BAT",
"tokenFiatAmount": NaN,
"tokenFiatAmount": 0,
},
]
}
Expand Down Expand Up @@ -1667,7 +1667,7 @@ exports[`Tokens render matches snapshot 1`] = `
}
testID="main-balance-test-id"
>
&lt; $0.01
$0
</Text>
</View>
</TouchableOpacity>
Expand Down Expand Up @@ -2220,7 +2220,7 @@ exports[`Tokens should hide zero balance tokens when setting is on 1`] = `
"iconUrl": "",
"name": "Bat",
"symbol": "BAT",
"tokenFiatAmount": NaN,
"tokenFiatAmount": 0,
},
]
}
Expand All @@ -2246,7 +2246,7 @@ exports[`Tokens should hide zero balance tokens when setting is on 1`] = `
"iconUrl": "",
"name": "Bat",
"symbol": "BAT",
"tokenFiatAmount": NaN,
"tokenFiatAmount": 0,
},
]
}
Expand Down Expand Up @@ -2922,7 +2922,7 @@ exports[`Tokens should hide zero balance tokens when setting is on 1`] = `
}
testID="main-balance-test-id"
>
&lt; $0.01
$0
</Text>
</View>
</TouchableOpacity>
Expand Down Expand Up @@ -3475,7 +3475,7 @@ exports[`Tokens should render correctly 1`] = `
"iconUrl": "",
"name": "Bat",
"symbol": "BAT",
"tokenFiatAmount": NaN,
"tokenFiatAmount": 0,
},
]
}
Expand All @@ -3501,7 +3501,7 @@ exports[`Tokens should render correctly 1`] = `
"iconUrl": "",
"name": "Bat",
"symbol": "BAT",
"tokenFiatAmount": NaN,
"tokenFiatAmount": 0,
},
]
}
Expand Down Expand Up @@ -4177,7 +4177,7 @@ exports[`Tokens should render correctly 1`] = `
}
testID="main-balance-test-id"
>
&lt; $0.01
$0
</Text>
</View>
</TouchableOpacity>
Expand Down Expand Up @@ -4730,7 +4730,7 @@ exports[`Tokens should show all balance tokens when hideZeroBalanceTokens settin
"iconUrl": "",
"name": "Bat",
"symbol": "BAT",
"tokenFiatAmount": NaN,
"tokenFiatAmount": 0,
},
{
"address": "0x02",
Expand All @@ -4739,7 +4739,7 @@ exports[`Tokens should show all balance tokens when hideZeroBalanceTokens settin
"iconUrl": "",
"name": "Link",
"symbol": "LINK",
"tokenFiatAmount": NaN,
"tokenFiatAmount": 0,
},
]
}
Expand All @@ -4765,7 +4765,7 @@ exports[`Tokens should show all balance tokens when hideZeroBalanceTokens settin
"iconUrl": "",
"name": "Bat",
"symbol": "BAT",
"tokenFiatAmount": NaN,
"tokenFiatAmount": 0,
},
{
"address": "0x02",
Expand All @@ -4774,7 +4774,7 @@ exports[`Tokens should show all balance tokens when hideZeroBalanceTokens settin
"iconUrl": "",
"name": "Link",
"symbol": "LINK",
"tokenFiatAmount": NaN,
"tokenFiatAmount": 0,
},
]
}
Expand Down Expand Up @@ -5450,7 +5450,7 @@ exports[`Tokens should show all balance tokens when hideZeroBalanceTokens settin
}
testID="main-balance-test-id"
>
&lt; $0.01
$0
</Text>
</View>
</TouchableOpacity>
Expand Down Expand Up @@ -5716,7 +5716,7 @@ exports[`Tokens should show all balance tokens when hideZeroBalanceTokens settin
}
testID="main-balance-test-id"
>
&lt; $0.01
$0
</Text>
</View>
</TouchableOpacity>
Expand Down
37 changes: 36 additions & 1 deletion app/components/UI/Tokens/util/deriveBalanceFromAssetMarketDetails.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ describe('deriveBalanceFromAssetMarketDetails', () => {
});
});

it('returns "< 0.01 USD" if fiat balance is less than 0.01', () => {
it('returns "< 0.01 USD" if token asset fiat balance is less than 0.01', () => {
(renderFromTokenMinimalUnit as jest.Mock).mockReturnValue('2');
(balanceToFiatNumber as jest.Mock).mockReturnValue(0.005);
(addCurrencySymbol as jest.Mock).mockReturnValue('$0.01');
Expand All @@ -156,4 +156,39 @@ describe('deriveBalanceFromAssetMarketDetails', () => {
});
expect(addCurrencySymbol).toHaveBeenCalledWith('0.01', 'USD');
});

it('returns "< 0.01 USD" if ETH asset fiat balance is less than 0.01', () => {
(renderFromTokenMinimalUnit as jest.Mock).mockReturnValue('2');
(balanceToFiatNumber as jest.Mock).mockReturnValue(0.005);
(addCurrencySymbol as jest.Mock).mockReturnValue('$0.01');

const ethAsset: TokenI = {
address: '0x0',
symbol: 'ETH',
balance: '< .00001',
decimals: 18,
isETH: true,
balanceFiat: '< $0.01',
aggregators: [],
hasBalanceError: false,
image: '',
name: '',
logo: undefined,
};

const result = deriveBalanceFromAssetMarketDetails(
ethAsset,
tokenExchangeRates,
tokenBalances,
conversionRate,
currentCurrency,
);

expect(result).toEqual({
balanceFiat: '< $0.01',
balanceFiatCalculation: NaN,
balanceValueFormatted: '< .00001 ETH',
});
expect(addCurrencySymbol).toHaveBeenCalledWith('0.01', 'USD');
});
});
19 changes: 18 additions & 1 deletion app/components/UI/Tokens/util/deriveBalanceFromAssetMarketDetails.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,22 @@ export const deriveBalanceFromAssetMarketDetails = (
: TOKEN_RATE_UNDEFINED,
balanceValueFormatted,
};

const balanceFiatWithoutFormatting = asset?.balanceFiat?.replace(
/[^0-9.]/g,
'',
);
const balanceFiatBelowMinimialUnit = asset?.balanceFiat?.includes('<');
// TODO: if balanceFiat is below minimial unit, use the balanceFiat value
// balanceFiatCalculation will be NaN and balanceFiat will be < 0.01
// This is not ideal, but it's a workaround for now as the places we use
// balanceFiatCalculation are for tokens, which do not have a balanceFiat value
// so there is no adverse effect at the moment
const assetBalanceFiat = balanceFiatBelowMinimialUnit
? asset.balanceFiat
: balanceFiatWithoutFormatting;
const balanceFiatCalculation = Number(
asset.balanceFiat ||
assetBalanceFiat ||
balanceToFiatNumber(balance, conversionRate, tokenMarketData.price),
);

Expand All @@ -71,5 +85,8 @@ export const deriveBalanceFromAssetMarketDetails = (
? addCurrencySymbol(balanceFiatCalculation, currentCurrency)
: `< ${addCurrencySymbol('0.01', currentCurrency)}`;

// NOTE: the way we calculate the balances here loses precision for both tokens and native assets
// This is different than how portfolio fiat total is calculated and causes variance between the totals

return { balanceFiat, balanceValueFormatted, balanceFiatCalculation };
};
Loading

0 comments on commit 002e75a

Please sign in to comment.