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

Fix infura client chain ID #8629

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Fix infura client chain ID #8629

merged 1 commit into from
Dec 7, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented May 19, 2020

Our eth_chainId implementation in createInfuraClient.js declared its chain ID values inline, and they were incorrectly 0-padded. This PR fixes this by using the network enums for both net_version and eth_chainId.

@rekmarks rekmarks requested a review from a team May 19, 2020 18:28
@rekmarks rekmarks force-pushed the fix-infura-client-chainId branch from 1a993bd to caad6fc Compare May 20, 2020 16:07
@rekmarks rekmarks marked this pull request as ready for review May 20, 2020 16:08
@Gudahtt
Copy link
Member

Gudahtt commented May 20, 2020

This is a breaking change. I'm not sure we should include this in v8 without any notice.

@metamaskbot
Copy link
Collaborator

Builds ready [caad6fc]
Page Load Metrics (622 ± 61 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3110644178
domContentLoaded37282862012761
load37483062212761
domInteractive37282762012761

@rekmarks
Copy link
Member Author

@Gudahtt yeah, I agree. We should discuss internally and circle back.

brad-decker
brad-decker previously approved these changes May 20, 2020
@brad-decker
Copy link
Contributor

approval was based on code check, missed the comments about breaking changes.

@rekmarks rekmarks dismissed brad-decker’s stale review May 20, 2020 16:47

We shouldn't merge this yet.

@rekmarks
Copy link
Member Author

@brad-decker no worries, we'll hold off for now!

@rekmarks rekmarks added the DO-NOT-MERGE Pull requests that should not be merged label May 22, 2020
@rekmarks rekmarks marked this pull request as draft June 22, 2020 02:39
@danjm
Copy link
Contributor

danjm commented Nov 2, 2020

@rekmarks I'm guessing this PR can be closed?

@rekmarks
Copy link
Member Author

rekmarks commented Nov 2, 2020

@danjm this is part of the November 16 breaking changes.

@rekmarks
Copy link
Member Author

rekmarks commented Nov 2, 2020

Which I will now create a label for.

@rekmarks rekmarks force-pushed the fix-infura-client-chainId branch from caad6fc to ba36c2d Compare December 2, 2020 21:08
@rekmarks rekmarks removed the DO-NOT-MERGE Pull requests that should not be merged label Dec 2, 2020
@rekmarks rekmarks requested a review from Gudahtt December 2, 2020 21:10
@rekmarks rekmarks marked this pull request as ready for review December 2, 2020 21:10
@metamaskbot
Copy link
Collaborator

Builds ready [ba36c2d]
Page Load Metrics (504 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3388582010
domContentLoaded30769250210350
load30869450410450
domInteractive30669250110350

@rekmarks
Copy link
Member Author

rekmarks commented Dec 5, 2020

@brad-decker this is ready for review!

@rekmarks rekmarks merged commit 4839e31 into develop Dec 7, 2020
@rekmarks rekmarks deleted the fix-infura-client-chainId branch December 7, 2020 19:39
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants