Skip to content

Conversation

@nicosantangelo
Copy link
Contributor

@nicosantangelo nicosantangelo commented May 15, 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.

Extend providers

This is a first implementation to allow for the provider to be wrapped and extended with extra functionality.
The DSL mimics the way DSL extendConfig and extendEnvironment work, but it allows for an async wrapping to be made.

Hardhat will "compile" the provider into hre.network.provider from the combination of all extender functions it finds, in order.
The API looks like this:

extendProvider(async (provider: EIP1193Provider) => {
  const wrappedProvider = new ExtendedProvider(provider);
  await wrappedProvider.doSomething();
  return wrappedProvider;
});

Considerations

The new composed provider will run after the first call to request is made, unlike before, where it was built when first accessed (lazyObject). Because of this it was (currently) decided that any call to a sync method, i.e. EventEmitter methods, will throw informing the user that she needs to call request first.
This should be documented.

⚠️ Problem with hardhat-ethers

Currently, we have a problem with hardhat-ethers regarding this new implementation. hardhat-ethers extends the environment and hooks two events to the provider by calling createProviderProxy, here.
The provider will now be a LazyInitializationProvider which causes that line to throw because .request() hasn't been called yet

It's a bit tricky to fix, the options I have in mind are:

  • Having an implementation that doesn't throw on EventEmitter calls if .request() hasn't been called. This will work but doing a full implementation might be tricky. For example, should we support setMaxListeners? and should we delete listeners when .off() is called? Maybe we could have a private EventEmitter and then just move the listeners by looping though Object.keys(privateEmitter.eventNames())
  • Expose a public async .init() method on our providers that can be called to force what .request() does underneath. This will make either EthereumProvider or EIP1193Provider muddy in my opinion
  • Use an async extendProvider method to extend the ethers provider instead of doing it on extendEnvironment. This sounds good on paper but it poses two problems. First what method to call to initialize the provider, second that hre.ethers is a lazyObject and this would initialize it immediately

I think the first option is the best one but I'm not completely sure (and I might be missing some other easier version!), I've implemented it locally already and seems to work well

TODO

  • Automated tests
  • Write documentation
  • Consider making ProviderWrapper part of the public API. Maybe best for a different PR?

@changeset-bot
Copy link

changeset-bot bot commented May 15, 2023

🦋 Changeset detected

Latest commit: a60b1cd

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

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

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 May 15, 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 26, 2023 6:31pm
hardhat-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 26, 2023 6:31pm

@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 15, 2023 18:09 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 15, 2023 18:09 Inactive
@fvictorio
Copy link
Member

Use an async extendProvider method to extend the ethers provider instead of doing it on extendEnvironment. This sounds good on paper but it poses two problems. First what method to call to initialize the provider, second that hre.ethers is a lazyObject and this would initialize it immediately

I think we can rule out this option because the provider created in hardhat-ethers is in hre.ethers.provider, but extendProvider will affect hre.network.provider (the provider that is always available, no matter which plugins you are using).

Expose a public async .init() method on our providers that can be called to force what .request() does underneath. This will make either EthereumProvider or EIP1193Provider muddy in my opinion

Agree that it's not worth it, especially because, worst case, you can just use a dummy request to trigger that.

Having an implementation that doesn't throw on EventEmitter calls if .request() hasn't been called. This will work but doing a full implementation might be tricky.

What if we don't do a full implementation, and only implement the minimal things we need to prevent breaking hardhat-ethers? I know it doesn't feel great, but it seems like the most reasonable option. This would mean only implementing the .on callback. I would even be fine with a super restrictive implementation (e.g., a given event can only be registered once, if you register a second listener it throws).

@fvictorio
Copy link
Member

Also, I think that's the only option that doesn't need a change in hardhat-ethers. Otherwise, people upgrading Hardhat but not the plugin would get an error.

@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 16, 2023 10:37 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 16, 2023 10:38 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 16, 2023 10:41 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 16, 2023 10:42 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 16, 2023 10:45 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 16, 2023 10:46 Inactive
): void;
}

export type ProviderFactory = () => Promise<EthereumProvider>;
Copy link
Member

Choose a reason for hiding this comment

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

One convention we have is that anything not under src/internal is public. So this is (technically) adding a type to our public API. I don't mind if there's a good reason to do it, but I'm not sure if it makes sense to expose this type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think that even thought this looks like an innocuous enough type, there's no real reason to make it public. Maybe if the plan was to make LazyInitializationProvider a core functionality (and to make it public) it'd make more sense.

Would it make sense to move the type to src/internal/core/providers/lazy-initialization.ts? where LazyInitializationProvider lives

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that makes sense. I'm also fine with just inlining this type, since it's short enough. Whatever you prefer!

@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 16, 2023 15:48 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 16, 2023 15:49 Inactive
@nicosantangelo nicosantangelo force-pushed the feat/extend-providers branch from b1b20b5 to 8c4ea0e Compare May 16, 2023 15:51
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 16, 2023 15:51 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 16, 2023 15:52 Inactive
@nicosantangelo nicosantangelo force-pushed the feat/extend-providers branch from 8c4ea0e to f245b7c Compare May 16, 2023 15:52
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 16, 2023 15:53 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 16, 2023 15:54 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 16, 2023 15:56 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 16, 2023 15:57 Inactive
@nicosantangelo
Copy link
Contributor Author

What if we don't do a full implementation, and only implement the minimal things we need to prevent breaking hardhat-ethers? I know it doesn't feel great, but it seems like the most reasonable option. This would mean only implementing the .on callback. I would even be fine with a super restrictive implementation (e.g., a given event can only be registered once, if you register a second listener it throws).
Also, I think that's the only option that doesn't need a change in hardhat-ethers. Otherwise, people upgrading Hardhat but not the plugin would get an error.

Agree! the second point most of all I think makes it the better option by far.
What do you think of this implementation. It doesn't need the error we created earlier and allows for all EventEmitter methods to be called without initialization moving them over from the _emmiter to the provider after _initProvider

Now I'm writing this, releasing some memory and making _emitter undefined after might be useful but a bit tricky in terms of typings. If you think it's a good idea we can do that too!

@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 16, 2023 16:08 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 16, 2023 16:08 Inactive
@fvictorio
Copy link
Member

What do you think of this implementation.

I like it. Just a minor comment: we try to avoid using non-null assertinos (the ! operator). But I think that's easily fixable if this._initProvider returns the provider (maybe we should rename it to this._getOrInitProvider or something like that).

@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 23, 2023 16:47 Inactive
@nicosantangelo nicosantangelo force-pushed the feat/extend-providers branch from 44884a9 to 6362de0 Compare May 23, 2023 16:48
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 23, 2023 16:48 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 23, 2023 16:49 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 23, 2023 21:45 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 23, 2023 21:45 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 24, 2023 16:26 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 24, 2023 16:26 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 24, 2023 16:46 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 24, 2023 16:46 Inactive
/**
* A class that delays the (async) creation of its internal provider until the first call
* to a JSON RPC method via request/send/sendAsync.
* Trying to use the EventEmitter API without calling request first (initializing the provider)
Copy link
Member

Choose a reason for hiding this comment

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

I think this line is out of date now, isn't it?

@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 24, 2023 18:34 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 24, 2023 18:34 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 25, 2023 12:05 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 25, 2023 12:05 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat-storybook May 26, 2023 18:31 Inactive
@vercel vercel bot temporarily deployed to Preview – hardhat May 26, 2023 18:31 Inactive
@fvictorio
Copy link
Member

Closing in favor of #4008, which includes these changes.

@fvictorio fvictorio closed this Jun 16, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2023
@fvictorio fvictorio deleted the feat/extend-providers branch March 7, 2024 15:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants