Alternative savings fix (#9675)

* Alternative savings fix

* Further required changes to savings fix

* Further fix to savings calculations that properly accounts for metamask fees

* metaMaskFeeInEth property on quotes to decimal string

* Fix swaps controller unit tests

* Improve documentation in swaps controller

* Prevent getMedianEthValueQuote from mutation passed quotes array with .sort() call

* Another fix and refactor to savings calculations in _findTopQuoteAndCalculateSavings

Cleaner structuring of conditionals for setting tokenValueOfQuoteForSorting, ethValueOfQuote and metaMaskFeeInEth in swaps controller

Stop subtracting medianMetaMaskFee from savings, but include it in savings data

Another fix and refactor to savings calculations in _findTopQuoteAndCalculateSavings

* Add and update unit tests for _findTopQuoteAndCalculateSavings

* Improve calculation of overallValueOfQuoteForSorting for case where ETH is the source token

* Clean up getMedianEthValueQuote code, test and comments

* Clean up _findTopQuoteAndCalculateSavings, create test input and expected results helper functions

* Update getMedianEthValueQuote to account for multiple quotes with overall values equal to the median

* Add jsdoc comment for meansOfQuotesFeesAndValue

* Fix jsdoc comment for getMedianEthValueQuote

app/scripts/controllers/swaps.js

@@ -2,7 +2,7 @@ import { ethers } from 'ethers'
 import log from 'loglevel'
 import BigNumber from 'bignumber.js'
 import ObservableStore from 'obs-store'
-import { mapValues } from 'lodash'
+import { mapValues, cloneDeep } from 'lodash'
 import abi from 'human-standard-token-abi'
 import { calcTokenAmount } from '../../../ui/app/helpers/utils/token-util'
 import { calcGasTotal } from '../../../ui/app/pages/send/send.utils'
@@ -200,15 +200,12 @@ export default class SwapsController {
     if (Object.values(newQuotes).length === 0) {
       this.setSwapsErrorKey(QUOTES_NOT_AVAILABLE_ERROR)
     } else {
-      const topQuoteData = await this._findTopQuoteAndCalculateSavings(
-        newQuotes,
-      )
-
-      if (topQuoteData.topAggId) {
-        topAggId = topQuoteData.topAggId
-        newQuotes[topAggId].isBestQuote = topQuoteData.isBest
-        newQuotes[topAggId].savings = topQuoteData.savings
-      }
+      const [
+        _topAggId,
+        quotesWithSavingsAndFeeData,
+      ] = await this._findTopQuoteAndCalculateSavings(newQuotes)
+      topAggId = _topAggId
+      newQuotes = quotesWithSavingsAndFeeData
     }
 
     // If a newer call has been made, don't update state with old information
@@ -460,15 +457,14 @@ export default class SwapsController {
       return {}
     }
 
+    const newQuotes = cloneDeep(quotes)
+
     const usedGasPrice = customGasPrice || (await this._getEthersGasPrice())
 
-    let topAggId = ''
-    let ethTradeValueOfBestQuote = null
-    let ethFeeForBestQuote = null
-    const allEthTradeValues = []
-    const allEthFees = []
+    let topAggId = null
+    let overallValueOfBestQuoteForSorting = null
 
-    Object.values(quotes).forEach((quote) => {
+    Object.values(newQuotes).forEach((quote) => {
       const {
         aggregator,
         approvalNeeded,
@@ -480,6 +476,7 @@ export default class SwapsController {
         sourceAmount,
         sourceToken,
         trade,
+        fee: metaMaskFee,
       } = quote
 
       const tradeGasLimitForCalculation = gasEstimate
@@ -529,60 +526,101 @@ export default class SwapsController {
             )
           : totalEthCost
 
+      const decimalAdjustedDestinationAmount = calcTokenAmount(
+        destinationAmount,
+        destinationTokenInfo.decimals,
+      )
+
+      const tokenPercentageOfPreFeeDestAmount = new BigNumber(100, 10)
+        .minus(metaMaskFee, 10)
+        .div(100)
+      const destinationAmountBeforeMetaMaskFee = decimalAdjustedDestinationAmount.div(
+        tokenPercentageOfPreFeeDestAmount,
+      )
+      const metaMaskFeeInTokens = destinationAmountBeforeMetaMaskFee.minus(
+        decimalAdjustedDestinationAmount,
+      )
+
       const tokenConversionRate = tokenConversionRates[destinationToken]
-      const ethValueOfTrade =
-        destinationToken === ETH_SWAPS_TOKEN_ADDRESS
-          ? calcTokenAmount(destinationAmount, 18).minus(totalEthCost, 10)
-          : new BigNumber(tokenConversionRate || 1, 10)
-              .times(
-                calcTokenAmount(
-                  destinationAmount,
-                  destinationTokenInfo.decimals,
-                ),
-                10,
-              )
-              .minus(tokenConversionRate ? totalEthCost : 0, 10)
-
-      // collect values for savings calculation
-      allEthTradeValues.push(ethValueOfTrade)
-      allEthFees.push(ethFee)
+      const conversionRateForSorting = tokenConversionRate || 1
+
+      const ethValueOfTokens = decimalAdjustedDestinationAmount.times(
+        conversionRateForSorting,
+        10,
+      )
+
+      const conversionRateForCalculations =
+        destinationToken === ETH_SWAPS_TOKEN_ADDRESS ? 1 : tokenConversionRate
+
+      const overallValueOfQuoteForSorting =
+        conversionRateForCalculations === undefined
+          ? ethValueOfTokens
+          : ethValueOfTokens.minus(ethFee, 10)
+
+      quote.ethFee = ethFee.toString(10)
+
+      if (conversionRateForCalculations !== undefined) {
+        quote.ethValueOfTokens = ethValueOfTokens.toString(10)
+        quote.overallValueOfQuote = overallValueOfQuoteForSorting.toString(10)
+        quote.metaMaskFeeInEth = metaMaskFeeInTokens
+          .times(conversionRateForCalculations)
+          .toString(10)
+      }
 
       if (
-        ethTradeValueOfBestQuote === null ||
-        ethValueOfTrade.gt(ethTradeValueOfBestQuote)
+        overallValueOfBestQuoteForSorting === null ||
+        overallValueOfQuoteForSorting.gt(overallValueOfBestQuoteForSorting)
       ) {
         topAggId = aggregator
-        ethTradeValueOfBestQuote = ethValueOfTrade
-        ethFeeForBestQuote = ethFee
+        overallValueOfBestQuoteForSorting = overallValueOfQuoteForSorting
       }
     })
 
     const isBest =
-      quotes[topAggId].destinationToken === ETH_SWAPS_TOKEN_ADDRESS ||
-      Boolean(tokenConversionRates[quotes[topAggId]?.destinationToken])
+      newQuotes[topAggId].destinationToken === ETH_SWAPS_TOKEN_ADDRESS ||
+      Boolean(tokenConversionRates[newQuotes[topAggId]?.destinationToken])
 
     let savings = null
 
     if (isBest) {
+      const bestQuote = newQuotes[topAggId]
+
       savings = {}
+
+      const {
+        ethFee: medianEthFee,
+        metaMaskFeeInEth: medianMetaMaskFee,
+        ethValueOfTokens: medianEthValueOfTokens,
+      } = getMedianEthValueQuote(Object.values(newQuotes))
+
       // Performance savings are calculated as:
-      //   valueForBestTrade - medianValueOfAllTrades
-      savings.performance = ethTradeValueOfBestQuote.minus(
-        getMedian(allEthTradeValues),
+      //   (ethValueOfTokens for the best trade) - (ethValueOfTokens for the media trade)
+      savings.performance = new BigNumber(bestQuote.ethValueOfTokens, 10).minus(
+        medianEthValueOfTokens,
         10,
       )
 
-      // Performance savings are calculated as:
-      //   medianFeeOfAllTrades - feeForBestTrade
-      savings.fee = getMedian(allEthFees).minus(ethFeeForBestQuote, 10)
+      // Fee savings are calculated as:
+      //   (fee for the median trade) - (fee for the best trade)
+      savings.fee = new BigNumber(medianEthFee).minus(bestQuote.ethFee, 10)
+
+      savings.metaMaskFee = bestQuote.metaMaskFeeInEth
 
-      // Total savings are the sum of performance and fee savings
-      savings.total = savings.performance.plus(savings.fee, 10).toString(10)
+      // Total savings are calculated as:
+      //   performance savings + fee savings - metamask fee
+      savings.total = savings.performance
+        .plus(savings.fee)
+        .minus(savings.metaMaskFee)
+        .toString(10)
       savings.performance = savings.performance.toString(10)
       savings.fee = savings.fee.toString(10)
+      savings.medianMetaMaskFee = medianMetaMaskFee
+
+      newQuotes[topAggId].isBestQuote = true
+      newQuotes[topAggId].savings = savings
     }
 
-    return { topAggId, isBest, savings }
+    return [topAggId, newQuotes]
   }
 
   async _getERC20Allowance(contractAddress, walletAddress) {
@@ -685,34 +723,123 @@ export default class SwapsController {
 }
 
 /**
- * Calculates the median of a sample of BigNumber values.
+ * Calculates the median overallValueOfQuote of a sample of quotes.
  *
- * @param {import('bignumber.js').BigNumber[]} values - A sample of BigNumber
- * values. The array will be sorted in place.
- * @returns {import('bignumber.js').BigNumber} The median of the sample.
+ * @param {Array} quotes - A sample of quote objects with overallValueOfQuote, ethFee, metaMaskFeeInEth, and ethValueOfTokens properties
+ * @returns {Object} An object with the ethValueOfTokens, ethFee, and metaMaskFeeInEth of the quote with the median overallValueOfQuote
  */
-function getMedian(values) {
-  if (!Array.isArray(values) || values.length === 0) {
+function getMedianEthValueQuote(_quotes) {
+  if (!Array.isArray(_quotes) || _quotes.length === 0) {
     throw new Error('Expected non-empty array param.')
   }
 
-  values.sort((a, b) => {
-    if (a.equals(b)) {
+  const quotes = [..._quotes]
+
+  quotes.sort((quoteA, quoteB) => {
+    const overallValueOfQuoteA = new BigNumber(quoteA.overallValueOfQuote, 10)
+    const overallValueOfQuoteB = new BigNumber(quoteB.overallValueOfQuote, 10)
+    if (overallValueOfQuoteA.equals(overallValueOfQuoteB)) {
       return 0
     }
-    return a.lessThan(b) ? -1 : 1
+    return overallValueOfQuoteA.lessThan(overallValueOfQuoteB) ? -1 : 1
   })
 
-  if (values.length % 2 === 1) {
-    // return middle value
-    return values[(values.length - 1) / 2]
+  if (quotes.length % 2 === 1) {
+    // return middle values
+    const medianOverallValue =
+      quotes[(quotes.length - 1) / 2].overallValueOfQuote
+    const quotesMatchingMedianQuoteValue = quotes.filter(
+      (quote) => medianOverallValue === quote.overallValueOfQuote,
+    )
+    return meansOfQuotesFeesAndValue(quotesMatchingMedianQuoteValue)
   }
 
   // return mean of middle two values
-  const upperIndex = values.length / 2
-  return values[upperIndex].plus(values[upperIndex - 1]).dividedBy(2)
+  const upperIndex = quotes.length / 2
+  const lowerIndex = upperIndex - 1
+
+  const overallValueAtUpperIndex = quotes[upperIndex].overallValueOfQuote
+  const overallValueAtLowerIndex = quotes[lowerIndex].overallValueOfQuote
+
+  const quotesMatchingUpperIndexValue = quotes.filter(
+    (quote) => overallValueAtUpperIndex === quote.overallValueOfQuote,
+  )
+  const quotesMatchingLowerIndexValue = quotes.filter(
+    (quote) => overallValueAtLowerIndex === quote.overallValueOfQuote,
+  )
+
+  const feesAndValueAtUpperIndex = meansOfQuotesFeesAndValue(
+    quotesMatchingUpperIndexValue,
+  )
+  const feesAndValueAtLowerIndex = meansOfQuotesFeesAndValue(
+    quotesMatchingLowerIndexValue,
+  )
+
+  return {
+    ethFee: new BigNumber(feesAndValueAtUpperIndex.ethFee, 10)
+      .plus(feesAndValueAtLowerIndex.ethFee, 10)
+      .dividedBy(2)
+      .toString(10),
+    metaMaskFeeInEth: new BigNumber(
+      feesAndValueAtUpperIndex.metaMaskFeeInEth,
+      10,
+    )
+      .plus(feesAndValueAtLowerIndex.metaMaskFeeInEth, 10)
+      .dividedBy(2)
+      .toString(10),
+    ethValueOfTokens: new BigNumber(
+      feesAndValueAtUpperIndex.ethValueOfTokens,
+      10,
+    )
+      .plus(feesAndValueAtLowerIndex.ethValueOfTokens, 10)
+      .dividedBy(2)
+      .toString(10),
+  }
+}
+
+/**
+ * Calculates the arithmetic mean for each of three properties - ethFee, metaMaskFeeInEth and ethValueOfTokens - across
+ * an array of objects containing those properties.
+ *
+ * @param {Array} quotes - A sample of quote objects with overallValueOfQuote, ethFee, metaMaskFeeInEth and
+ * ethValueOfTokens properties
+ * @returns {Object} An object with the arithmetic mean each of the ethFee, metaMaskFeeInEth and ethValueOfTokens of
+ * the passed quote objects
+ */
+function meansOfQuotesFeesAndValue(quotes) {
+  const feeAndValueSumsAsBigNumbers = quotes.reduce(
+    (feeAndValueSums, quote) => ({
+      ethFee: feeAndValueSums.ethFee.plus(quote.ethFee, 10),
+      metaMaskFeeInEth: feeAndValueSums.metaMaskFeeInEth.plus(
+        quote.metaMaskFeeInEth,
+        10,
+      ),
+      ethValueOfTokens: feeAndValueSums.ethValueOfTokens.plus(
+        quote.ethValueOfTokens,
+        10,
+      ),
+    }),
+    {
+      ethFee: new BigNumber(0, 10),
+      metaMaskFeeInEth: new BigNumber(0, 10),
+      ethValueOfTokens: new BigNumber(0, 10),
+    },
+  )
+
+  return {
+    ethFee: feeAndValueSumsAsBigNumbers.ethFee
+      .div(quotes.length, 10)
+      .toString(10),
+    metaMaskFeeInEth: feeAndValueSumsAsBigNumbers.metaMaskFeeInEth
+      .div(quotes.length, 10)
+      .toString(10),
+    ethValueOfTokens: feeAndValueSumsAsBigNumbers.ethValueOfTokens
+      .div(quotes.length, 10)
+      .toString(10),
+  }
 }
 
 export const utils = {
-  getMedian,
+  getMedianEthValueQuote,
+  meansOfQuotesFeesAndValue,
 }

test/unit/app/controllers/swaps-test.js

@@ -2,12 +2,14 @@ import assert from 'assert'
 import sinon from 'sinon'
 
 import { ethers } from 'ethers'
+import { mapValues } from 'lodash'
 import BigNumber from 'bignumber.js'
 import ObservableStore from 'obs-store'
 import {
   ROPSTEN_NETWORK_ID,
   MAINNET_NETWORK_ID,
 } from '../../../../app/scripts/controllers/network/enums'
+import { ETH_SWAPS_TOKEN_ADDRESS } from '../../../../ui/app/helpers/constants/swaps'
 import { createTestProviderTools } from '../../../stub/provider'
 import SwapsController, {
   utils,
@@ -25,6 +27,10 @@ const MOCK_FETCH_PARAMS = {
 
 const TEST_AGG_ID_1 = 'TEST_AGG_1'
 const TEST_AGG_ID_2 = 'TEST_AGG_2'
+const TEST_AGG_ID_3 = 'TEST_AGG_3'
+const TEST_AGG_ID_4 = 'TEST_AGG_4'
+const TEST_AGG_ID_5 = 'TEST_AGG_5'
+const TEST_AGG_ID_6 = 'TEST_AGG_6'
 const TEST_AGG_ID_BEST = 'TEST_AGG_BEST'
 const TEST_AGG_ID_APPROVAL = 'TEST_AGG_APPROVAL'
 
@@ -61,6 +67,7 @@ const MOCK_QUOTES_APPROVAL_REQUIRED = {
     aggType: 'AGG',
     slippage: 3,
     approvalNeeded: MOCK_APPROVAL_NEEDED,
+    fee: 1,
   },
 }
 
@@ -72,7 +79,10 @@ const MOCK_FETCH_METADATA = {
 }
 
 const MOCK_TOKEN_RATES_STORE = new ObservableStore({
-  contractExchangeRates: { '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48': 2 },
+  contractExchangeRates: {
+    '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48': 2,
+    '0x1111111111111111111111111111111111111111': 0.1,
+  },
 })
 
 const MOCK_GET_PROVIDER_CONFIG = () => ({ type: 'FAKE_NETWORK' })
@@ -350,6 +360,13 @@ describe('SwapsController', function () {
     })
 
     describe('_findTopQuoteAndCalculateSavings', function () {
+      beforeEach(function () {
+        const { swapsState } = swapsController.store.getState()
+        swapsController.store.updateState({
+          swapsState: { ...swapsState, customGasPrice: '0x174876e800' },
+        })
+      })
+
       it('returns empty object if passed undefined or empty object', async function () {
         assert.deepStrictEqual(
           await swapsController._findTopQuoteAndCalculateSavings(),
@@ -360,6 +377,261 @@ describe('SwapsController', function () {
           {},
         )
       })
+
+      it('returns the top aggId and quotes with savings and fee values if passed necessary data and an even number of quotes', async function () {
+        const [
+          topAggId,
+          resultQuotes,
+        ] = await swapsController._findTopQuoteAndCalculateSavings(
+          getTopQuoteAndSavingsMockQuotes(),
+        )
+        assert.equal(topAggId, TEST_AGG_ID_1)
+        assert.deepStrictEqual(
+          resultQuotes,
+          getTopQuoteAndSavingsBaseExpectedResults(),
+        )
+      })
+
+      it('returns the top aggId and quotes with savings and fee values if passed necessary data and an odd number of quotes', async function () {
+        const testInput = getTopQuoteAndSavingsMockQuotes()
+        delete testInput[TEST_AGG_ID_6]
+        const expectedResultQuotes = getTopQuoteAndSavingsBaseExpectedResults()
+        delete expectedResultQuotes[TEST_AGG_ID_6]
+        expectedResultQuotes[TEST_AGG_ID_1].savings = {
+          total: '0.0292',
+          performance: '0.0297',
+          fee: '0.02',
+          metaMaskFee: '0.0205',
+          medianMetaMaskFee: '0.0202',
+        }
+
+        const [
+          topAggId,
+          resultQuotes,
+        ] = await swapsController._findTopQuoteAndCalculateSavings(testInput)
+        assert.equal(topAggId, TEST_AGG_ID_1)
+        assert.deepStrictEqual(resultQuotes, expectedResultQuotes)
+      })
+
+      it('returns the top aggId, without best quote flagged, and quotes with fee values if passed necessary data but no custom convert rate exists', async function () {
+        const testInput = mapValues(
+          getTopQuoteAndSavingsMockQuotes(),
+          (quote) => ({
+            ...quote,
+            destinationToken: '0xnoConversionRateExists',
+          }),
+        )
+        const expectedResultQuotes = {
+          [TEST_AGG_ID_1]: {
+            ...testInput[TEST_AGG_ID_1],
+            ethFee: '0.01',
+          },
+          [TEST_AGG_ID_2]: {
+            ...testInput[TEST_AGG_ID_2],
+            ethFee: '0.02',
+          },
+          [TEST_AGG_ID_3]: {
+            ...testInput[TEST_AGG_ID_3],
+            ethFee: '0.03',
+          },
+          [TEST_AGG_ID_4]: {
+            ...testInput[TEST_AGG_ID_4],
+            ethFee: '0.04',
+          },
+          [TEST_AGG_ID_5]: {
+            ...testInput[TEST_AGG_ID_5],
+            ethFee: '0.05',
+          },
+          [TEST_AGG_ID_6]: {
+            ...testInput[TEST_AGG_ID_6],
+            ethFee: '0.06',
+          },
+        }
+
+        const [
+          topAggId,
+          resultQuotes,
+        ] = await swapsController._findTopQuoteAndCalculateSavings(testInput)
+        assert.equal(topAggId, TEST_AGG_ID_1)
+        assert.deepStrictEqual(resultQuotes, expectedResultQuotes)
+      })
+
+      it('returns the top aggId and quotes with savings and fee values if passed necessary data and the source token is ETH', async function () {
+        const testInput = mapValues(
+          getTopQuoteAndSavingsMockQuotes(),
+          (quote) => ({
+            ...quote,
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+          }),
+        )
+        const baseExpectedResultQuotes = getTopQuoteAndSavingsBaseExpectedResults()
+        const expectedResultQuotes = {
+          [TEST_AGG_ID_1]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_1],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '2.0195',
+          },
+          [TEST_AGG_ID_2]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_2],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '1.9996',
+          },
+          [TEST_AGG_ID_3]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_3],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '1.9698',
+          },
+          [TEST_AGG_ID_4]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_4],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '1.94',
+          },
+          [TEST_AGG_ID_5]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_5],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '1.9102',
+          },
+          [TEST_AGG_ID_6]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_6],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '1.8705',
+          },
+        }
+
+        const [
+          topAggId,
+          resultQuotes,
+        ] = await swapsController._findTopQuoteAndCalculateSavings(testInput)
+        assert.equal(topAggId, TEST_AGG_ID_1)
+        assert.deepStrictEqual(resultQuotes, expectedResultQuotes)
+      })
+
+      it('returns the top aggId and quotes with savings and fee values if passed necessary data and the source token is ETH and an ETH fee is included in the trade value of what would be the best quote', async function () {
+        const testInput = mapValues(
+          getTopQuoteAndSavingsMockQuotes(),
+          (quote) => ({
+            ...quote,
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+          }),
+        )
+        // 0.04 ETH fee included in trade value
+        testInput[TEST_AGG_ID_1].trade.value = '0x8b553ece48ec0000'
+        const baseExpectedResultQuotes = getTopQuoteAndSavingsBaseExpectedResults()
+        const expectedResultQuotes = {
+          [TEST_AGG_ID_1]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_1],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8b553ece48ec0000' },
+            overallValueOfQuote: '1.9795',
+            ethFee: '0.05',
+          },
+          [TEST_AGG_ID_2]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_2],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '1.9996',
+            isBestQuote: true,
+            savings: {
+              total: '0.0243',
+              performance: '0.0297',
+              fee: '0.015',
+              metaMaskFee: '0.0204',
+              medianMetaMaskFee: '0.0201',
+            },
+          },
+          [TEST_AGG_ID_3]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_3],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '1.9698',
+          },
+          [TEST_AGG_ID_4]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_4],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '1.94',
+          },
+          [TEST_AGG_ID_5]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_5],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '1.9102',
+          },
+          [TEST_AGG_ID_6]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_6],
+            sourceToken: ETH_SWAPS_TOKEN_ADDRESS,
+            destinationToken: '0x1111111111111111111111111111111111111111',
+            trade: { value: '0x8ac7230489e80000' },
+            overallValueOfQuote: '1.8705',
+          },
+        }
+        delete expectedResultQuotes[TEST_AGG_ID_1].isBestQuote
+        delete expectedResultQuotes[TEST_AGG_ID_1].savings
+
+        const [
+          topAggId,
+          resultQuotes,
+        ] = await swapsController._findTopQuoteAndCalculateSavings(testInput)
+        assert.equal(topAggId, TEST_AGG_ID_2)
+        assert.deepStrictEqual(resultQuotes, expectedResultQuotes)
+      })
+
+      it('returns the top aggId and quotes with savings and fee values if passed necessary data and the source token is not ETH and an ETH fee is included in the trade value of what would be the best quote', async function () {
+        const testInput = getTopQuoteAndSavingsMockQuotes()
+        // 0.04 ETH fee included in trade value
+        testInput[TEST_AGG_ID_1].trade.value = '0x8e1bc9bf040000'
+        const baseExpectedResultQuotes = getTopQuoteAndSavingsBaseExpectedResults()
+        const expectedResultQuotes = {
+          ...baseExpectedResultQuotes,
+          [TEST_AGG_ID_1]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_1],
+            trade: { value: '0x8e1bc9bf040000' },
+            overallValueOfQuote: '1.9795',
+            ethFee: '0.05',
+          },
+          [TEST_AGG_ID_2]: {
+            ...baseExpectedResultQuotes[TEST_AGG_ID_2],
+            isBestQuote: true,
+            savings: {
+              total: '0.0243',
+              performance: '0.0297',
+              fee: '0.015',
+              metaMaskFee: '0.0204',
+              medianMetaMaskFee: '0.0201',
+            },
+          },
+        }
+        delete expectedResultQuotes[TEST_AGG_ID_1].isBestQuote
+        delete expectedResultQuotes[TEST_AGG_ID_1].savings
+
+        const [
+          topAggId,
+          resultQuotes,
+        ] = await swapsController._findTopQuoteAndCalculateSavings(testInput)
+        assert.equal(topAggId, [TEST_AGG_ID_2])
+        assert.deepStrictEqual(resultQuotes, expectedResultQuotes)
+      })
     })
 
     describe('fetchAndSetQuotes', function () {
@@ -394,9 +666,15 @@ describe('SwapsController', function () {
           gasEstimateWithRefund: 'b8cae',
           savings: {
             fee: '0',
+            metaMaskFee: '0.5050505050505050505',
             performance: '6',
-            total: '6',
+            total: '5.4949494949494949495',
+            medianMetaMaskFee: '0.44444444444444444444',
           },
+          ethFee: '33554432',
+          overallValueOfQuote: '-33554382',
+          metaMaskFeeInEth: '0.5050505050505050505',
+          ethValueOfTokens: '50',
         })
 
         assert.strictEqual(
@@ -515,7 +793,7 @@ describe('SwapsController', function () {
           MOCK_FETCH_METADATA,
         )
 
-        assert.strictEqual(newQuotes[topAggId].isBestQuote, false)
+        assert.strictEqual(newQuotes[topAggId].isBestQuote, undefined)
       })
     })
 
@@ -865,37 +1143,215 @@ describe('SwapsController', function () {
   })
 
   describe('utils', function () {
-    describe('getMedian', function () {
-      const { getMedian } = utils
+    describe('getMedianEthValueQuote', function () {
+      const { getMedianEthValueQuote } = utils
 
       it('calculates median correctly with uneven sample', function () {
-        const values = [3, 2, 6].map((value) => new BigNumber(value))
-        const median = getMedian(values)
+        const expectedResult = {
+          ethFee: '10',
+          metaMaskFeeInEth: '5',
+          ethValueOfTokens: '0.3',
+        }
+        const values = [
+          {
+            overallValueOfQuote: '3',
+            ethFee: '10',
+            metaMaskFeeInEth: '5',
+            ethValueOfTokens: '0.3',
+          },
+          {
+            overallValueOfQuote: '2',
+            ethFee: '20',
+            metaMaskFeeInEth: '3',
+            ethValueOfTokens: '0.2',
+          },
+          {
+            overallValueOfQuote: '6',
+            ethFee: '40',
+            metaMaskFeeInEth: '6',
+            ethValueOfTokens: '0.6',
+          },
+        ]
 
-        assert.strictEqual(
-          median.toNumber(),
-          3,
-          'should have returned correct median',
+        const median = getMedianEthValueQuote(values)
+
+        assert.deepEqual(
+          median,
+          expectedResult,
+          'should have returned correct median quote object',
         )
       })
 
       it('calculates median correctly with even sample', function () {
-        const values = [3, 2, 2, 6].map((value) => new BigNumber(value))
-        const median = getMedian(values)
+        const expectedResult = {
+          ethFee: '20',
+          metaMaskFeeInEth: '6.5',
+          ethValueOfTokens: '0.25',
+        }
+        const values = [
+          {
+            overallValueOfQuote: '3',
+            ethFee: '10',
+            metaMaskFeeInEth: '5',
+            ethValueOfTokens: '0.3',
+          },
+          {
+            overallValueOfQuote: '1',
+            ethFee: '20',
+            metaMaskFeeInEth: '3',
+            ethValueOfTokens: '0.2',
+          },
+          {
+            overallValueOfQuote: '2',
+            ethFee: '30',
+            metaMaskFeeInEth: '8',
+            ethValueOfTokens: '0.2',
+          },
+          {
+            overallValueOfQuote: '6',
+            ethFee: '40',
+            metaMaskFeeInEth: '6',
+            ethValueOfTokens: '0.6',
+          },
+        ]
+        const median = getMedianEthValueQuote(values)
 
-        assert.strictEqual(
-          median.toNumber(),
-          2.5,
-          'should have returned correct median',
+        assert.deepEqual(
+          median,
+          expectedResult,
+          'should have returned correct median quote object',
+        )
+      })
+
+      it('calculates median correctly with an uneven sample where multiple quotes have the median overall value', function () {
+        const expectedResult = {
+          ethFee: '2',
+          metaMaskFeeInEth: '0.5',
+          ethValueOfTokens: '5',
+        }
+
+        const values = [
+          {
+            overallValueOfQuote: '1',
+            ethValueOfTokens: '2',
+            ethFee: '1',
+            metaMaskFeeInEth: '0.2',
+          },
+          {
+            overallValueOfQuote: '3',
+            ethValueOfTokens: '4',
+            ethFee: '1',
+            metaMaskFeeInEth: '0.4',
+          },
+          {
+            overallValueOfQuote: '3',
+            ethValueOfTokens: '5',
+            ethFee: '2',
+            metaMaskFeeInEth: '0.5',
+          },
+          {
+            overallValueOfQuote: '3',
+            ethValueOfTokens: '6',
+            ethFee: '3',
+            metaMaskFeeInEth: '0.6',
+          },
+          {
+            overallValueOfQuote: '4',
+            ethValueOfTokens: '6',
+            ethFee: '2',
+            metaMaskFeeInEth: '0.6',
+          },
+          {
+            overallValueOfQuote: '4',
+            ethValueOfTokens: '7',
+            ethFee: '3',
+            metaMaskFeeInEth: '0.7',
+          },
+          {
+            overallValueOfQuote: '6',
+            ethValueOfTokens: '8',
+            ethFee: '2',
+            metaMaskFeeInEth: '0.8',
+          },
+        ]
+        const median = getMedianEthValueQuote(values)
+
+        assert.deepEqual(
+          median,
+          expectedResult,
+          'should have returned correct median quote object',
+        )
+      })
+
+      it('calculates median correctly with an even sample where multiple quotes have the same overall value as either of the two middle values', function () {
+        const expectedResult = {
+          ethFee: '2',
+          metaMaskFeeInEth: '0.55',
+          ethValueOfTokens: '5.5',
+        }
+
+        const values = [
+          {
+            overallValueOfQuote: '1',
+            ethValueOfTokens: '2',
+            ethFee: '1',
+            metaMaskFeeInEth: '0.2',
+          },
+          {
+            overallValueOfQuote: '3',
+            ethValueOfTokens: '4',
+            ethFee: '1',
+            metaMaskFeeInEth: '0.4',
+          },
+          {
+            overallValueOfQuote: '3',
+            ethValueOfTokens: '5',
+            ethFee: '2',
+            metaMaskFeeInEth: '0.5',
+          },
+          {
+            overallValueOfQuote: '4',
+            ethValueOfTokens: '6',
+            ethFee: '2',
+            metaMaskFeeInEth: '0.6',
+          },
+          {
+            overallValueOfQuote: '4',
+            ethValueOfTokens: '7',
+            ethFee: '3',
+            metaMaskFeeInEth: '0.7',
+          },
+          {
+            overallValueOfQuote: '6',
+            ethValueOfTokens: '8',
+            ethFee: '2',
+            metaMaskFeeInEth: '0.8',
+          },
+        ]
+        const median = getMedianEthValueQuote(values)
+
+        assert.deepEqual(
+          median,
+          expectedResult,
+          'should have returned correct median quote object',
         )
       })
 
       it('throws on empty or non-array sample', function () {
-        assert.throws(() => getMedian([]), 'should throw on empty array')
+        assert.throws(
+          () => getMedianEthValueQuote([]),
+          'should throw on empty array',
+        )
 
-        assert.throws(() => getMedian(), 'should throw on non-array param')
+        assert.throws(
+          () => getMedianEthValueQuote(),
+          'should throw on non-array param',
+        )
 
-        assert.throws(() => getMedian({}), 'should throw on non-array param')
+        assert.throws(
+          () => getMedianEthValueQuote({}),
+          'should throw on non-array param',
+        )
       })
     })
   })
@@ -934,6 +1390,7 @@ function getMockQuotes() {
         symbol: 'USDC',
         decimals: 18,
       },
+      fee: 1,
     },
 
     [TEST_AGG_ID_BEST]: {
@@ -967,6 +1424,7 @@ function getMockQuotes() {
         symbol: 'USDC',
         decimals: 18,
       },
+      fee: 1,
     },
 
     [TEST_AGG_ID_2]: {
@@ -1000,6 +1458,160 @@ function getMockQuotes() {
         symbol: 'USDC',
         decimals: 18,
       },
+      fee: 1,
+    },
+  }
+}
+
+function getTopQuoteAndSavingsMockQuotes() {
+  // These destination amounts are calculated using the following "pre-fee" amounts
+  // TEST_AGG_ID_1: 20.5
+  // TEST_AGG_ID_2: 20.4
+  // TEST_AGG_ID_3: 20.2
+  // TEST_AGG_ID_4: 20
+  // TEST_AGG_ID_5: 19.8
+  // TEST_AGG_ID_6: 19.5
+
+  return {
+    [TEST_AGG_ID_1]: {
+      aggregator: TEST_AGG_ID_1,
+      approvalNeeded: null,
+      gasEstimate: '0x186a0',
+      destinationAmount: '20295000000000000000',
+      destinationToken: '0x1111111111111111111111111111111111111111',
+      destinationTokenInfo: { decimals: 18 },
+      sourceAmount: '10000000000000000000',
+      sourceToken: '0xsomeERC20TokenAddress',
+      trade: {
+        value: '0x0',
+      },
+      fee: 1,
+    },
+    [TEST_AGG_ID_2]: {
+      aggregator: TEST_AGG_ID_2,
+      approvalNeeded: null,
+      gasEstimate: '0x30d40',
+      destinationAmount: '20196000000000000000',
+      destinationToken: '0x1111111111111111111111111111111111111111',
+      destinationTokenInfo: { decimals: 18 },
+      sourceAmount: '10000000000000000000',
+      sourceToken: '0xsomeERC20TokenAddress',
+      trade: {
+        value: '0x0',
+      },
+      fee: 1,
+    },
+    [TEST_AGG_ID_3]: {
+      aggregator: TEST_AGG_ID_3,
+      approvalNeeded: null,
+      gasEstimate: '0x493e0',
+      destinationAmount: '19998000000000000000',
+      destinationToken: '0x1111111111111111111111111111111111111111',
+      destinationTokenInfo: { decimals: 18 },
+      sourceAmount: '10000000000000000000',
+      sourceToken: '0xsomeERC20TokenAddress',
+      trade: {
+        value: '0x0',
+      },
+      fee: 1,
+    },
+    [TEST_AGG_ID_4]: {
+      aggregator: TEST_AGG_ID_4,
+      approvalNeeded: null,
+      gasEstimate: '0x61a80',
+      destinationAmount: '19800000000000000000',
+      destinationToken: '0x1111111111111111111111111111111111111111',
+      destinationTokenInfo: { decimals: 18 },
+      sourceAmount: '10000000000000000000',
+      sourceToken: '0xsomeERC20TokenAddress',
+      trade: {
+        value: '0x0',
+      },
+      fee: 1,
+    },
+    [TEST_AGG_ID_5]: {
+      aggregator: TEST_AGG_ID_5,
+      approvalNeeded: null,
+      gasEstimate: '0x7a120',
+      destinationAmount: '19602000000000000000',
+      destinationToken: '0x1111111111111111111111111111111111111111',
+      destinationTokenInfo: { decimals: 18 },
+      sourceAmount: '10000000000000000000',
+      sourceToken: '0xsomeERC20TokenAddress',
+      trade: {
+        value: '0x0',
+      },
+      fee: 1,
+    },
+    [TEST_AGG_ID_6]: {
+      aggregator: TEST_AGG_ID_6,
+      approvalNeeded: null,
+      gasEstimate: '0x927c0',
+      destinationAmount: '19305000000000000000',
+      destinationToken: '0x1111111111111111111111111111111111111111',
+      destinationTokenInfo: { decimals: 18 },
+      sourceAmount: '10000000000000000000',
+      sourceToken: '0xsomeERC20TokenAddress',
+      trade: {
+        value: '0x0',
+      },
+      fee: 1,
+    },
+  }
+}
+
+function getTopQuoteAndSavingsBaseExpectedResults() {
+  const baseTestInput = getTopQuoteAndSavingsMockQuotes()
+  return {
+    [TEST_AGG_ID_1]: {
+      ...baseTestInput[TEST_AGG_ID_1],
+      isBestQuote: true,
+      ethFee: '0.01',
+      overallValueOfQuote: '2.0195',
+      metaMaskFeeInEth: '0.0205',
+      ethValueOfTokens: '2.0295',
+      savings: {
+        total: '0.0441',
+        performance: '0.0396',
+        fee: '0.025',
+        metaMaskFee: '0.0205',
+        medianMetaMaskFee: '0.0201',
+      },
+    },
+    [TEST_AGG_ID_2]: {
+      ...baseTestInput[TEST_AGG_ID_2],
+      ethFee: '0.02',
+      overallValueOfQuote: '1.9996',
+      metaMaskFeeInEth: '0.0204',
+      ethValueOfTokens: '2.0196',
+    },
+    [TEST_AGG_ID_3]: {
+      ...baseTestInput[TEST_AGG_ID_3],
+      ethFee: '0.03',
+      overallValueOfQuote: '1.9698',
+      metaMaskFeeInEth: '0.0202',
+      ethValueOfTokens: '1.9998',
+    },
+    [TEST_AGG_ID_4]: {
+      ...baseTestInput[TEST_AGG_ID_4],
+      ethFee: '0.04',
+      overallValueOfQuote: '1.94',
+      metaMaskFeeInEth: '0.02',
+      ethValueOfTokens: '1.98',
+    },
+    [TEST_AGG_ID_5]: {
+      ...baseTestInput[TEST_AGG_ID_5],
+      ethFee: '0.05',
+      overallValueOfQuote: '1.9102',
+      metaMaskFeeInEth: '0.0198',
+      ethValueOfTokens: '1.9602',
+    },
+    [TEST_AGG_ID_6]: {
+      ...baseTestInput[TEST_AGG_ID_6],
+      ethFee: '0.06',
+      overallValueOfQuote: '1.8705',
+      metaMaskFeeInEth: '0.0195',
+      ethValueOfTokens: '1.9305',
     },
   }
 }