PollingController: add option to poll by blockTracker #3623
Conversation
adonesky1
force-pushed
the
add-block-tracker-polling
branch
4 times, most recently
from
4d19a0b
to
04526f2
Compare
adonesky1
force-pushed
the
add-block-tracker-polling
branch
from
04526f2
to
5ac05de
Compare
adonesky1
force-pushed
the
add-block-tracker-polling
branch
from
d73e4b4
to
60f9c75
Compare
adonesky1
requested review from
shanejonas,
jiexi,
BelfordZ and
matthewwalsh0
|
It's almost like we need another abstraction that handles just the idea of polling tokens, starting, and stopping. And two implementations of that for Polling by setTimeout and polling by subscription with listener |
adonesky1
force-pushed
the
add-block-tracker-polling
branch
from
195a836
to
ac73471
Compare
I agree this would be a more elegant solution. Not sure if we need this to be flexible to more ways of polling. All a question of time and effort imo. |
| networkClientId, | ||
| options, | ||
| ); | ||
| blockTracker.addListener('latest', updateOnNewBlock); |
There was a problem hiding this comment.
As discussed, do we want a mutex here to ensure executions don't overlap if you have slow asynchronous work inside _executePoll?
Or alternatively we could chain blockTracker.once but that may risk missing blocks which could be undesirable.
Or alternatively, we could put the burden of a mutex on the child controller inside their _executePoll?
| } | ||
|
|
||
| setPollWithBlockTracker( | ||
| getNetworkClientById: (networkClientId: NetworkClientId) => NetworkClient, |
There was a problem hiding this comment.
I wasn't sure if this argument would be best as a ControllerMessenger as this (Network)PollingController can know how to get a network client given an ID, plus it encourages messenger adoption and avoids each child controller having to define this function by binding it to their messenger for example.
But I suppose the counter-argument is that it would require all child controllers to be BaseController2 or have a messenger.
| clearTimeout(this.#intervalIds[key]); | ||
| delete this.#intervalIds[key]; | ||
| // if applicable stop polling on a static interval | ||
| if (this.#intervalIds[key]) { |
There was a problem hiding this comment.
Very minor, but for the sake of the size of this method, I wonder if it's worth moving each of these blocks to private methods like removeExistingTimeout and removeExistingListener?
|
I agree with @jiexi. I am hesitant accepting this as I am a bit uncomfortable with mashing two styles of polling into one controller. I don't think this extends the API in an intuitive way, and it makes the code more difficult to comprehend. Since there is no state in PollingController, it feels like this would be a perfect opportunity to refactor the existing code to create "poller" objects and make use of the strategy pattern to select which one to use at runtime. Is there any other solution that the Confirmation Systems can use in the meantime? |
Ready for review: **I will add changelog entries when/if we decide to roll with this approach.** Alternative approach to refactoring the PollingController per this [feedback](#3623 (comment)) on [PollingTracker enhancement work](#3623)
|
Superceded by #3636 |
Addresses: https://github.com/MetaMask/MetaMask-planning/issues/1788
@matthewwalsh0 and the Confirmation Systems Team is hoping to use the
PollingControllerfor a newUserOperationControllerbut are needing to tie polling intervals to block times rather than a static interval length. This is achievable using the vanilla blockTracker and adding callbacks to its listeners but we will want to multiplex it for multichain and it so it makes sense to do so using thePollingControllerabstraction since they are basically doing the same thing but just using a different source for the interval of execution.