Validate ethereum_tester class in web3.EthereumTesterProvider #1217
Conversation
docs/providers.rst
Outdated
| see the ``eth-tester`` library documentation for details. | ||
| This provider integrates with the ``eth-tester`` library. The ``eth_tester`` constructor | ||
| argument should be an instance of the :class:`~eth_tester.EthereumTester` or | ||
| :class:`~eth_tester.PyEVMBackend` class provided by the ``eth-tester`` library. |
There was a problem hiding this comment.
I'm not sure if encouraging the use of PyEVMBackend directly is a good idea or not in the docs. Is it better to just promote that eth_tester.EthereumTester is the only way to instantiate an EthereumTesterProvider instance, and the code will be silently forgiving to anybody who mistakenly provides an unwrapped PyEVMBackend?
There was a problem hiding this comment.
I think it's good to document it here as not doing so might lead to the alternate surprise of why it works without wrapping the backend. However, maybe noting the PyEVMBackend specifically could be replaced by indicating that any subclass of the BaseBackend class is allowed.
njgheorghita
force-pushed
the
validate-eth-tester-backend-class
branch
2 times, most recently
from
69a90e3 to
96d4c2e
Compare
docs/providers.rst
Outdated
| see the ``eth-tester`` library documentation for details. | ||
| This provider integrates with the ``eth-tester`` library. The ``eth_tester`` constructor | ||
| argument should be an instance of the :class:`~eth_tester.EthereumTester` or | ||
| :class:`~eth_tester.PyEVMBackend` class provided by the ``eth-tester`` library. |
There was a problem hiding this comment.
I think it's good to document it here as not doing so might lead to the alternate surprise of why it works without wrapping the backend. However, maybe noting the PyEVMBackend specifically could be replaced by indicating that any subclass of the BaseBackend class is allowed.
web3/providers/eth_tester/main.py
Outdated
| @@ -30,12 +30,22 @@ class EthereumTesterProvider(BaseProvider): | |||
| api_endpoints = None | |||
|
|
|||
| def __init__(self, ethereum_tester=None, api_endpoints=None): | |||
| # do not import eth_tester until runtime, it is not a default dependency | |||
| from eth_tester import EthereumTester, PyEVMBackend | |||
There was a problem hiding this comment.
Instead of PyEVMBackend this should be the BaseChainBackend class.
There was a problem hiding this comment.
@pipermerriam That's what I thought at first as well, but then from what I can tell PyEVMBackend is not a subclass of BaseChainBackend - https://github.com/ethereum/eth-tester/blob/91468f5f79/eth_tester/backends/pyevm/main.py#L295
There was a problem hiding this comment.
That's a bug we should remedy. I'll open an issue.
web3/providers/eth_tester/main.py
Outdated
| self.ethereum_tester = EthereumTester() | ||
| else: | ||
| self.ethereum_tester = ethereum_tester | ||
| if isinstance(ethereum_tester, EthereumTester): |
There was a problem hiding this comment.
Instead of nesting this in the else, this could all be one large if/elif/elif clause.
web3/providers/eth_tester/main.py
Outdated
| if isinstance(ethereum_tester, EthereumTester): | ||
| self.ethereum_tester = ethereum_tester | ||
| elif isinstance(ethereum_tester, PyEVMBackend): | ||
| self.ethereum_tester = EthereumTester(ethereum_tester) |
There was a problem hiding this comment.
I find it a little odd to be able to pass the tester or the backend, but no objection 👍
njgheorghita
force-pushed
the
validate-eth-tester-backend-class
branch
from
96d4c2e to
93cc27e
Compare
What was wrong?
Instantiating a provider with
PyEVMBackendshould be done likeweb3 = Web3(Web3.EthereumTesterProvider(EthereumTester(PyEVMBackend()))), but can be mistakenly instantiated without theEthereumTesterwhich will cause tx normalization errors.fixes #1213
fixes #1212
Also dropped the already deprecated
web3.soliditySha3method in this pr.How was it fixed?
Added a check in
EthereumTesterProvider.init()that theethereum_testerarg is one ofEthereumTesterPyEVMBackendand then auto-wraps it inEthereumTesterCute Animal Picture