fix contract exchange rate race condition

[brad-decker]

Fixes: #10189

Token fiat amounts would become out of sync with one another because contract exchange rates are fetched when tokens are added, not when the network switches. As a result, token exchange rates would get updated on network switch as a result of the token list changing -- but the token rates controller's event was happening prior to the currency rate controller's native currency key being updated.

To fix this I simplified the code a bit, breaking apart the dependency of token-rates-controller to currency-rate-controller. The only thing that token rates needed from currency rates was the native currency. Because currency rates controller also has a dependency on the network's native currency I decided to tie token rates controller to the network instead.

I also removed the 'preferences' setter in the controller because it confused me. in no place are we setting preferences on the controller except the constructor.

[metamaskbot]

Builds ready [afeb45e]

• builds: chrome, firefox, opera
• bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
• code coverage: Report
• storybook: Storybook
• all artifacts

Page Load Metrics (606 ± 47 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	47	83	60	9	4
		domContentLoaded	393	827	605	97	47
		load	394	828	606	97	47
		domInteractive	393	827	605	97	47

[Gudahtt]

These changes all seem like improvements!

I'm still a little fuzzy on the order-of-operations when a network switch happens. It seems like now the exchange rate update is being triggered twice - once as a result of the network change, and a second time as a result of the tokens changing.

[brad-decker]

It seems like now the exchange rate update is being triggered twice - once as a result of the network change, and a second time as a result of the tokens changing.

this is true, but we need both in the case where tokens change without a network change.

[Gudahtt]

🤔 Could we only update on a token change than, and assume it will always change when the network switches?

It does seem to change every time at the moment. But. That doesn't seem great to have that assumption baked in here when it might not always apply...

[brad-decker]

You'd still have a race condition then if the token preference change occurs before the network switch, which doesn't seem likely.

[brad-decker]

removing the additional sync led to a race condition, which is weird but I think I found a way around it. The race condition occurs because we're copying the nativeCurrency into this controller's local context.

By making the updateExchangeRates function always ask for the most recent nativeCurrency from the network controller, we guarantee that even if there is some weird anomaly with nativeCurrency changing after tokens are set that we can at least recover from it on the next tick of the timer in token-rates.

in practice, this failed state doesn't seem to occur. 🎉

Here's my theory:

1. User changes network.
   2a. Preferences controller is listening for providerStore state changes, NOT NetworkDidChange. Updates Token state.
   2b. Network Did Change Fires
   3a. Token Rates updates its tokens and gets new exchange rates.
   3b. Token Rates Controller sets new nativeCurrency & requested new exchange rates

By removing 3b, it broke this paradigm. However ticker is part of the provider store, so by always getting the value fresh from this store before fetching exchange rates you're guaranteed to be using the value that was set in #2a before tokens are updated.

[metamaskbot]

Builds ready [96ad3e2]

• builds: chrome, firefox, opera
• bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
• code coverage: Report
• storybook: Storybook
• all artifacts

Page Load Metrics (592 ± 37 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	48	93	60	11	5
		domContentLoaded	358	683	591	78	38
		load	359	684	592	78	37
		domInteractive	358	683	590	78	38

[Gudahtt]

LGTM!

This is a good case-study for our future controller messaging improvements.