Alternative savings fix by danjm · Pull Request #9675 · MetaMask/metamask-extension

danjm · 2020-10-22T07:22:31Z

This PR replaces #9662, and fixes some additional issues

The most fundamental change made in this PR is an update to the basic formula used for calculating total savings:

average savings = (how much your net worth will change if you use the metaswap best quote) - (how much your net worth would have changed if you had swapped the median quote directly via the aggregator)

where:
(how much your net worth will change if you use the metaswap best quote) = (usd value of tokens received) - (usd network fees) 
and where:

(how much your net worth would have changed if you had swapped the average quote directly via the aggregator) = ((usd value of tokens that would be received from the average quote) / (1 - metamask rate)) - (usd network fees of the average quote)

And so the the savings.total property is now calculated directly from the overall eth value of the best trade to the overall eth value of the median trade. Related to this is that savings.performance is now calculated by subtracting fee savings from total savings.

Another important fix is that what was previously called ethValueOfTrade is no longer used in calculations of values that will be shown to the user on the front end. It has been renamed to tokenValueOfQuoteForSorting to reflect its purpose. This variable assumes a tokenConversionRate of 1 if none is available, which is fine for comparing quotes to calculate the top quote in such cases, but would be meaningless or misleading if used to calculate user facing values. A new ethValueOfQuote variable has been created that defaults the conversion rate to 0 if not available, which will result in ethValueOfQuote being zero (in this case, the UI will have to choose not to render savings data).

Also important change is that savings.fee is now calculated by comparing the fee of the best quote to the fee of the quote with the media value, as opposed to comparing the fee of the best quote to the median fee. To support this, the getMedian function has been refactored to a function that takes a list of quotes and returns an object with relevant savings information from the quote with the media ethValueOfQuote

Finally, this PR ensures that the metamask fee is accounted for in the savings calculations and makes that information available in the savings property for use in the UI

There are some other code refactors made to improve readability given the above changes.

github-actions · 2020-10-22T07:22:46Z

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 · 2020-10-22T07:25:33Z

Builds ready [4cbdce7]

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

