ethereum-optimism / plugins

[Optimism] Plugins!
MIT License
12 stars 10 forks source link

HardhatNode.mineBlock causes an exception during OVM tests #17

Closed Bobface closed 3 years ago

Bobface commented 3 years ago

Summary

When running OVM tests using the l2ethers injected into Hardhat via the @eth-optimism/plugins/hardhat/ethers plugin, an exception is caused by HardhatNode.mineBlock causes an exception during tests:

PS C:\Users\user\Desktop\ideamarket-contracts> npx hardhat test .\test\contracts\ovm\core\IdeaToken.test.js

  core/IdeaToken
    1) "before each" hook for "admin is owner"

  0 passing (1s)
  1 failing

  1) core/IdeaToken
       "before each" hook for "admin is owner":
     TypeError: Cannot read property 'includes' of undefined
      at HardhatNode.mineBlock (node_modules\hardhat\src\internal\hardhat-network\provider\node.ts:324:24)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)
      at EthModule._sendTransactionAndReturnHash (node_modules\hardhat\src\internal\hardhat-network\provider\modules\eth.ts:1223:18)
      at HardhatNetworkProvider.request (node_modules\hardhat\src\internal\hardhat-network\provider\provider.ts:100:18)
      at EthersProviderWrapper.send (node_modules\@eth-optimism\plugins\src\hardhat\ethers\internal\ethers-provider-wrapper.ts:13:20)

The error happens when deploying a contract:

https://github.com/Ideamarket/ideamarket-contracts/blob/0f0f253a9afc75469e916e90799a0a656eb14a6f/test/contracts/ovm/core/IdeaToken.test.js#L23-L24

Package versions

npm list --depth=0

+-- @eth-optimism/dev@1.1.1
+-- @eth-optimism/plugins@0.0.18
+-- @nomiclabs/hardhat-ethers@2.0.1
+-- @nomiclabs/hardhat-waffle@2.0.1
+-- @nomiclabs/hardhat-web3@2.0.0
+-- @openzeppelin/cli@2.8.2
+-- @openzeppelin/contracts@3.3.0
+-- @openzeppelin/test-helpers@0.5.10
+-- bignumber.js@9.0.1
+-- chai@4.3.3
+-- dotenv@8.2.0
+-- ethereum-waffle@3.3.0
+-- ethers@5.0.31
+-- hardhat@2.1.1
+-- moment@2.29.1
+-- prettier@2.2.1
+-- prompt@1.1.0
+-- solidity-coverage@0.7.16
`-- web3@1.3.4

Reproduce

git clone https://github.com/ideamarket/ideamarket-contracts.git
cd ideamarket-contracts
git checkout 0f0f253
npm i
npx hardhat compile
npx hardhat test ./test/contracts/ovm/core/IdeaToken.test.js
tynes commented 3 years ago

One to one support for @eth-optimism/plugins/hardhat/ethers with @nomiclabs/hardhat-ethers is not prioritized for the near future. The best way to develop would be with https://github.com/ethereum-optimism/optimism-integration. This prevents the possibility of a difference in the JS OVM and the actual in production OVM. If you would like to continue using the plugin, we would be happy to accept any pull requests for bugfixes

Bobface commented 3 years ago

@tynes Thanks for your response, I'd like to continue using the plugin and have done some digging into what is going on. I think that I have identified the problem:

When a transaction reverts, Hardhat's fork of ethereumjs-vm throws a VMError exception. See here:

https://github.com/nomiclabs/ethereumjs-vm/blob/acc1623c67c072606c74697e5a10e4ae5a698fe3/lib/exceptions.ts#L16-L24

When receiving an exception during tx execution, Hardhat checks which type of exception was thrown. This is the where the bug can be located:

https://github.com/nomiclabs/hardhat/blob/3be60c05dc7bd7c37da3a38ba64c62e75fade831/packages/hardhat-core/src/internal/hardhat-network/provider/node.ts#L1166-L1168

This if-statement should not be entered, as the exception is a VmError.

However, eth-optimism provides its own fork of ethereumjs-vm. This causes the instanceof check to fail, and the if-statement is entered. The exception is re-thrown though it should not, causing a crash in another part of the code, where another type of exception containing a message field is expected, which VMError does not have:

https://github.com/nomiclabs/hardhat/blob/3be60c05dc7bd7c37da3a38ba64c62e75fade831/packages/hardhat-core/src/internal/hardhat-network/provider/node.ts#L324-L326


In summary, every test which includes a reverting transaction will cause Hardhat to crash. I hope that that's enough information to implement a fix for this problem. If you'd still like me to submit a PR, I would greatly appreciate some assistance on how to best fix this bug.

smartcontracts commented 3 years ago

@Bobface fantastic analysis, I think you have it dead on. Submitting a PR to hardhat to loosen this check a bit might be our best option. I can't think of an easier way to do this other than introducing more hardhat-specific hacks. I'd prefer not to do that though :-(

smartcontracts commented 3 years ago

Opened up an issue: https://github.com/nomiclabs/hardhat/issues/1317

smartcontracts commented 3 years ago

The wonderful people at hardhat made a fix for this. Hopefully it'll be released soon and we can get this closed out.

platocrat commented 3 years ago

@smartcontracts I saw that nomiclabs/hardhat#1317 was closed.

Is there anything that still needs to be done to close this issue?

smartcontracts commented 3 years ago

Closing!