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

Add config validation for empty network and forking url #4096

Merged
merged 5 commits into from
May 17, 2024

Conversation

kshyun28
Copy link
Contributor

@kshyun28 kshyun28 commented Jul 1, 2023

  • Because this PR includes a bug fix, relevant tests have been included.
  • Because this PR includes a new feature, the change was previously discussed on an Issue or with someone from the team.
  • I didn't do anything of this.

Considering the discussions in the related issue, this PR adds empty string validation such as:

  • empty strings ""
  • empty strings with at least 1 whitespace " "

for the following configs:

  • [CUSTOM_NETWORK].url
  • [HARDHAT_NETWORK_NAME].forking.url

To improve the error message, instead of using the default invalid error message, I also added an empty error message. This can be up for debate since we can just use invalid error message which makes the code simpler.

This closes #2730

Screenshot of sample config error
Hardhat empty url config error

@changeset-bot
Copy link

changeset-bot bot commented Jul 1, 2023

🦋 Changeset detected

Latest commit: e5b9101

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
hardhat Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jul 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 17, 2024 7:42pm
hardhat-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 17, 2024 7:42pm

@fvictorio
Copy link
Member

Thank you @kshyun28, this looks good! I just left a minor comment.

@fvictorio
Copy link
Member

Uhm, thinking about this a bit more, I'm afraid this change will break some existing projects that use an empty string as a placeholder, like this template.

This is not your fault, my initial description in the issue was a mistake 😞

Instead of that, I think we should validate the URL here:

(This is used both by normal networks and by the forking feature.)

This means adding a new general error here and throwing it there when url.trim() is empty. The error message should say something about trying to connect to an empty URL, and suggesting that this can be caused by an empty forking or network URL (we can't know for sure because it's a shared code).

I'm sorry for asking you to discard what you did so far, but I think doing this when the URL is actually used is the right thing to do here.

@kshyun28
Copy link
Contributor Author

kshyun28 commented Jul 7, 2023

Hello @fvictorio, I understand that if this implementation of the fix would break some existing projects, better not go with this approach.

And no worries, it's not really discarding since the issue is moving and allows us to see potential issues.

I'll try to reimplement the fixes in the HttpProvider. Actually that was the first thing that came to mind for me, but considered the config validation approach. At least we now know that config validation won't work. :)

Thanks for the guidance!

@alcuadrado alcuadrado merged commit 9e838b3 into NomicFoundation:main May 17, 2024
71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Better error message when URL is an empty string
3 participants