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

fix: Workaround for ethers.js bug in webpack environments #538

Closed
wants to merge 3 commits into from

Conversation

joshdchang
Copy link

@joshdchang joshdchang commented Jul 16, 2024

Description

I was getting the following issue when trying to connect in a nodejs server-side webpack environment (in Next.js):

Error: could not detect network (event="noNetwork", code=NETWORK_ERROR, version=providers/5.7.2)

After doing some searching and testing, I figured out it was an bug from ethers.js with webpack. I tried the workaround from ethers-io/ethers.js#3536 (comment) and it fixed the issue. This PR only changes one line of code.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Try running it in Next.js server side with and without this change. You will see the difference.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@CLAassistant
Copy link

CLAassistant commented Jul 16, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@Ansonhkg Ansonhkg left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this!

Could you change the JsonRpcProvider to StaticJsonRpcProvider?

We have just got this PR up (#537) which could half to RPC request time by simply changing it to static

@Ansonhkg Ansonhkg requested a review from MaximusHaximus July 16, 2024 21:18
@joshdchang
Copy link
Author

Could you change the JsonRpcProvider to StaticJsonRpcProvider?

We have just got this PR up (#537) which could half to RPC request time by simply changing it to static

Sure let me try

@joshdchang
Copy link
Author

@Ansonhkg Okay I changed it to StaticJsonRpcProvider and it seems to work well.

@Ansonhkg Ansonhkg self-requested a review July 16, 2024 22:00
Copy link
Collaborator

@Ansonhkg Ansonhkg left a comment

Choose a reason for hiding this comment

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

By default, when you create a new provider, ethers.js performs an initial setup that involves fetching network information (such as the network name and chain ID). This setup ensures that the provider is correctly configured to interact with the specified network.

The skipFetchSetup option allows you to skip this initial network information fetch. This can be useful in scenarios where you want to delay the network setup or when you already have the necessary network information and want to avoid the additional request.

This LGTM. @MaximusHaximus WDYT?

@Ansonhkg Ansonhkg changed the title Workaround for ethers.js bug in webpack environments fix: Workaround for ethers.js bug in webpack environments Jul 17, 2024
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Thanks so much for the contribution @joshdchang :D We really appreciate it.

FYI, this change is a little bit different than you described @Ansonhkg -- this is literally controlling how fetch() is called rather than controlling whether chainId is checked (which was the difference between JSONRPCProvider and StaticJSONRPCProvider)

From the docs:

connection.skipFetchSetup ⇒ boolean

Normally a connection will specify the default values for a connection such as CORS-behavior and caching policy, to ensure compatibility across platforms. On some services, such as Cloudflare Workers, specifying any value (inclluding the default values) will cause failure. Setting this to true will prevent any values being passed to the underlying API.```

I think it should be fine to add this argument to the JSONRPCProvider construction, but we create these providers in a bunch of different places, so while this PR as currently written would fix using the contracts SDK directly, consumers of the SDK will still run into this error in other places in the SDK where we construct the JSONRPCProvider on their behalf - likely just a few minutes later down their exploration of the JS SDK.

For your reference, every place that we need the new argument added are the same exact lines that were changed from JSONRPCProvider to StaticJSONRPCProvider in #537, plus one additional place that I didn't change in that PR because we couldn't use the StaticJSONRPCProvider for generic chains where we wouldn't necessarily be able to skip the chainId pre-check behaviour here

Are you OK to make that change @joshdchang?

@joshdchang
Copy link
Author

Are you OK to make that change @joshdchang?

Hi, thanks for this! Do you want me to change it to StaticJSONRPCProvider in all the locations as well? This would overlap substantially with #537 and might lead to merge conflicts. Maybe this is best done in one PR?

@Ansonhkg Ansonhkg added Proposal good first issue Good for newcomers and removed Proposal labels Jul 24, 2024
@Ansonhkg Ansonhkg self-requested a review July 25, 2024 11:24
@MaximusHaximus
Copy link
Contributor

Are you OK to make that change @joshdchang?

Hi, thanks for this! Do you want me to change it to StaticJSONRPCProvider in all the locations as well? This would overlap substantially with #537 and might lead to merge conflicts. Maybe this is best done in one PR?

Sorry for the delayed response, and thanks so much for contributing!
We've landed the JsonStaticRPCProvider PR now, so you're GTG without needing to worry about conflicts :)

@FedericoAmura
Copy link
Contributor

Replacing this with #646 which has the changes needed on all the providers. Thanks @joshdchang !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants