Prevent redundant token rate updates #1512
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Gudahtt
force-pushed
the
fix-another-token-rate-event-ordering-bug
branch
from
0a78d8d
to
1cc946f
Compare
The `TokenRatesController` was updating token rates each time any state change event was recieved, even when the change was totally unrelated. The controller now ignores irrelevant state changes. Only relevant state changes will trigger token rate updates. Fixes #1466
Gudahtt
force-pushed
the
prevent-redundant-token-rate-lookups
branch
from
82ad946
to
a346aa6
Compare
| @@ -295,7 +295,7 @@ describe('TokenRatesController', () => { | |||
| expect(mock).not.toThrow(); | |||
| }); | |||
|
|
|||
| it('should update exchange rates when tokens change while polling is active', async () => { | |||
| it('should update exchange rates when tokens change', async () => { | |||
There was a problem hiding this comment.
It looks like we have a test for when allTokens changes, but what do you think about adding another one for when allDetectedTokens changes?
There was a problem hiding this comment.
We already have a test like this, though it's at the end of the test suite (the order doesn't make a great deal of sense; I think it's last because it was worked on last)
There was a problem hiding this comment.
Ah I see now.
In that case should we have a test that's the same as that test but verifies nothing happens when polling is disabled?
There was a problem hiding this comment.
Good catch. Since this is unrelated to the changes in this PR, I will address it in a follow-up PR to reorganize these tests and add few missing test cases
There was a problem hiding this comment.
This file has a test "should clear contractExchangeRates state when network is changed", which covers the case in which onNetworkStateChange is called with a different chain ID, but it seems like we might be missing a test for when this occurs but polling is inactive. Does it make sense to add this test?
There was a problem hiding this comment.
That sounds like "should not update exchange rates when ticker changes while polling is inactive"
There was a problem hiding this comment.
Oh, I guess that doesn't cover the "clearing" behavior, true. I'll look at adding a test for that
There was a problem hiding this comment.
The test "should not update exchange rates when ticker changes while polling is inactive" verifies that nothing happens when the ticker changes but polling is inactive, but I'm talking about when the chain ID changes but polling is inactive. Does that test already exist?
There was a problem hiding this comment.
The tests with "ticker" in the description actually test when the ticker and chain ID change. We don't have any tests for when they change independently.
There was a problem hiding this comment.
I can add tests for independent chain ID and ticker changes in a follow-up PR, along with the additional "clearing" test case
Missing test cases found in #1512 have been added.
Missing test cases found in #1512 have been added.
Missing test cases found in #1512 have been added.
Missing test cases found in #1512 have been added.
Missing test cases found in #1512 have been added.
Missing test cases found in #1512 have been added.
The `TokenRatesController` was updating token rates each time any state change event was recieved, even when the change was totally unrelated. The controller now ignores irrelevant state changes. Only relevant state changes will trigger token rate updates. Fixes #1466
Missing test cases found in #1512 have been added.
The `TokenRatesController` was updating token rates each time any state change event was recieved, even when the change was totally unrelated. The controller now ignores irrelevant state changes. Only relevant state changes will trigger token rate updates. Fixes #1466
Missing test cases found in #1512 have been added.
Explanation
The
TokenRatesControllerwas updating token rates each time any state change event was recieved, even when the change was totally unrelated.The controller now ignores irrelevant state changes. Only relevant state changes will trigger token rate updates.
References
Fixes #1466
Changelog
@metamask/assets-controllersTokenRateControllerfrom making redundant token rate updatesChecklist