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

Support hex and number net_version responses #19156

Merged
merged 1 commit into from
May 15, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented May 15, 2023

Explanation

Hex and number responses from the net_version request are now accepted. While most chains return decimal strings for this request, some chains were returning hex responses instead and failing our validation for this request (which was added in v10.30.0). This resolves that problem.

Support for number responses was added because it likely worked in older versions as well, so support is maintained to avoid similar problems.

Fixes #19151

Manual Testing Steps

  • Add The SKALE custom network using the information shown here: https://mainnet.skalenodes.com/#/chains/elated-tan-skat
  • Switch to this network, and see that it is able to successfully connect
    • Previously it would say "Loading" for some time, followed by an "Oops, something failed" message and a "Try again" button.

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@Gudahtt Gudahtt marked this pull request as ready for review May 15, 2023 20:33
@Gudahtt Gudahtt requested a review from a team as a code owner May 15, 2023 20:33
@Gudahtt Gudahtt requested a review from garrettbear May 15, 2023 20:33
@Gudahtt
Copy link
Member Author

Gudahtt commented May 15, 2023

Note that the lint failure appears to be on develop; presumably caused by a merge error in the most recent commit.

Edit: Fixed in this PR: #19157

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

One minor thing but makes sense to me regardless.

],
stubLookupNetworkWhileSetting: true,
});
const validNetworkIds = [12345, '12345', toHex(12345)];
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Is it worth adding a test for an invalid network ID? Could do this in a new PR as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thought, but we already covered this below (using the id invalid).

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.

LGTM

Hex and number responses from the `net_version` request are now
accepted. While most chains return decimal strings for this request,
some chains were returning hex responses instead and failing our
validation for this request (which was added in v10.30.0). This
resolves that problem.

Support for number responses was added because it likely worked in
older versions as well, so support is maintained to avoid similar
problems.

Fixes #19151
@Gudahtt Gudahtt force-pushed the support-hex-response-to-net_version branch from 5616f6b to fa4f45f Compare May 15, 2023 21:07
@kladkogex
Copy link

Thank you folks from SKALE!

@Gudahtt Gudahtt merged commit 05715dd into develop May 15, 2023
@Gudahtt Gudahtt deleted the support-hex-response-to-net_version branch May 15, 2023 21:26
@github-actions github-actions bot locked and limited conversation to collaborators May 15, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [fa4f45f]
Page Load Metrics (1672 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint971921262311
domContentLoaded1541178816456531
load1541183716726933
domInteractive1541178816456531
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 289 bytes
  • ui: 0 bytes
  • common: 0 bytes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Can not connect to the network
5 participants