-
Notifications
You must be signed in to change notification settings - Fork 269
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
Prepare for next hardhat version #796
Conversation
env.network.provider.on(HARDHAT_NETWORK_RESET_EVENT, () => { | ||
api.attachToHardhatVM(env.network.provider); | ||
env.network.provider.on(HARDHAT_NETWORK_RESET_EVENT, async () => { | ||
await api.attachToHardhatVM(env.network.provider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super confident about this change. I think it's working (I ran coverage in a test that uses hardhat_reset
), but it seems brittle to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fvictorio Is your concern that the event callback won't be awaited by EventEmitter? Or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a specific concern tbh. It's just that something that used to happen "immediately" after a reset now will not. There might be a hidden assumption that is now broken, but it's hard to know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thank you. Left a question about the reset event just for my own understanding.
This change means that solidity-coverage might not work correctly with extended providers, since we are not passing the extenders to createProvider. I don't think this is going to be a problem immediately, but it's something that should be fixed sooner rather than later.
Do you think solidity-coverage will need to begin using the extendProvider API under development in hh 3932 ? Or is that not going to help?
(Am merging and will try to publish a new version tomorrow after doing additional maintenance here).
// already initialized. | ||
await provider.init(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
env.network.provider.on(HARDHAT_NETWORK_RESET_EVENT, () => { | ||
api.attachToHardhatVM(env.network.provider); | ||
env.network.provider.on(HARDHAT_NETWORK_RESET_EVENT, async () => { | ||
await api.attachToHardhatVM(env.network.provider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fvictorio Is your concern that the event callback won't be awaited by EventEmitter? Or something else?
I'm not sure, tbh. I think hardhat-gas-reporter might use that new feature. solidity-coverage should (maybe) use the trace hooks instead. That's my guess at least. |
Hey @cgewecke, could you please release a new version with this change? We released the new version of Hardhat that has that change, and so the current latest version of |
@fvictorio Published in 0.8.3 now |
Thanks! |
solidity-coverage
relies on a couple of internal APIs that we are about to change:network.provider
object is now lazily initialized, which means that you have to call.init()
it before trying to navigate the_wrapped
provider chaincreateProvider
signature changed, and this function is async nowThis change means that solidity-coverage might not work correctly with extended providers, since we are not passing the extenders to
createProvider
. I don't think this is going to be a problem immediately, but it's something that should be fixed sooner rather than later.