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

Remove blockscale, replace with ethgasstation #7059

Merged
merged 1 commit into from
Aug 22, 2019
Merged

Conversation

tmashuang
Copy link
Contributor

@tmashuang tmashuang commented Aug 22, 2019

Remove blockscale, replace with ethgasstation for gas estimation. Also, use safelow, average, and fast values instead of safelow, fast, fastest.

Closes #7040
Closes #7024

@metamaskbot
Copy link
Collaborator

Builds ready [f9bdc80]: chrome, firefox, edge, opera

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

Looks good. Is it necessary to change all of the test expectations?

@tmashuang
Copy link
Contributor Author

Is it necessary to change all of the test expectations?

Yes, since I changed the values of what our average and fast are in relation to the response. If it was just changing the url, I wouldn't have needed to change it.

{
'headers': {},
'referrer': 'https://dev.blockscale.net/api/',
'referrer': 'http://ethgasstation.info/json/',
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious to know why http is used here instead of https. I noticed other fetches to ethgasstation set this referrer as well.

Copy link
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.

Looks good!

The change from slow/fast/faster to slow/average/fast seems like a good idea. I went looking briefly for an indication of why fast/faster were used in the first place, but couldn't find any. I guess it may have been a mistake.

I did notice that the dev.blockscale.net URLs were still in the e2e and integration tests - seemingly only used in the fetch mocks. Each of those cases should be deleted, since we're not using blockscale anymore.

@danfinlay danfinlay merged commit fe2d053 into develop Aug 22, 2019
@whymarrh whymarrh deleted the Gas-Estimation-Api branch September 4, 2019 19:22
Gudahtt added a commit that referenced this pull request Dec 10, 2019
These fetch mocks have been unnecessary since #7059
Gudahtt added a commit that referenced this pull request Dec 10, 2019
These fetch mocks have been unnecessary since #7059
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.

Default gas price lower than slow processing time Investigate reports of low gas defaults
6 participants