Page Load Metrics (460 ± 72 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	103	43	17	8
		domContentLoaded	301	852	459	150	72
		load	303	853	460	150	72
		domInteractive	301	852	459	150	72

metamaskbot · 2020-10-22T08:09:59Z

Builds ready [609b0d7]

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

Page Load Metrics (427 ± 59 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	45	35	4	2
		domContentLoaded	291	609	425	122	59
		load	293	612	427	122	59
		domInteractive	291	609	425	122	59

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

app/scripts/controllers/swaps.js

metamaskbot · 2020-10-26T19:56:39Z

Builds ready [24a011e]

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

Page Load Metrics (460 ± 61 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	83	37	12	6
		domContentLoaded	295	626	459	126	61
		load	297	627	460	126	61
		domInteractive	295	625	458	126	61

…sk fees

… .sort() call

danjm · 2020-11-04T16:30:21Z

I just pushed a number of changes, including a rebase onto develop. I will reply to previous comments and add further commentary on latest changes in a little while.

danjm · 2020-11-04T17:33:19Z

6dc0b6d Added unit tests for _findTopQuoteAndCalculateSavings for the case of ERC-20 -> ERC-20 swaps when the destination conversion rate is known. We should also add unit tests for:

ETH -> ERC-20 swaps, ~~for both when the destination conversion rate is known and unknown~~
ERC-20 -> ERC-20 swaps, for when the destination conversion rate is unknown
both swaps where ETH and where ERC-20 is being received, and trade.value includes some amount for a fee

So 5 sets of unit tests to add.

metamaskbot · 2020-11-04T17:51:24Z

Builds ready [d1f4ddc]

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

Page Load Metrics (456 ± 59 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	37	115	50	19	9
		domContentLoaded	291	802	454	122	59
		load	292	803	456	122	59
		domInteractive	290	802	454	122	59

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

danjm · 2020-11-05T15:29:38Z

@Gudahtt @rekmarks This is again ready for review.

Some notables changes that have been made:

I have removed logic that attempts to determine cost "if you had swapped the average quote directly via the aggregator", and have gone back to the old savings calculation based on comparing received tokens and network fees of our best quote to our median quote. Whether we eventually modify this calculation is a UX decision that we are not yet ready to make. For now, for the purposes of analytics on savings, this gives us all the data we need. The metamask fee that would be extracted on the median quote is also now captured, so that we can calculate cost "if you had swapped the average quote directly via the aggregator" from our analytics data if we want. (These changes made in 333a41a)
In response to some of @Gudahtt's comments, and in general to try to make the _findTopQuoteAndCalculateSavings more understand I have reorganized some of the logic and renamed some variables (These changes made in 333a41a)
The _findTopQuoteAndCalculateSavings no longer mutates the quote object it is passed (These changes made in 333a41a)
I have now added many more unit tests for the different scenarios that have to be handled by _findTopQuoteAndCalculateSavings (These changes made in 2217b02)
getMedianEthValueQuote also no longer mutates the array it is passed (e826431)

metamaskbot · 2020-11-05T15:41:33Z

Builds ready [2217b02]

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

Page Load Metrics (494 ± 72 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	82	40	12	6
		domContentLoaded	297	850	492	150	72
		load	298	851	494	150	72
		domInteractive	296	849	492	150	72

app/scripts/controllers/swaps.js

…TH is the source token

metamaskbot · 2020-11-05T18:09:44Z

Builds ready [e52eb94]

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 ± 59 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	100	40	15	7
		domContentLoaded	246	691	373	123	59
		load	247	692	375	123	59
		domInteractive	246	691	373	123	59

danjm · 2020-11-06T01:15:11Z

@Gudahtt comments have been addressed

metamaskbot · 2020-11-06T01:37:06Z

Builds ready [1458ad7]

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

Page Load Metrics (543 ± 74 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	105	49	16	8
		domContentLoaded	380	888	541	154	74
		load	382	894	543	154	74
		domInteractive	380	888	541	154	74

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

…cted results helper functions

metamaskbot · 2020-11-09T11:29:55Z

Builds ready [e4bdc81]

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

Page Load Metrics (421 ± 63 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	90	38	13	6
		domContentLoaded	243	668	419	132	63
		load	244	670	421	132	63
		domInteractive	242	668	419	132	63

…rall values equal to the median

metamaskbot · 2020-11-09T16:31:50Z

Builds ready [ec9f935]

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

Page Load Metrics (571 ± 80 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	32	116	68	26	12
		domContentLoaded	352	834	569	167	80
		load	355	835	571	168	80
		domInteractive	351	834	568	167	80

app/scripts/controllers/swaps.js

Gudahtt

LGTM!

metamaskbot · 2020-11-09T17:08:58Z

Builds ready [59faf71]

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

Page Load Metrics (448 ± 58 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	92	37	13	6
		domContentLoaded	300	685	447	121	58
		load	301	687	448	121	58
		domInteractive	300	685	446	121	58

danjm requested a review from a team as a code owner October 22, 2020 07:22

danjm requested review from Gudahtt and rekmarks October 22, 2020 07:22

danjm added the area-swaps label Oct 22, 2020

danjm mentioned this pull request Oct 22, 2020

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

Merged

danjm added the T16-deploy-blocker label Oct 26, 2020

Gudahtt reviewed Oct 26, 2020

View reviewed changes

brad-decker removed the T16-deploy-blocker label Oct 26, 2020

danjm mentioned this pull request Oct 27, 2020

Use better value for average savings analytics #9733

Merged

Gudahtt mentioned this pull request Oct 27, 2020

Update fee card designs to show savings and MM fee #9629

Merged

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

danjm assigned Gudahtt and rekmarks Nov 3, 2020

danjm added 7 commits November 4, 2020 12:27

Alternative savings fix

ae7ace9

Further required changes to savings fix

c6bcf3c

Further fix to savings calculations that properly accounts for metama…

7347a50

…sk fees

metaMaskFeeInEth property on quotes to decimal string

7de759a

Fix swaps controller unit tests

f39f80b

Improve documentation in swaps controller

88f72d8

Prevent getMedianEthValueQuote from mutation passed quotes array with…

e826431

… .sort() call

danjm force-pushed the alternative-savings-fix branch from 24a011e to 6dc0b6d Compare November 4, 2020 16:27

danjm marked this pull request as draft November 4, 2020 17:03

danjm force-pushed the alternative-savings-fix branch from d1f4ddc to e4b9cd3 Compare November 5, 2020 15:05

danjm marked this pull request as ready for review November 5, 2020 15:06

danjm added 2 commits November 5, 2020 11:48

Add and update unit tests for _findTopQuoteAndCalculateSavings

2217b02

danjm force-pushed the alternative-savings-fix branch from e4b9cd3 to 2217b02 Compare November 5, 2020 15:19

Gudahtt reviewed Nov 5, 2020

View reviewed changes

Improve calculation of overallValueOfQuoteForSorting for case where E…

e52eb94

…TH is the source token

Clean up getMedianEthValueQuote code, test and comments

1458ad7

Gudahtt reviewed Nov 6, 2020

View reviewed changes

test/unit/app/controllers/swaps-test.js Outdated Show resolved Hide resolved

test/unit/app/controllers/swaps-test.js Outdated Show resolved Hide resolved

test/unit/app/controllers/swaps-test.js Show resolved Hide resolved

test/unit/app/controllers/swaps-test.js Show resolved Hide resolved

Clean up _findTopQuoteAndCalculateSavings, create test input and expe…

e4bdc81

…cted results helper functions

Update getMedianEthValueQuote to account for multiple quotes with ove…

ec9f935

…rall values equal to the median

Gudahtt reviewed Nov 9, 2020

View reviewed changes

app/scripts/controllers/swaps.js Outdated Show resolved Hide resolved

danjm added 2 commits November 9, 2020 13:12

Add jsdoc comment for meansOfQuotesFeesAndValue

9ec3a0d

Fix jsdoc comment for getMedianEthValueQuote

59faf71

Gudahtt approved these changes Nov 9, 2020

View reviewed changes

danjm merged commit c044b6f into develop Nov 9, 2020

danjm deleted the alternative-savings-fix branch November 9, 2020 17:09

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

Conversation

danjm commented Oct 22, 2020

Uh oh!

github-actions bot commented Oct 22, 2020

Uh oh!

metamaskbot commented Oct 22, 2020

Uh oh!

metamaskbot commented Oct 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Oct 26, 2020

Uh oh!

danjm commented Nov 4, 2020

Uh oh!

danjm commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metamaskbot commented Nov 4, 2020

Uh oh!

danjm commented Nov 5, 2020

Uh oh!

metamaskbot commented Nov 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Nov 5, 2020

Uh oh!

danjm commented Nov 6, 2020

Uh oh!

metamaskbot commented Nov 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Nov 9, 2020

Uh oh!

metamaskbot commented Nov 9, 2020

Uh oh!

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Nov 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danjm commented Nov 4, 2020 •

edited

Loading