Include trade.value in calculation of displayed network fees

[danjm]

This PR corrects an error in how we were rendering the displayed network fees on the view quote screen. For some aggregators, trade.value can be greater than the amount of eth being swapped (0 in the case of swapping an ERC-20, and sourceAmount in the case of swapping ETH). We weren't properly factoring this in before, and this could cause some rendered networks fees to be a little lower than they should be.

[github-actions]

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.

[metamaskbot]

Builds ready [57c0821]

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

Page Load Metrics (412 ± 50 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	43	36	4	2
		domContentLoaded	251	610	410	105	51
		load	253	612	412	105	50
		domInteractive	251	610	410	105	51

[rekmarks]

Rather than repeating the ethFee calculations in the UI, why don't we slap the ethFee calculated in the background (introduced in #9611) onto the quote objects and use them directly?

[Gudahtt]

I'm not sure I understand that description of the problem.

Are you saying that the "network fee" was lower than intended for some aggregators, because the "network fee" is intended to be the gas fee plus the aggregator fee (at least in the cases where the aggregator fee was included in trade.value)? The tooltip shown in-app describes the network fee as being solely the gas fee.

[danjm]

Let's block this PR on #9675, and then refactor it rely more on some of the logic added to the background controller in that PR

[danjm]

Are you saying that the "network fee" was lower than intended for some aggregators, because the "network fee" is intended to be the gas fee plus the aggregator fee (at least in the cases where the aggregator fee was included in trade.value)?

Yes, but for the displayed network fee only. The amount submitted is correct.

The tooltip shown in-app describes the network fee as being solely the gas fee.

Right, and the custom gas modal does not represent this in the total. I'll see if I can put together a straightforward fix for that.

[Gudahtt]

I was not referring to the custom gas modal. The tooltip for "Estimated network fee" on the "View quote" page says this:

The gas fee covers the cost of processing your swap and storing it on the Ethereum network. MetaMask does not profit from this fee.

It seems misleading to describe it as a "gas fee" and to make no mention that it includes more than just the gas fee.

[Gudahtt]

Was this an attempt to remove the dollar sign from the value? This will be broken for any locale that represents numbers in some manner other than <symbol><number>.

formatCurrency returns a string that is meant to be displayed exactly as-is. You'll need to return the number directly if you want to use it without any symbol.

[Gudahtt]

I'm noticing now that this was a pre-existing problem. It's definitely a release blocker though - this would give very wrong answers in some cases (e.g. 1,000 $ would turn into 000 $).

[Gudahtt]

I've discovered that this is done in two other places as well, but in all cases the result is used solely for metrics. So maybe not a release blocker then luckily.

Still something we should fix at some point though, so our metrics aren't skewed for other locales.

[danjm]

This has been addressed in a separate PR: #9727

There were more problems than just stated in the above comments. In particular, as all of these are just used for our metrics, they should all be denominated in USD.

[danjm]

I was not referring to the custom gas modal. The tooltip for "Estimated network fee" on the "View quote" page says this:

Yeah, I was not clear in my response. I said "Right," which was an affirmation of your point about the tooltip, and then I said something about the custom gas modal to say that your general point also applies to that in a way. Right now we say you can "Edit" the network fee, which includes these aggregator fees, clicking "Edit" opens the custom gas modal, but these aggregator fees are not represented there.

[danjm]

@Gudahtt eecf114fd updates the tooltip copy and the display of fees in the gas customization modal

Here are some screenshots of the latter (done by adding fake extra fees to the api response because at present no aggregator is including those extra fees... at least on the few quotes I tested)

Screenshots

[Gudahtt]

LGTM! One minor suggestion

[Gudahtt]

Oh, prettier needs to be run again to sort the new localized messages.

[danjm]

@Gudahtt message sorting / linting done in latest commit

[metamaskbot]

Builds ready [8b66039]

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

Page Load Metrics (452 ± 73 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	80	41	14	7
		domContentLoaded	265	764	450	151	73
		load	266	766	452	151	73
		domInteractive	265	763	450	151	73