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

#157 Add viem support and related tests #156

Closed

Conversation

muhammetaliakbay
Copy link
Contributor

@muhammetaliakbay muhammetaliakbay commented Oct 28, 2023

Related issue: #157

This PR

  • adds viem support and tests
  • upgrades typescript
  • replaces EGRDataCollectionProvider with SnifferProvider
  • integrates extendEnvironment to inject sniffer provider

@muhammetaliakbay muhammetaliakbay changed the title Add viem support and related tests #157 Add viem support and related tests Oct 28, 2023
@gionn
Copy link

gionn commented Dec 26, 2023

@cgewecke any feedback on this?

@cgewecke
Copy link
Owner

cgewecke commented Dec 28, 2023

@muhammetaliakbay @gionn Thanks so much for this and apologies for taking so long to review.

Could someone link to the relevant code at viem or hardhat where the freezing described below happens?

Injecting the modified provider in TASK_TEST_RUN_MOCHA_TESTS sub task is too late as viem reads and freezes the provider instance on the module load

I'm a little worried about configuring the provider for gas logging in an extendEnvironment hook because AFAIK.it will execute for every task hardhat runs. It might come into conflict with other plugins and is difficult to enable/disable conditionally.

That said there is also a new extendProvider hook available in more recent HH versions which seems to be designed for this kind of application. Does using that have same freezing problem?

(Will pull this PR down this week and take a closer look to see if there are any other issues...thanks for all the work you've already put into this).

@transmental
Copy link

pls merge this

@cgewecke cgewecke mentioned this pull request Jan 11, 2024
@cgewecke
Copy link
Owner

This work is being continued in #167 (@muhammetaliakbay's authorship is preserved there) ... am just trying to understand everything in the PR and seeing if a less extensive set of changes if possible to get viem support working. Leaving this open for now.

@cgewecke cgewecke changed the base branch from master to viem January 31, 2024 01:35
@cgewecke cgewecke self-requested a review January 31, 2024 02:09
Copy link
Owner

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

Thank you for preparing such a great set of tests for this - I would not have caught that there is a problem with transactions executed with the wallet clients.

Debugged that at Hardhat and think the problem is that in the extendEnviroment hook at the hardhat-viem plugin, the provider is passed directly as an argument to the viem wrapper methods on definition instead of being de-referenced dynamically when the methods are called. As such those methods never refer to the provider which is redefined in the test task here.

The main concern I have with the approach in this PR is that it uses extendEnvironment. As you note, the plugins have to be loaded in a specific sequence in hardhat.config.ts for them to work together...I think a more reliable way is to use Hardhat's new extendProvider hook, but that's a breaking change I'd like to introduce in 2.0

For the moment, I'm going to publish this branch at a special tag (viem), pin your original issue and direct people to this work-around.

@cgewecke cgewecke closed this Jan 31, 2024
@cgewecke cgewecke mentioned this pull request Feb 11, 2024
54 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants