Hardhat network configuration should not contain url property #1118
Conversation
Modified conditional clause for disallowing undefined url property of Hardhat network at the networks configuration object
| @@ -225,7 +225,7 @@ export function getValidationErrors(config: any): string[] { | |||
| if (config !== undefined && typeof config.networks === "object") { | |||
| const hardhatNetwork = config.networks[HARDHAT_NETWORK_NAME]; | |||
| if (hardhatNetwork !== undefined) { | |||
| if (hardhatNetwork.url !== undefined) { | |||
| if (hardhatNetwork.hasOwnProperty("url")) { | |||
There was a problem hiding this comment.
We usually use "url" in hardhatNetwork for this. I know there's a subtle difference between the two things in javascript, but I would do it that way just for consistency.
There was a problem hiding this comment.
I understand @fvictorio , from what you specify it should be valid to have a url property under the hardhat network object with value equal to undefined. Would it be correct?
There was a problem hiding this comment.
Hmm, no, it wouldn't. But (AFAIK) the only difference between the idiom I suggested and the one you used is that hasOwnProperty doesn't check the prototype chain, but that doesn't really matter in this scenario, that's why I think consistency is more important here.
There was a problem hiding this comment.
What any other clause could be used instead to check if url property is present or not? (Even if it is set to a value or as undefined).
There was a problem hiding this comment.
I think I'm not explaining myself correctly 😅 All I'm saying is that I'd rather do
if ("url" in hardhatNetwork) {instead of
if (hardhatNetwork.hasOwnProperty("url")) {They are very similar, and in many cases the latter one is the one you want. Since in this case they are pretty much the same, I'd use the first one since it's the one we use in the rest of the codebase.
There was a problem hiding this comment.
I understand now, thank you for your efforts on this PR review as I want to be an active contributor of this repository. I would do the changes adding a new commit to this PR and would ensure that new tests continue passing.
In this way, url parameter could not be a property under hardhat object.
There was a problem hiding this comment.
My 2cents to avoid future confusion:
In this case we do want to use in. If we were to use hasOwnProperty to validate the absence of a property, all the consumers of the config should be aware of this, and use the config's own properties instead of just doing config.value.
Comments have been taken into account
|
I uploaded the changes requested in order to comply with coding standards for this project. Even more, a validation conditional clause has been added as well in order to ensure validity of the |
|
Thanks for submitting this PR, @sisco0. It looks good now. |
Closes #1051.
This PR modified the configuration validator code in order to disallow
urlproperty of thehardhatobject. This issue was caused because the modified conditional clause was testingurlagainstundefined, which is a falsy value, but the property could be present after all. In order not to tackle with validations against this property in a subsequent step of the execution (which indeed is the purpose of the configuration validator) this conditional clause now uses.hasOwnPropertyinstead of comparing the value withundefined, where this property could be present with an undefined value.Test cases have been added as well for the case of
hardhatobject containing anurlproperty with value equal or not equal toundefined.I am happy to bring my efforts to this project by this first contribution and want to thank @alcuadrado for his accompaniment through this task accomplishment.