fix stale selectedNetworkClientId by converting to function pattern #6312
Conversation
salimtb
force-pushed
the
fix/add-selected-chain-as-function
branch
from
639453d to
09aab69
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
There was a problem hiding this comment.
I've tested with alongside the mobile code and it looks good to me. But to understand this edge case better, when you say the network client ID "could become stale and frozen after controller reinitialization" can you give an example of how this would happen? The network client ID is always refreshed in the networkDidChange listener so I don't see it becoming stale there.
| state?: Partial<EarnControllerState>; | ||
| addTransactionFn: typeof TransactionController.prototype.addTransaction; | ||
| selectedNetworkClientId: string; | ||
| selectedNetworkClientId: () => string; |
There was a problem hiding this comment.
Hmm, this is not something we typically have to do in any other controller, so this is surprising to me that we have to do this. In your PR description you mention that it is necessary after the controller is reinitialized. When would this occur? I know that the Engine gets reinitialized in mobile in certain cases but usually all controllers get reinitialized when this happens and so the NetworkController should be up to date. (Edit: I see that @ameliejyc expressed some similar concerns.)
If we still think this is necessary, have you considered moving the asynchronous operations in this controller to an init function? You should then be able to get the selectedNetworkClientId from NetworkController by going through its state.
There was a problem hiding this comment.
@mcmire the main issue was race conditions, but you're right moving to the init function should resolve it. I’ll proceed with this approach instead on the client side and close this PR
There was a problem hiding this comment.
or maybe you mean init function inside the EarnController ?
There was a problem hiding this comment.
Either an init method in EarnController or using the init pattern on the client side should work, whichever makes sense.
|
The new init approach should resolve the race conditions on the client side, so I’m closing this PR. |
Explanation
Problem
The selectedNetworkClientId parameter was being passed as a static string value, which could become stale and frozen after controller reinitialization. This led to the controller potentially using outdated network client IDs, especially during network switches or controller restarts.
Solution
Controller Changes:
Test Updates:
References
Checklist