Fix gas_fees properties collected for swaps analytics events by danjm · Pull Request #9727 · MetaMask/metamask-extension

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

                  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 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

      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

        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

      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

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

Conversation

danjm commented Oct 27, 2020

Uh oh!

github-actions bot commented Oct 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brad-decker commented Nov 3, 2020

Uh oh!

danjm commented Nov 3, 2020

Uh oh!

brad-decker commented Nov 4, 2020

Uh oh!

danjm commented Nov 6, 2020

Uh oh!

metamaskbot commented Nov 6, 2020

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Nov 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants