Fix gas_fees properties collected for swaps analytics events #9727

danjm · 2020-10-27T12:21:38Z

This is in response to a comment on another PR.

A few fixes are included here. The most important is that the gas_fees property on analytics events should be denominated in USD. This will allow all gas fees data to be easily comparible. This requires MetaMask/core#292

Additional fixes are commented on in the diff

github-actions · 2020-10-27T12:21:55Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

danjm · 2020-10-27T12:22:31Z

...ponents/app/gas-customization/gas-modal-page-container/gas-modal-page-container.component.js

@@ -239,7 +238,11 @@ export default class GasModalPageContainer extends Component {
                properties: {
                  speed_set: speedSet,
                  gas_mode: this.state.selectedTab,
-                  gas_fees: sumHexWEIsToRenderableFiat([this.props.value, newSwapGasTotal, this.props.customTotalSupplement], 'usd', this.props.conversionRate)?.slice(1),
+                  gas_fees: sumHexWEIsToUnformattedFiat(


this.props.value should not be included in the "gas_fees" that are calculated here

danjm · 2020-10-27T12:23:18Z

ui/app/ducks/swaps/swaps.js

    const destinationValue = calcTokenAmount(usedQuote.destinationAmount, destinationTokenInfo.decimals || 18).toPrecision(8)
    const usedGasLimitEstimate = usedQuote?.gasEstimateWithRefund || (`0x${decimalToHex(usedQuote?.averageGas || 0)}`)
    const totalGasLimitEstimate = (new BigNumber(usedGasLimitEstimate, 16)).plus(usedQuote.approvalNeeded?.gas || '0x0', 16).toString(16)
-    const gasEstimateTotalInEth = getValueFromWeiHex({
+    const gasEstimateTotalInUSD = getValueFromWeiHex({


The name gasEstimateTotalInEth did not reflect the the content/value of this variable

danjm · 2020-10-27T12:24:06Z

ui/app/ducks/swaps/swaps.js

@@ -480,7 +479,7 @@ export const signAndSendTransactions = (history, metaMetricsEvent) => {
      available_quotes: getQuotes(state)?.length,
      other_quote_selected: usedQuote.aggregator !== getTopQuote(state)?.aggregator,
      other_quote_selected_source: usedQuote.aggregator === getTopQuote(state)?.aggregator ? '' : usedQuote.aggregator,
-      gas_fees: formatCurrency(gasEstimateTotalInEth, 'usd')?.slice(1),
+      gas_fees: gasEstimateTotalInUSD,


Formatting is both unnecessary and diverges from what we want in our analytics, which is an unformatted decimal number

danjm · 2020-10-27T12:25:06Z

ui/app/helpers/utils/conversions.util.js

@@ -171,14 +171,23 @@ export function addHexes (aHexWEI, bHexWEI) {
  })
 }



The changes in this file don't change the functionality of sumHexWEIsToRenderableFiat, while making part of the logic previously in sumHexWEIsToRenderableFiat exportable via sumHexWEIsToUnformattedFiat

danjm · 2020-10-27T12:25:46Z

ui/app/pages/swaps/awaiting-swap/awaiting-swap.js

  if (usedQuote && tradeTxParams) {
-    const renderableGasFees = getRenderableGasFeesForQuote(usedQuote.gasEstimateWithRefund || usedQuote.averageGas, approveTxParams?.gas || '0x0', tradeTxParams.gasPrice, currentCurrency, conversionRate)
-    feeinFiat = renderableGasFees.feeinFiat?.slice(1)
+    const renderableGasFees = getRenderableGasFeesForQuote(usedQuote.gasEstimateWithRefund || usedQuote.averageGas, approveTxParams?.gas || '0x0', tradeTxParams.gasPrice, currentCurrency, usdConversionRate)


Formatting is both unnecessary and diverges from what we want in our analytics, which is an unformatted decimal number, which is what rawNetworkFees gives us.

danjm · 2020-10-27T12:27:28Z

ui/app/ducks/swaps/swaps.js

      numberOfDecimals: 6,
    })
    const averageSavings = usedQuote.isBestQuote
      ? decEthToConvertedCurrency(
        usedQuote.savings?.total,
        'usd',
-        conversionRate,
+        usdConversionRate,


While not a gas_fees property, the averageSavings calculation was also incorrectly using on the conversionRate, when we want our analytics in USD

brad-decker · 2020-11-03T18:09:37Z

@danjm sorry for dropping the ball on reviewing this. Could you rebase and I'll make this my next priority?

danjm · 2020-11-03T20:24:37Z

@brad-decker this has been rebased on top of latest develop

brad-decker · 2020-11-04T20:43:37Z

@danjm this has conflict markers in it, e.g <<<< HEAD

danjm · 2020-11-06T15:50:06Z

@brad-decker This has been rebased, and the error you identified removed

metamaskbot · 2020-11-06T16:49:42Z

Builds ready [eedd5e4]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (434 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	84	37	11	5
		domContentLoaded	269	629	432	125	60
		load	270	630	434	125	60
		domInteractive	269	628	432	125	60

brad-decker

LGTM, just a question that is non blocking

brad-decker · 2020-11-06T17:20:43Z

ui/app/pages/swaps/awaiting-swap/awaiting-swap.js

@@ -102,7 +102,7 @@ export default function AwaitingSwap({
      request_type: fetchParams?.balanceError ? 'Quote' : 'Order',
      slippage: fetchParams?.slippage,
      custom_slippage: fetchParams?.slippage === 2,
-      gas_fees: feeinFiat,
+      gas_fees: feeinUnformattedFiat,


do we also want to trade the usdConversionRate at the time of transaction or no?

yeah, sounds like a good idea

oh, actually, maybe not...

I don't think this gives us anything we need. We might want to also add the used gas limit here (as we already know the price). But I think that is for a separate PR.

cool, thanks @danjm

metamaskbot · 2020-11-10T17:16:41Z

Builds ready [2198a8a]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (375 ± 63 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	80	40	10	5
		domContentLoaded	263	748	374	131	63
		load	265	749	375	131	63
		domInteractive	263	747	373	131	63

danjm requested a review from a team as a code owner October 27, 2020 12:21

danjm requested a review from kumavis October 27, 2020 12:21

danjm commented Oct 27, 2020

View reviewed changes

danjm mentioned this pull request Oct 27, 2020

Include trade.value in calculation of displayed network fees #9621

Merged

danjm added this to the v8.1.next? milestone Nov 2, 2020

danjm added the area-swaps label Nov 2, 2020

danjm requested review from brad-decker and rekmarks November 3, 2020 17:25

danjm assigned brad-decker and rekmarks Nov 3, 2020

danjm force-pushed the usd-swaps-analytics-gas-fees branch from a1e44a1 to 9aa8bd5 Compare November 3, 2020 20:24

danjm force-pushed the usd-swaps-analytics-gas-fees branch from 9aa8bd5 to c9eac81 Compare November 4, 2020 02:50

danjm force-pushed the usd-swaps-analytics-gas-fees branch from c9eac81 to 6f15458 Compare November 6, 2020 15:49

danjm force-pushed the usd-swaps-analytics-gas-fees branch from 6f15458 to eedd5e4 Compare November 6, 2020 16:26

brad-decker previously approved these changes Nov 6, 2020

View reviewed changes

danjm dismissed brad-decker’s stale review via 588f472 November 10, 2020 13:51

danjm force-pushed the usd-swaps-analytics-gas-fees branch from eedd5e4 to 588f472 Compare November 10, 2020 13:51

Fix gas_fees properties collected for swaps analytics events

2198a8a

danjm force-pushed the usd-swaps-analytics-gas-fees branch from 588f472 to 2198a8a Compare November 10, 2020 16:54

brad-decker approved these changes Nov 10, 2020

View reviewed changes

danjm merged commit bcd5f2a into develop Nov 10, 2020

danjm deleted the usd-swaps-analytics-gas-fees branch November 10, 2020 17:39

github-actions bot locked and limited conversation to collaborators Nov 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gas_fees properties collected for swaps analytics events #9727

Fix gas_fees properties collected for swaps analytics events #9727

danjm commented Oct 27, 2020

github-actions bot commented Oct 27, 2020

danjm Oct 27, 2020

danjm Oct 27, 2020

danjm Oct 27, 2020

danjm Oct 27, 2020

danjm Oct 27, 2020

danjm Oct 27, 2020

brad-decker commented Nov 3, 2020

danjm commented Nov 3, 2020

brad-decker commented Nov 4, 2020

danjm commented Nov 6, 2020

metamaskbot commented Nov 6, 2020

brad-decker left a comment

brad-decker Nov 6, 2020

danjm Nov 6, 2020

danjm Nov 6, 2020

danjm Nov 6, 2020

brad-decker Nov 6, 2020

metamaskbot commented Nov 10, 2020

		@@ -171,14 +171,23 @@ export function addHexes (aHexWEI, bHexWEI) {
		})
		}

Fix gas_fees properties collected for swaps analytics events #9727

Fix gas_fees properties collected for swaps analytics events #9727

Conversation

danjm commented Oct 27, 2020

github-actions bot commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brad-decker commented Nov 3, 2020

danjm commented Nov 3, 2020

brad-decker commented Nov 4, 2020

danjm commented Nov 6, 2020

metamaskbot commented Nov 6, 2020

brad-decker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Nov 10, 2020