-
-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bug that allowed for multiple fetch requests instantiate from getGasFeeEstimatesAndStartPolling #586
Conversation
src/gas/GasFeeController.test.ts
Outdated
@@ -74,6 +76,9 @@ describe('GasFeeController', () => { | |||
estimatedBaseFee: '28', | |||
}) | |||
.persist(); | |||
mockGasFeeRequest.on('request', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the best I could come up with for tracking number of requests. I look through nock documentation fairly extensively, tried a few other things, but this meets my exact need. Open to suggested alternative if there is a simpler way based on the nock api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked this up and it seems event is the way to go, however (nit) I'd change the event handler to a jest mock and use expect(handler).toHaveBeenCalledTimes
instead of a counter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/gas/GasFeeController.test.ts
Outdated
expect(result2).toStrictEqual(pollToken); | ||
}); | ||
|
||
it('should fetch new estimates if the poll tokens are cleared, and then should not make additional new fetches', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused on this one... I see it that it shouldn't make additional fetches on getGasFeeEstimatesAndStartPolling calls (until pollTokens are cleared again), but won't it make additional fetches on the polling interval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this comment can be clarified. It means to say that subsequent calls to getGasFeeEstimatesAndStartPolling should not trigger new fetches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adonesky1 updated the comment in the latest commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small question but otherwise code LGTM! (didn't test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
… while awaiting for a previous getGasFeeEstimatesAndStartPolling call
eee033f
to
5595d22
Compare
…GasFeeEstimatesAndStartPolling (#586) * Ensure that the gas fee controller does not make new network requests while awaiting for a previous getGasFeeEstimatesAndStartPolling call * Use a jest function for handling events from nock calls in GasFeeController.test.ts * Improve unit test description'
…GasFeeEstimatesAndStartPolling (#586) * Ensure that the gas fee controller does not make new network requests while awaiting for a previous getGasFeeEstimatesAndStartPolling call * Use a jest function for handling events from nock calls in GasFeeController.test.ts * Improve unit test description'
There was a bug in
getGasFeeEstimatesAndStartPolling
that could result in multiple fetch requests to the gas api being made, despite thepollTokens
related logic meant to prevent this.This PR fixes this by ensuring that the
pollTokens
are set before the asyncronous call. Tests are added that fail without this change and succeed with it.