Internal improvement: Improve network check #2510
Conversation
| networks[network].networkCheckTimeout || DEFAULT_NETWORK_CHECK_TIMEOUT; | ||
| } else { | ||
| networkCheckTimeout = DEFAULT_NETWORK_CHECK_TIMEOUT; | ||
| } | ||
| const provider = this.getProvider(options); |
There was a problem hiding this comment.
still think calling getProvider and instantiating a new InterfaceAdapter/Web3Shim here is redundant, esp considering the rest of Environment.detect uses the already instantiated adapter/shim in the helpers.
There was a problem hiding this comment.
One thing that using getProvider gives us is more flexibility with what options can be. Not using it to get the provider makes it so that the provider property will need to be a provider and it will not accept a host and port 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.
The getter for config.provider will already return Provider.create which calls getProvider & then returns a normalized Provider. This also happens right before the testConnection call in Environment.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.
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. The getProvider 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.
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.
Co-Authored-By: Cruz Molina <[email protected]>
|
@CruzMolina I committed your suggestion for the InterfaceAdapter. |
Due to some awkward logging issues, it was decided to set a timeout default of 5000 ms for waiting on a response from the network while attempting to confirm that the connection is valid. It was also decided to get rid of the "Testing provider" logging as this will get run whenever
Environment.detectis called.Users will now be able to configure this timeout length as well by editing the
networks.<networkName>.networkCheckTimeoutproperty.