Fix another TokenRatesController event ordering bug
#1511
Conversation
When the network switches, the `TokenRatesController` was relying upon the combination of two state updates (from the network controller and the tokens controller), both of which were needed in order to ensure the controller configuration was valid. These events don't arrive at the same time, meaning that the controller was potentially making one invalid call each network switch. Additionally, there is a risk that the second event could result in no update (e.g. because the destination network isn't supported, or because there were no tokens), resulting in the data from the invalid call remaining in state. This has been prevented by updating the controller to use the `allTokens` and `allDetectedTokens` state rather than the `tokens` state. A network switch now triggers a single event, and the controller configuration never enters an invalid state.
Gudahtt
force-pushed
the
fix-another-token-rate-event-ordering-bug
branch
from
0a78d8d
to
1cc946f
Compare
| @@ -203,7 +214,9 @@ export class TokenRatesController extends BaseController< | |||
| interval: 3 * 60 * 1000, | |||
| nativeCurrency: initialTicker, | |||
| chainId: initialChainId, | |||
| tokens: [], | |||
| selectedAddress: initialSelectedAddress, | |||
| allTokens: {}, // TODO: initialize these correctly, maybe as part of BaseControllerV2 migration | |||
There was a problem hiding this comment.
I'm just now noticing that the only thing that TokenRatesController has in state is contractExchangeRates. Is config persisted to state and if not, shouldn't it at least be read-only? i.e. should allTokens and allDetectedTokens be a part of state too?
There was a problem hiding this comment.
Config and state are separate; config is not persisted to-disk.
I'm not sure I follow the question about whether it should be read-only; are you talking about config or state there? What exactly should we consider making read-only and why?
I don't understand why we'd move allTokens and allDetectedTokens into state; they're already in the state of the tokens controller. That would cause an overlap.
There was a problem hiding this comment.
By read-only, I mean that this property is only used to set the initial private state of the controller and is not changed. It seems that we are changing allTokens and allDetectedTokens throughout the lifetime of the controller, however, and that we'd want to instead use the values that come through in config to set private properties that then get changed.
Perhaps this pattern is not new, though, and config has been used as form of private state all along. I suppose it's not a big deal — this will go away when the controller is converted to BaseControllerV2 — I just didn't know if it was intentional or not.
There was a problem hiding this comment.
Indeed, this is how config has always worked unfortunately. Agreed that it's not good. That will be fixed with the BaseControllerV2 migration
* Fix another `TokenRatesController` event ordering bug When the network switches, the `TokenRatesController` was relying upon the combination of two state updates (from the network controller and the tokens controller), both of which were needed in order to ensure the controller configuration was valid. These events don't arrive at the same time, meaning that the controller was potentially making one invalid call each network switch. Additionally, there is a risk that the second event could result in no update (e.g. because the destination network isn't supported, or because there were no tokens), resulting in the data from the invalid call remaining in state. This has been prevented by updating the controller to use the `allTokens` and `allDetectedTokens` state rather than the `tokens` state. A network switch now triggers a single event, and the controller configuration never enters an invalid state. * Prevent update while polling inactive, and add tests
* Fix another `TokenRatesController` event ordering bug When the network switches, the `TokenRatesController` was relying upon the combination of two state updates (from the network controller and the tokens controller), both of which were needed in order to ensure the controller configuration was valid. These events don't arrive at the same time, meaning that the controller was potentially making one invalid call each network switch. Additionally, there is a risk that the second event could result in no update (e.g. because the destination network isn't supported, or because there were no tokens), resulting in the data from the invalid call remaining in state. This has been prevented by updating the controller to use the `allTokens` and `allDetectedTokens` state rather than the `tokens` state. A network switch now triggers a single event, and the controller configuration never enters an invalid state. * Prevent update while polling inactive, and add tests
Explanation
When the network switches, the
TokenRatesControllerwas relying upon the combination of two state updates (from the network controller and the tokens controller), both of which were needed in order to ensure the controller configuration was valid. These events don't arrive at the same time, meaning that the controller was potentially making one invalid call each network switch.Additionally, there is a risk that the second event could result in no update (e.g. because the destination network isn't supported, or because there were no tokens), resulting in the data from the invalid call remaining in state.
This has been prevented by updating the controller to use the
allTokensandallDetectedTokensstate rather than thetokensstate. A network switch now triggers a single event, and the controller configuration never enters an invalid state.References
This tangentially relates to #1466
Changelog
@metamask/assets-controllersTokenRatesControllernow requires two new constructor parameters:selectedAddressandonPreferencesStateChangeTokenRatesControllerconfiguration/state upon network switchingChecklist