Skip to content

Commit

Permalink
fix: swaps quote nan to bnjs (#9848)
Browse files Browse the repository at this point in the history
<!--
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**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

This PR fixes an issue where the app crashes after fetching a Swaps
quote. This is caused by `renderFromWei(missingEthBalance)` when
`missingEthBalance` is `"NaN"`.

The root issue is that
`accountTrackerControllerState.accounts[addr].balance` flip flops
between `undefined` and a string hex value. Swaps checks the gas token
balance to make sure the user has enough to submit the Swap transaction.
This PR does not solve that issue, it only adds guards to prevent the
app from crashing.

## **Related issues**

Fixes: #9672

## **Manual testing steps**

1. Go to Swaps
2. Fetch a quote
3. Sit on quote view screen and wait. This might take some time. If you
throttle your network, it seems to happen more often.
4. It should not crash the app

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->
n/a

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask 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
infiniteflower authored Jun 5, 2024
1 parent cb39b6a commit d8987c8
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 10 deletions.
11 changes: 10 additions & 1 deletion app/components/UI/Swaps/QuotesView.js
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,9 @@ function SwapsQuotesView({
const ethAmountBN = isSwapsNativeAsset(sourceToken)
? sourceBN
: new BigNumber(0);

// accounts[selectedAddress].balance might be undefined
// BigNumber(undefined) returns "NaN", which causes missingEthBalance to be "NaN" downstream
const ethBalanceBN = new BigNumber(accounts[selectedAddress].balance);
const gasBN = toWei(selectedQuoteValue?.maxEthFee || '0');
const hasEnoughEthBalance = ethBalanceBN.gte(ethAmountBN.plus(gasBN));
Expand Down Expand Up @@ -1674,6 +1677,12 @@ function SwapsQuotesView({
hasEnoughTokenBalance &&
hasEnoughEthBalance;

const shouldShowNeedMoreAlert =
(!isSwapsNativeAsset(sourceToken) && !hasEnoughTokenBalance) ||
(isSwapsNativeAsset(sourceToken) &&
!hasEnoughEthBalance &&
missingEthBalance?.isNaN() === false);

return (
<ScreenView
contentContainerStyle={styles.screen}
Expand All @@ -1682,7 +1691,7 @@ function SwapsQuotesView({
scrollEnabled={!isSwiping}
>
<View style={styles.topBar}>
{(!hasEnoughTokenBalance || !hasEnoughEthBalance) && (
{shouldShowNeedMoreAlert && (
<View style={styles.alertBar}>
<Alert small type={AlertType.Info}>
{`${strings('swaps.you_need')} `}
Expand Down
23 changes: 14 additions & 9 deletions app/util/number/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -312,17 +312,22 @@ export function fiatNumberToTokenMinimalUnit(
export function renderFromWei(value, decimalsToShow = 5) {
let renderWei = '0';
// avoid undefined
if (value) {
const wei = fromWei(value);
const weiNumber = parseFloat(wei);
if (weiNumber < 0.00001 && weiNumber > 0) {
renderWei = '< 0.00001';
} else {
const base = Math.pow(10, decimalsToShow);
renderWei = (Math.round(weiNumber * base) / base).toString();
// This function can throw an error if value is "NaN", so wrap in try/catch
try {
if (value) {
const wei = fromWei(value);
const weiNumber = parseFloat(wei);
if (weiNumber < 0.00001 && weiNumber > 0) {
renderWei = '< 0.00001';
} else {
const base = Math.pow(10, decimalsToShow);
renderWei = (Math.round(weiNumber * base) / base).toString();
}
}
return renderWei;
} catch (e) {
return renderWei;
}
return renderWei;
}

/**
Expand Down
4 changes: 4 additions & 0 deletions app/util/number/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,10 @@ describe('Number utils :: renderFromWei', () => {
expect(renderFromWei('133700000000000000')).toEqual('0.1337');
expect(renderFromWei('1337')).toEqual('< 0.00001');
expect(renderFromWei('0')).toEqual('0');

// https://github.com/MetaMask/metamask-mobile/issues/9672
// fromWei will throw error if "NaN" is passed
expect(renderFromWei('NaN')).toEqual('0');
});

it('renderFromWei using BN number', () => {
Expand Down

0 comments on commit d8987c8

Please sign in to comment.