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.

Sign up for GitHub

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

Jump to bottom

network.js: convert rpc protocol to lower case. #4855

Merged
merged 1 commit into from
Jul 24, 2018
Merged

network.js: convert rpc protocol to lower case. #4855

merged 1 commit into from
Jul 24, 2018

Conversation

pinkiebell
Copy link
Contributor

@pinkiebell pinkiebell commented Jul 23, 2018 •
edited
Loading

Fixes #4253

@pinkiebell pinkiebell changed the title network.js: convert rpc protocol to lowercase. network.js: convert rpc protocol to lower case. Jul 23, 2018
@gitcoinbot gitcoinbot mentioned this pull request Jul 23, 2018
Closed
@whymarrh
Copy link
Contributor

Hey @pinkiebell, thanks for this! Two comments:

  1. Our test-e2e-beta-firefox CI check is currently a bit flaky, don't mind that failure. I don't think there's anything in here that should cause issues.
  2. I see you're using substring to parse the scheme out of the URL, would it make more sense to use the URL module require('url') instead? (I'll leave this inline as well.)

whymarrh
whymarrh reviewed Jul 24, 2018
View reviewed changes
app/scripts/controllers/network/network.js Outdated
@@ -155,6 +155,11 @@ module.exports = class NetworkController extends EventEmitter {
}

_configureStandardProvider ({ rpcUrl }) {
const colon = rpcUrl.indexOf(':')
if (colon !== -1) {
const proto = rpcUrl.substring(0, colon)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using indexOf and substring here, can we use the URL module to pick the scheme out of the URL. We should be able to create a URL instance and mutate the protocol.

@pinkiebell
Copy link
Contributor Author

@whymarrh Should be good now 👍

whymarrh
whymarrh approved these changes Jul 24, 2018
View reviewed changes
Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@whymarrh whymarrh merged commit 3667f3c into MetaMask:develop Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whymarrh whymarrh whymarrh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pinkiebell @whymarrh @bdresser