Skip to content

Switch gas price estimation in swaps to metaswap-api /gasPrices#9599

Merged
danjm merged 1 commit intodevelopfrom
etherscan-api-swaps-gasprice
Nov 4, 2020
Merged

Switch gas price estimation in swaps to metaswap-api /gasPrices#9599
danjm merged 1 commit intodevelopfrom
etherscan-api-swaps-gasprice

Conversation

@danjm
Copy link
Copy Markdown
Contributor

@danjm danjm commented Oct 14, 2020 •
edited
Loading

This PR updates the gas price estimation api for swaps. We now use the the metaswap-api /gasPrices endpoint.

The follow changes are included:

  • If the gas customization modal props include a truthy hideAdvancedTimeEstimates boolean, then the gas price chart and the time estimate on the advanced tab will be hidden. This is utilized in swaps.
  • If the gas customization modal props include a truthy noFetchOnMount boolean, then the gas customization modal component will not dispatch gas duck actions to fetch new gas prices on mount

Here is a demo:

etherscan-gas-api

@danjm danjm requested a review from a team as a code owner October 14, 2020 11:38
@danjm danjm requested a review from kumavis October 14, 2020 11:38
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 14, 2020

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Oct 14, 2020

Builds ready [b1e2891]
Page Load Metrics (423 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint288138115
domContentLoaded25461042112560
load25561142312560
domInteractive25460942112560

@danjm danjm mentioned this pull request Oct 14, 2020
Merged
@danjm danjm force-pushed the etherscan-api-swaps-gasprice branch from b1e2891 to 8a23996 Compare October 15, 2020 06:21
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Oct 15, 2020

Builds ready [8a23996]
Page Load Metrics (444 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint298543178
domContentLoaded30463644213163
load30563744413163
domInteractive30363544213163

Gudahtt
Gudahtt reviewed Oct 15, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider creating a separate gas customization modal for this, instead of re-using this one? That is what we had discussed doing when you brought this up on a call. There isn't a whole lot here being used - just a the layout and the two inputs. Meanwhile there is over a page worth of data being pulled in in the container for this component, none of which is relevant here. This component really wasn't meant for this.

Gudahtt
Gudahtt reviewed Oct 15, 2020
View reviewed changes
@danjm
Copy link
Copy Markdown
Contributor Author

danjm commented Oct 16, 2020

I just pushed a significant overhaul of this PR that creates an entirely new modal component for use just in swaps. It is a copy of existing behaviour. There are some improvements that have been discussed (e.g. warning on setting gas price too low, fetching gas price at an interval) that can be added in future PRs.

@danjm danjm force-pushed the etherscan-api-swaps-gasprice branch from e7d578d to 1f95d2f Compare October 16, 2020 09:57
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Oct 16, 2020

Builds ready [1f95d2f]
Page Load Metrics (469 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31123532512
domContentLoaded29986246816077
load30186346916077
domInteractive29986146716077

@danjm danjm mentioned this pull request Oct 16, 2020
Merged
@Gudahtt Gudahtt mentioned this pull request Oct 21, 2020
Merged
@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Oct 21, 2020

This needs to be rebased - there are a number of conflicts to resolve

@danjm danjm force-pushed the etherscan-api-swaps-gasprice branch 2 times, most recently from e7edabe to 64d27e8 Compare October 22, 2020 04:48
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Oct 22, 2020

Builds ready [fd84b40]
Page Load Metrics (520 ± 94 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3098572110
domContentLoaded325103651819694
load326103752019694
domInteractive325103651819694

@danjm
Copy link
Copy Markdown
Contributor Author

danjm commented Oct 22, 2020

@Gudahtt This and the dependent #9600 have been rebased

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Oct 22, 2020

Builds ready [4eb6146]
Page Load Metrics (465 ± 74 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint307739105
domContentLoaded30893446415474
load30993646515474
domInteractive30793446415474

Gudahtt
Gudahtt reviewed Oct 22, 2020
View reviewed changes
Gudahtt
Gudahtt reviewed Oct 22, 2020
View reviewed changes
Gudahtt
Gudahtt reviewed Oct 22, 2020
View reviewed changes
danjm
danjm commented Oct 22, 2020
View reviewed changes
danjm
danjm commented Oct 22, 2020
View reviewed changes
Gudahtt
Gudahtt reviewed Oct 22, 2020
View reviewed changes
danjm
danjm commented Oct 22, 2020
View reviewed changes
Gudahtt
Gudahtt reviewed Oct 23, 2020
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Oct 23, 2020

Builds ready [fca101c]
Page Load Metrics (381 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint309641147
domContentLoaded3066623809947
load3076643819947
domInteractive3066623799947

This was referenced Oct 25, 2020
Open
Closed
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Oct 25, 2020

Builds ready [f16e669]
Page Load Metrics (456 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint299741199
domContentLoaded29762745412660
load29862945612660
domInteractive29662745412660

@danjm danjm mentioned this pull request Oct 25, 2020
Open
Gudahtt
Gudahtt reviewed Oct 26, 2020
View reviewed changes
Gudahtt
Gudahtt reviewed Oct 26, 2020
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Oct 26, 2020

Builds ready [9517b6a]
Page Load Metrics (510 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32108562311
domContentLoaded32692350816177
load32892851016177
domInteractive32692350816177

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Oct 26, 2020

Builds ready [0d43898]
Page Load Metrics (389 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29433542
domContentLoaded29862738711455
load30062838911455
domInteractive29862638711455

@danjm danjm force-pushed the etherscan-api-swaps-gasprice branch from 0d43898 to 17decb1 Compare October 29, 2020 15:58
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Oct 29, 2020

Builds ready [17decb1]
Page Load Metrics (482 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint309545199
domContentLoaded25773348015273
load25873448215273
domInteractive25673348015273

@danjm danjm added this to the v8.1.next? milestone Nov 2, 2020
@danjm
Copy link
Copy Markdown
Contributor Author

danjm commented Nov 2, 2020

@Gudahtt Anything else you'd like to see addressed before we merge this PR?

Gudahtt
Gudahtt previously approved these changes Nov 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@danjm danjm force-pushed the etherscan-api-swaps-gasprice branch from 17decb1 to a417a68 Compare November 3, 2020 16:31
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 3, 2020

Builds ready [a417a68]
Page Load Metrics (455 ± 70 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint318341147
domContentLoaded24567045314670
load24767245514670
domInteractive24566945314670

@danjm
Switch gas price estimation in swaps to metaswap-api /gasPrices
2630068 
Adds swaps-gas-customization-modal and utilize in swaps

Remove swaps specific code from gas-modal-page-container/

Remove slow estimate data from swaps-gas-customization-modal.container

Use average as lower safe price limit in swaps-gas-customization-modal

Lint fix

Fix up unit tests

Update ui/app/ducks/swaps/swaps.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Remove stale properties from gas-modal-page-container.component.js

Replace use of isCustomPrice safe with isCustomSwapsGasPriceSafe, in swaps-gas-customization-modal

Remove use of averageIsSafe in isCustomPriceSafe function

Stop calling resetCustomGasState in swaps

Refactor 'setter' type actions and creators to 'event based', for swaps slice custom gas logic

Replace use of advanced-tab-content.component with advanceGasInputs in swaps gas customization component

Add validation for the gasPrices endpoint

swaps custom gas price should be considered safe if >= to average

Update renderDataSummary unit test

Lint fix

Remove customOnHideOpts for swapsGasCustomizationModal in modal.js

Better handling for swaps gas price loading and failure states

Improve semantics: isCustomSwapsGasPriceSafe renamed to isCustomSwapsGasPriceUnSafe

Mutate state directly in swaps gas slice reducer

Remove unused params

More reliable tracking of speed setting for Gas Fees Changed metrics event

Lint fix

Throw error when fetchSwapsGasPrices response is invalid

add disableSave and customTotalSupplement to swaps-gas-customization container return

Update ui/app/ducks/swaps/swaps.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Improve error handling in fetchMetaSwapsGasPriceEstimates

Remove metricsEvent from swaps-gas-customization-modal context

Base check of gas speed type in swaps-gas-customization-modal on gasEstimateType

Improve naming of variable and functions use to set customPriceIsSafe prop of AdvancedGasInputs in swaps-gas-customization-modal

Simplify sinon spy/stub code in gas-price-button-group-component.test.js

Remove unnecessary getSwapsFallbackGasPrice call in swaps-gas-customization-modal

Remove use of getSwapsTradeTxParams and clean up related gas price logic in swaps

Improve validator of SWAP_GAS_PRICE_VALIDATOR

Ensure default tradeValue
@danjm danjm force-pushed the etherscan-api-swaps-gasprice branch from a417a68 to 2630068 Compare November 4, 2020 02:43
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 4, 2020

Builds ready [2630068]
Page Load Metrics (462 ± 75 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint299340189
domContentLoaded25172746115675
load25272946215675
domInteractive25072746015675

@danjm danjm merged commit a0d7c71 into develop Nov 4, 2020
@danjm danjm deleted the etherscan-api-swaps-gasprice branch November 4, 2020 16:14
@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@kumavis kumavis Awaiting requested review from kumavis

Assignees

@Gudahtt Gudahtt

Labels

area-swaps

Projects

None yet

Milestone

v8.1.next?

Development

Successfully merging this pull request may close these issues.

5 participants

@danjm @metamaskbot @Gudahtt @brad-decker @jacobc-eth