Skip to content
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

Add options to GasFeeController fetchGasFeeEstimates #526

Merged
merged 2 commits into from
Jul 21, 2021

Conversation

wachunei
Copy link
Member

This PR adds an options object to fetchGasFeeEstimates and _fetchGasFeeEstimateData. Options include shouldUpdateState: boolean property to decide wether to update state or not, true by default.

@wachunei wachunei requested a review from a team as a code owner July 15, 2021 20:01
@Gudahtt
Copy link
Member

Gudahtt commented Jul 15, 2021

What is this for? I can't think of a situation where you'd want to keep the gas estimate data stale on purpose.

@wachunei
Copy link
Member Author

SwapsController needs estimate data every polling cycle, which is every ~45 seconds (or the value the quotes come with). At the same time GasFeeController is polling an updating its state, so there's a case the estimate data from the GasFeeController is updated when fetching the next SwapsController state, even if the current polling cycle from GasFeeController has not finished, and when it finishes it will update again.

This option is for SwapsController to fetch and set data to the state on the very first cycle only, from there on, GasFeeController will update its state with the polling.

@Gudahtt
Copy link
Member

Gudahtt commented Jul 15, 2021

Right but what negative consequence are we trying to avoid by not updating the state with the new gas estimates? Would it be bad if the gas estimates were updated slightly sooner because of the swaps controller fetching updated estimates?

@wachunei
Copy link
Member Author

The UI animates when the estimates update from the GasFeeController, this will result on the UI animating more than necessary.

@Gudahtt
Copy link
Member

Gudahtt commented Jul 15, 2021

Huh, OK. Well... this seems less than ideal. I see why this might be the simplest solution for now though.

Back when the swaps UI was being discussed, I had thought the plan was to control the gas estimate change animation in the UI to ensure nothing janky happened like successive refreshes. I don't know how it was implemented after though. The UI does seem like the better place to handle that UI problem.

@brad-decker
Copy link
Contributor

Any ideas on how we prevent a UI update from a state change that results in new values, sooner than expected? We can only prevent these animations if the underlying values don't change -- if they do there is no way to know if this occurred on a normal interval or because of a fetch like this, at least I don't think there is.

fWIW on mobile the animations are already being handled this way (triggered by an underlying value change)

@Gudahtt
Copy link
Member

Gudahtt commented Jul 16, 2021

Sure, depend on what we want. e.g. if we want to prevent the value from changing twice within a 10 second window, we can cache the value in the UI and start a timer, then update the value based on state only when the timer expires (and let it update immediately from that point forward, until the next update, at which point the timer starts again). Or if we want to prevent the gas price from changing within 10 seconds of the page loading, we can do that. Or both. Whatever design wants.

@wachunei wachunei merged commit 2cbfe52 into main Jul 21, 2021
@wachunei wachunei deleted the feature/gas-fee-controller-update-state branch July 21, 2021 20:49
@wachunei wachunei mentioned this pull request Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants