fix(assets-controllers): Prevent overlapping token rate updates #3635
Conversation
Gudahtt
force-pushed
the
prevent-overlapping-token-rate-updates
branch
2 times, most recently
from
ee460b7
to
d56d67b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Gudahtt
force-pushed
the
prevent-overlapping-token-rate-updates
branch
from
d56d67b
to
8dd58e7
Compare
| ); | ||
|
|
||
| if (suppressUnhandledRejection) { | ||
| promise.catch((_error) => { |
There was a problem hiding this comment.
This is how I resolved the test failure on this branch previously (I accidentally squashed the commit).
This PR introduces a deferred Promise just in case there is an overlapping request. If not, the deferred promise is not needed. This is problematic when a request fails that has no overlap, as Node sees this as an unhandled Promise rejection and possible mistake. Easily resolved by introducing a catch block for the deferred promise before clearing it though, to suppress that warning
There was a problem hiding this comment.
in the case Promise.withResolvers is approved as is in the current stage would this be an issue? is this something we should report as feedback?
There was a problem hiding this comment.
Thanks for linking, that is a pretty neat spec! Looking forward to being able to use that.
From looking at the spec and the issues/PRs, it looks like there is no mention of changing the "unhandled promise" handling. TBH I'm not even sure if Node.js and browsers are aligned in how that's handled.
Likely they'd consider such a change as out-of-scope for that proposal; not all deferred Promises are "optional", and this case where a promise might be unresolved technically applies to non-deferred Promises as well. Seems like a separate problem.
There was a problem hiding this comment.
I see, so it seems like even if withResolvers makes it to the final spec we would still need to wrap it and add the catch handler. I would have loved this to be easily swappable with the official implementation.
I see there's a TODO comment to move this to utils, is there a reason not to do it now?
There was a problem hiding this comment.
omg it just went to stage 4 today, what a coincidence
There was a problem hiding this comment.
I see there's a TODO comment to move this to utils, is there a reason not to do it now?
Yep, this is part of a set of changes that we're aiming to get included in a release candidate ASAP for release in the first week of January, so I was looking for shortcuts. I'll set a reminder for myself to migrate this
Gudahtt
force-pushed
the
prevent-overlapping-token-rate-updates
branch
from
5c1e57f
to
19be290
Compare
Gudahtt
force-pushed
the
prevent-overlapping-token-rate-updates
branch
2 times, most recently
from
d4e77d1
to
8f62ac4
Compare
Gudahtt
force-pushed
the
prevent-overlapping-token-rate-updates
branch
from
8f62ac4
to
e0527f1
Compare
Previously it was possible for two redundant token rate updates to be ongoing at the same time. This is non-optimal for performance reasons, and because the answer might change between the two requests and get persisted in the wrong order. We now guard against this by storing in-progress updates as a private instance variable. Any redundant calls will wait on the in-progress call to finish, then return as a no-op. Fixes #3606
Gudahtt
force-pushed
the
prevent-overlapping-token-rate-updates
branch
from
e0527f1
to
b8f83ba
Compare
…om utils (#3769) ## Explanation The `createDeferredPromise` function that had been defined in the `TokenRatesController` has been replaced by the function with the same name from the `@metamask/utils` package. It behaves identically. The `@metamask/utils` package has been updated from `^8.2.0` to `^8.3.0` in all packages because the `createDeferredPromise` function was added in `v8.3.0`. ## References This is a follow-up to #3635. The function was temporarily inlined in that PR, but the plan was always to ship it as part of utils. This was done now because it helps somewhat with #3763, but it's not a complete blocker. ## Changelog This change entry applies to all packages in the diff for this PR. #### Changed - Bump `@metamask/utils` to `^8.3.0` ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate
Explanation
Previously it was possible for two redundant token rate updates to be ongoing at the same time. This is non-optimal for performance reasons, and because the answer might change between the two requests and get persisted in the wrong order.
We now guard against this by storing in-progress updates as a private instance variable. Any redundant calls will wait on the in-progress call to finish, then return as a no-op.
References
Fixes #3606
Changelog
@metamask/assets-controllers-Fixed: Prevent overlapping token rate updates
Checklist