This repository has been archived by the owner on Feb 26, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Internal improvement: Improve network check #2510
Merged
Merged
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
5847a03
Prepare testConnection method to accept config property and edit the …
eggplantzzz d1a391a
Add default time for networkCheckTimeout and use it in provider
eggplantzzz 58c594d
Allow prettier to reformat
eggplantzzz 995d585
Prevent 'undefined' errors when not passing property
eggplantzzz 80b0e0d
Pass the networkType to the Web3Shim
eggplantzzz f6b6d9d
Edit wording of error message
eggplantzzz 433cbb8
Update packages/provider/index.js
eggplantzzz 0497a70
Merge in develop branch
eggplantzzz 5a0be44
Resolve merge conflict
eggplantzzz d404911
Prevent trying to access undefined if networks is not provided
eggplantzzz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still think calling
getProvider
and instantiating a newInterfaceAdapter/Web3Shim
here is redundant, esp considering the rest ofEnvironment.detect
uses the already instantiated adapter/shim in the helpers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that using
getProvider
gives us is more flexibility with whatoptions
can be. Not using it to get the provider makes it so that theprovider
property will need to be a provider and it will not accept ahost
andport
property etc. I'm not sure what behavior we want for this method. I'm not sure I have a strong preference, any thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getter for
config.provider
will already return Provider.create which callsgetProvider
& then returns a normalized Provider. This also happens right before thetestConnection
call inEnvironment.detect
.It makes sense that
testConnection
should be testing an already normalized Provider.Also makes sense to DRY up the code in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I understand what you are saying that it should have already parsed and set a provider on the config object. However what I am pointing out is that
testConnection
is exposed in this package. ThegetProvider
method allows for the input to be a little more flexible.I don't know what you mean by DRY here because bye using the
getProvider
method you are re-using existing code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. I agree as an exposed pkg util func it's useful. It still feels repetitive to implement it in the
Environment.detect
flow.