Skip to content

Commit

Permalink
chore: refactor and unify low return warning (#29918)
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**

Users are experiencing issues with receiving significantly lower amounts
of destination tokens than expected during swaps. This is due to
excessive price differentials between the estimated swap amount and the
actual return. Currently, the logic for triggering a low return warning
differs between swaps and bridges, leading to inconsistencies and user
confusion.

This PR brings swap to parity with bridge by adding a warning when a
swap results in a return below 80% of the value of the source tokens.
<!--
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?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29918?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to swaps
2. Select tokens and input amount to force warning (eg. 0.01 ETH to ENA)
3. See warning

## **Screenshots/Recordings**

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

### **Before**
![Screenshot 2025-01-27 at 19 55
48](https://github.com/user-attachments/assets/828a13ac-10bc-4624-b538-ca1e05e36fff)

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

### **After**
![Screenshot 2025-01-27 at 19 55
18](https://github.com/user-attachments/assets/5109052f-25c0-4e32-bc4a-4ab5a7e9d526)

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

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/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
- [x] 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-extension/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
bfullam authored Jan 30, 2025
1 parent 0b202e3 commit 5f76d25
Show file tree
Hide file tree
Showing 11 changed files with 173 additions and 19 deletions.
2 changes: 1 addition & 1 deletion shared/constants/bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const ETH_USDT_ADDRESS = '0xdac17f958d2ee523a2206206994597c13d831ec7';
export const METABRIDGE_ETHEREUM_ADDRESS =
'0x0439e60F02a8900a951603950d8D4527f400C3f1';
export const BRIDGE_QUOTE_MAX_ETA_SECONDS = 60 * 60; // 1 hour
export const BRIDGE_QUOTE_MAX_RETURN_DIFFERENCE_PERCENTAGE = 0.5; // if a quote returns in x times less return than the best quote, ignore it
export const BRIDGE_QUOTE_MAX_RETURN_DIFFERENCE_PERCENTAGE = 0.35; // if a quote returns in x times less return than the best quote, ignore it

export const BRIDGE_PREFERRED_GAS_ESTIMATE = 'high';
export const BRIDGE_DEFAULT_SLIPPAGE = 0.5;
Expand Down
1 change: 1 addition & 0 deletions shared/constants/swaps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const SLIPPAGE_LOW_ERROR = 'slippage-low';
export const SLIPPAGE_NEGATIVE_ERROR = 'slippage-negative';

export const MAX_ALLOWED_SLIPPAGE = 15;
export const SWAPS_QUOTE_MAX_RETURN_DIFFERENCE_PERCENTAGE = 0.35;

// An address that the metaswap-api recognizes as the default token for the current network,
// in place of the token address that ERC-20 tokens have
Expand Down
8 changes: 4 additions & 4 deletions ui/ducks/bridge/selectors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1183,7 +1183,7 @@ describe('Bridge selectors', () => {
).toStrictEqual(false);
});

it('should return isEstimatedReturnLow=true return value is 50% less than sent funds', () => {
it('should return isEstimatedReturnLow=true return value is less than 65% of sent funds', () => {
const state = createBridgeMockStore({
featureFlagOverrides: {
extensionConfig: {
Expand Down Expand Up @@ -1239,7 +1239,7 @@ describe('Bridge selectors', () => {
expect(result.isEstimatedReturnLow).toStrictEqual(true);
});

it('should return isEstimatedReturnLow=false when return value is more than 50% of sent funds', () => {
it('should return isEstimatedReturnLow=false when return value is more than 65% of sent funds', () => {
const state = createBridgeMockStore({
featureFlagOverrides: {
extensionConfig: {
Expand All @@ -1254,7 +1254,7 @@ describe('Bridge selectors', () => {
fromToken: { address: zeroAddress(), symbol: 'ETH' },
toToken: { address: zeroAddress(), symbol: 'TEST' },
fromTokenExchangeRate: 2524.25,
toTokenExchangeRate: 0.63,
toTokenExchangeRate: 0.95,
fromTokenInputValue: 1,
},
bridgeStateOverrides: {
Expand Down Expand Up @@ -1292,7 +1292,7 @@ describe('Bridge selectors', () => {
expect(
getBridgeQuotes(state as never).activeQuote?.adjustedReturn
.valueInCurrency,
).toStrictEqual(new BigNumber('12.87194306627291988'));
).toStrictEqual(new BigNumber('20.69239170627291988'));
expect(result.isEstimatedReturnLow).toStrictEqual(false);
});

Expand Down
2 changes: 1 addition & 1 deletion ui/ducks/bridge/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ export const getValidationErrors = createDeepEqualSelector(
fromTokenInputValue
? activeQuote.adjustedReturn.valueInCurrency.lt(
new BigNumber(
BRIDGE_QUOTE_MAX_RETURN_DIFFERENCE_PERCENTAGE,
1 - BRIDGE_QUOTE_MAX_RETURN_DIFFERENCE_PERCENTAGE,
).times(activeQuote.sentAmount.valueInCurrency),
)
: false,
Expand Down
54 changes: 54 additions & 0 deletions ui/ducks/swaps/swaps.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ import {
SWAPS_FETCH_ORDER_CONFLICT,
ALLOWED_SMART_TRANSACTIONS_CHAIN_IDS,
Slippage,
SWAPS_QUOTE_MAX_RETURN_DIFFERENCE_PERCENTAGE,
} from '../../../shared/constants/swaps';
import {
IN_PROGRESS_TRANSACTION_STATUSES,
Expand All @@ -100,6 +101,7 @@ import {
import { EtherDenomination } from '../../../shared/constants/common';
import { Numeric } from '../../../shared/modules/Numeric';
import { calculateMaxGasLimit } from '../../../shared/lib/swaps-utils';
import { useTokenFiatAmount } from '../../hooks/useTokenFiatAmount';

const debugLog = createProjectLogger('swaps');

Expand Down Expand Up @@ -1435,3 +1437,55 @@ export function cancelSwapsSmartTransaction(uuid) {
}
};
}

export const getIsEstimatedReturnLow = ({ usedQuote, rawNetworkFees }) => {
const sourceTokenAmount = calcTokenAmount(
usedQuote?.sourceAmount,
usedQuote?.sourceTokenInfo?.decimals,
);
// Disabled because it's not a hook
// eslint-disable-next-line react-hooks/rules-of-hooks
const sourceTokenFiatAmount = useTokenFiatAmount(
usedQuote?.sourceTokenInfo?.address,
sourceTokenAmount || 0,
usedQuote?.sourceTokenInfo?.symbol,
{
showFiat: true,
},
true,
null,
false,
);
const destinationTokenAmount = calcTokenAmount(
usedQuote?.destinationAmount,
usedQuote?.destinationTokenInfo?.decimals,
);
// Disabled because it's not a hook
// eslint-disable-next-line react-hooks/rules-of-hooks
const destinationTokenFiatAmount = useTokenFiatAmount(
usedQuote?.destinationTokenInfo?.address,
destinationTokenAmount || 0,
usedQuote?.destinationTokenInfo?.symbol,
{
showFiat: true,
},
true,
null,
false,
);
const adjustedReturnValue =
destinationTokenFiatAmount && rawNetworkFees
? new BigNumber(destinationTokenFiatAmount).minus(
new BigNumber(rawNetworkFees),
)
: null;
const isEstimatedReturnLow =
sourceTokenFiatAmount && adjustedReturnValue
? adjustedReturnValue.lt(
new BigNumber(sourceTokenFiatAmount).times(
1 - SWAPS_QUOTE_MAX_RETURN_DIFFERENCE_PERCENTAGE,
),
)
: false;
return isEstimatedReturnLow;
};
2 changes: 1 addition & 1 deletion ui/helpers/utils/token-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ export function getTokenFiatAmount(

currentTokenInFiat = currentTokenInFiat.round(2).toString();
let result;
if (hideCurrencySymbol) {
if (hideCurrencySymbol && formatted) {
result = formatCurrency(currentTokenInFiat, currentCurrency);
} else if (formatted) {
result = `${formatCurrency(
Expand Down
4 changes: 3 additions & 1 deletion ui/hooks/useTokenFiatAmount.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { isEqualCaseInsensitive } from '../../shared/modules/string-utils';
* @param {boolean} hideCurrencySymbol - Indicates whether the returned formatted amount should include the trailing currency symbol
* @returns {string} The formatted token amount in the user's chosen fiat currency
* @param {string} [chainId] - The chain id
* @param {boolean} formatted - Whether the return value should be formatted or not
*/
export function useTokenFiatAmount(
tokenAddress,
Expand All @@ -36,6 +37,7 @@ export function useTokenFiatAmount(
overrides = {},
hideCurrencySymbol,
chainId = null,
formatted = true,
) {
const allMarketData = useSelector(getMarketData);

Expand Down Expand Up @@ -91,7 +93,7 @@ export function useTokenFiatAmount(
currentCurrency,
tokenAmount,
tokenSymbol,
true,
formatted,
hideCurrencySymbol,
),
[
Expand Down
29 changes: 28 additions & 1 deletion ui/pages/swaps/prepare-swap-page/prepare-swap-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
TextVariant,
BLOCK_SIZES,
FontWeight,
TextAlign,
} from '../../../helpers/constants/design-system';
import {
fetchQuotesAndSetQuoteState,
Expand Down Expand Up @@ -95,6 +96,7 @@ import {
QUOTES_NOT_AVAILABLE_ERROR,
QUOTES_EXPIRED_ERROR,
MAX_ALLOWED_SLIPPAGE,
SWAPS_QUOTE_MAX_RETURN_DIFFERENCE_PERCENTAGE,
} from '../../../../shared/constants/swaps';
import {
CHAINID_DEFAULT_BLOCK_EXPLORER_URL_MAP,
Expand Down Expand Up @@ -132,6 +134,7 @@ import {
Text,
TextField,
TextFieldSize,
BannerAlertSeverity,
} from '../../../components/component-library';
import { ModalContent } from '../../../components/component-library/modal-content/deprecated';
import { ModalHeader } from '../../../components/component-library/modal-header/deprecated';
Expand Down Expand Up @@ -178,6 +181,8 @@ export default function PrepareSwapPage({
const [quoteCount, updateQuoteCount] = useState(0);
const [prefetchingQuotes, setPrefetchingQuotes] = useState(false);
const [rotateSwitchTokens, setRotateSwitchTokens] = useState(false);
const [isLowReturnBannerOpen, setIsLowReturnBannerOpen] = useState(true);
const [isEstimatedReturnLow, setIsEstimatedReturnLow] = useState(false);

const isBridgeSupported = useSelector(getIsBridgeEnabled);
const isFeatureFlagLoaded = useSelector(getIsFeatureFlagLoaded);
Expand Down Expand Up @@ -358,6 +363,8 @@ export default function PrepareSwapPage({
) {
dispatch(setSwapsErrorKey(QUOTES_NOT_AVAILABLE_ERROR));
}
// Resets the banner visibility when the estimated return is low
setIsLowReturnBannerOpen(true);
};

// The below logic simulates a sequential loading of the aggregator quotes, even though we are fetching them all with a single call.
Expand Down Expand Up @@ -396,6 +403,11 @@ export default function PrepareSwapPage({
prefetchingQuotes,
]);

useEffect(() => {
// Reopens the low return banner if a new quote is selected
setIsLowReturnBannerOpen(true);
}, [usedQuote]);

const onFromSelect = (token) => {
if (
token?.address &&
Expand Down Expand Up @@ -1165,6 +1177,18 @@ export default function PrepareSwapPage({
</BannerAlert>
</Box>
)}
{isEstimatedReturnLow && isLowReturnBannerOpen && (
<BannerAlert
marginTop={3}
title={t('lowEstimatedReturnTooltipTitle')}
severity={BannerAlertSeverity.Warning}
description={t('lowEstimatedReturnTooltipMessage', [
SWAPS_QUOTE_MAX_RETURN_DIFFERENCE_PERCENTAGE * 100,
])}
textAlign={TextAlign.Left}
onClose={() => setIsLowReturnBannerOpen(false)}
/>
)}
{swapsErrorKey && (
<Box display={DISPLAY.FLEX} marginTop={2}>
<SwapsBannerAlert
Expand Down Expand Up @@ -1193,7 +1217,10 @@ export default function PrepareSwapPage({
/>
)}
{showReviewQuote && (
<ReviewQuote setReceiveToAmount={setReceiveToAmount} />
<ReviewQuote
setReceiveToAmount={setReceiveToAmount}
setIsEstimatedReturnLow={setIsEstimatedReturnLow}
/>
)}
</div>
{!areQuotesPresent && (
Expand Down
32 changes: 22 additions & 10 deletions ui/pages/swaps/prepare-swap-page/review-quote.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
fetchSwapsSmartTransactionFees,
getSmartTransactionFees,
getCurrentSmartTransactionsEnabled,
getIsEstimatedReturnLow,
} from '../../../ducks/swaps/swaps';
import { getCurrentChainId } from '../../../../shared/modules/selectors/networks';
import {
Expand Down Expand Up @@ -181,7 +182,10 @@ ViewAllQuotesLink.propTypes = {
t: PropTypes.func.isRequired,
};

export default function ReviewQuote({ setReceiveToAmount }) {
export default function ReviewQuote({
setReceiveToAmount,
setIsEstimatedReturnLow,
}) {
const history = useHistory();
const dispatch = useDispatch();
const t = useContext(I18nContext);
Expand Down Expand Up @@ -421,7 +425,7 @@ export default function ReviewQuote({ setReceiveToAmount }) {
sourceTokenValue,
} = renderableDataForUsedQuote;

let { feeInFiat, feeInEth, rawEthFee, feeInUsd } =
let { feeInFiat, feeInEth, rawEthFee, feeInUsd, rawNetworkFees } =
getRenderableNetworkFeesForQuote({
tradeGas: usedGasLimit,
approveGas,
Expand Down Expand Up @@ -472,14 +476,15 @@ export default function ReviewQuote({ setReceiveToAmount }) {
smartTransactionFees?.tradeTxFees.maxFeeEstimate +
(smartTransactionFees?.approvalTxFees?.maxFeeEstimate || 0);

({ feeInFiat, feeInEth, rawEthFee, feeInUsd } = getFeeForSmartTransaction({
chainId,
currentCurrency,
conversionRate,
USDConversionRate,
nativeCurrencySymbol,
feeInWeiDec: stxEstimatedFeeInWeiDec,
}));
({ feeInFiat, feeInEth, rawEthFee, feeInUsd, rawNetworkFees } =
getFeeForSmartTransaction({
chainId,
currentCurrency,
conversionRate,
USDConversionRate,
nativeCurrencySymbol,
feeInWeiDec: stxEstimatedFeeInWeiDec,
}));
additionalTrackingParams.stx_fee_in_usd = Number(feeInUsd);
additionalTrackingParams.stx_fee_in_eth = Number(rawEthFee);
additionalTrackingParams.estimated_gas =
Expand Down Expand Up @@ -1122,6 +1127,12 @@ export default function ReviewQuote({ setReceiveToAmount }) {
currentCurrency,
]);

const isEstimatedReturnLow = getIsEstimatedReturnLow({
usedQuote,
rawNetworkFees,
});
setIsEstimatedReturnLow(isEstimatedReturnLow);

return (
<div className="review-quote">
<div className="review-quote__content">
Expand Down Expand Up @@ -1489,4 +1500,5 @@ export default function ReviewQuote({ setReceiveToAmount }) {

ReviewQuote.propTypes = {
setReceiveToAmount: PropTypes.func.isRequired,
setIsEstimatedReturnLow: PropTypes.func.isRequired,
};
57 changes: 57 additions & 0 deletions ui/pages/swaps/prepare-swap-page/review-quote.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const middleware = [thunk];
const createProps = (customProps = {}) => {
return {
setReceiveToAmount: jest.fn(),
setIsEstimatedReturnLow: jest.fn(),
...customProps,
};
};
Expand Down Expand Up @@ -143,6 +144,62 @@ describe('ReviewQuote', () => {
expect(getByText('Swap')).toBeInTheDocument();
});

it('should call setIsEstimatedReturnLow(true) when return value is less than 65% of sent funds', async () => {
const setReceiveToAmountMock = jest.fn();
const setIsEstimatedReturnLowMock = jest.fn();
const props = {
setReceiveToAmount: setReceiveToAmountMock,
setIsEstimatedReturnLow: setIsEstimatedReturnLowMock,
};

const state = createSwapsMockStore();

// Set up market data for price calculations
state.metamask.marketData = {
[CHAIN_IDS.MAINNET]: {
'0x6B175474E89094C44Da98b954EedeAC495271d0F': {
// DAI
price: 100,
decimal: 18,
},
'0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48': {
// USDC
price: 60,
decimal: 6,
},
},
};

// Set up the quotes with amounts that will result in less than 65% return
state.metamask.swapsState.quotes = {
TEST_AGG_2: {
sourceAmount: '1000000000000000000', // 1 DAI (18 decimals)
destinationAmount: '1000000', // 1 USDC (6 decimals)
trade: {
value: '0x0',
},
sourceTokenInfo: {
symbol: 'DAI',
decimals: 18,
address: '0x6B175474E89094C44Da98b954EedeAC495271d0F',
},
destinationTokenInfo: {
symbol: 'USDC',
decimals: 6,
address: '0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48',
},
},
};

const store = configureMockStore(middleware)(state);

await act(async () => {
renderWithProvider(<ReviewQuote {...props} />, store);
});

expect(setIsEstimatedReturnLowMock).toHaveBeenCalledWith(true);
});

describe('uses gas fee estimates from transaction controller if 1559 and smart disabled', () => {
let smartDisabled1559State;

Expand Down
Loading

0 comments on commit 5f76d25

Please sign in to comment.