Recompile test contracts with latest Solidity version + large refactor #2797
Conversation
fselmo
force-pushed
the
recompile-test-contracts-with-latest-solidity
branch
from
0da1539 to
c918271
Compare
- Update test contracts to compile with Solidity v0.8.17 - Refactor some of the logic using test contracts for efficiency
fselmo
force-pushed
the
recompile-test-contracts-with-latest-solidity
branch
from
c918271 to
39a02f9
Compare
fselmo
changed the title
fselmo
force-pushed
the
recompile-test-contracts-with-latest-solidity
branch
from
39a02f9 to
6267e4a
Compare
- Create script to compile all test contracts at once, with the exception of the FixedReflector contract since no source can be found for it. - Re-organize all contracts as ``.sol`` files with defined Solidity contracts - move all to `web3/_utils/contract_sources`. - Refactor pytest fixtures with appropriate casing since a lot of these were being redefined. - Remove unnecessary pytest fixtures and import constants where appropriate, in an effort to reduce test times and make code more readable (one single source for a constant in most cases). - Run the new test contract compiler script for Solidity version 0.8.17. Integration tests: - Re-generate the fixture to account for new bytecodes for contracts. Fixed test errors: - Validating the ``value`` field for a transaction against the ``payable`` value in the abi needed one more check against the ``stateMutability`` value of ``nonpayable``.
- Clean up contract compiler script
fselmo
force-pushed
the
recompile-test-contracts-with-latest-solidity
branch
from
6267e4a to
9504205
Compare
There was a problem hiding this comment.
Thank you for this refactor and for the script! Generally, this looks really good. I left a few nitpicks, and I noticed that in some places, we have fixtures for our constants (for ex. SOME_CONTRACT_BYTECODE) and sometimes we don't - is there a pattern to leaving those that I didn't pick up on? I commented on it in tests/core/contracts/test_contract_deployment.py, but noticed it elsewhere too. Let me know if that doesn't make sense.
| @@ -7,10 +7,17 @@ | |||
| from web3 import ( | |||
| constants, | |||
| ) | |||
| from web3._utils.contract_sources.contract_data.constructor_contracts import ( | |||
| CONSTRUCTOR_WITH_ADDRESS_ARGUMENT_CONTRACT_RUNTIME, | |||
There was a problem hiding this comment.
nit: for consistency's sake, do you think it's worth making these constants into fixtures like you did with math_contract_runtime?
There was a problem hiding this comment.
The distinction I made was whether or not it's imported in many files as a constant or just one or maybe two test files. I'd be good with making this a session-scoped fixture, or keep it as a constant and reduce some of the clutter in the contracts conftest file. Do you have a preference here? 🤔
There was a problem hiding this comment.
Got it. If it's used in just 1-2 files, I think we can leave it!
| @@ -217,8 +219,8 @@ def setup_chain_state(w3): | |||
| # Math Contract | |||
| # | |||
| math_contract_factory = w3.eth.contract( | |||
There was a problem hiding this comment.
I think we should standardize this naming convention - sometimes this w3.eth.contractobject is called math_contract_instance and sometimes math_contract_factory. I slightly prefer factory I think, but it's not a strong preference.
There was a problem hiding this comment.
factory is present in the integration tests, I'm good with it. I can standardize it 👍🏼
| f"# source: web3/_utils/contract_sources/{dot_sol_filename}:{c}" | ||
| ) | ||
| if len(contract_source) > 88: | ||
| contract_source += " # noqa: E501" |
fselmo
force-pushed
the
recompile-test-contracts-with-latest-solidity
branch
from
56ab08d to
e224442
Compare
I apologize for the beefiness ahead of time but most of this is refactoring to accommodate the new contract sources.
What was wrong?
closes #2301
contributes quite a bit to #2460
How was it fixed?
web3/_utils/contract_sourcesfolder.web3/_utils/contract_sources/contract_dataand neatly assigned to constants for testing.I found one improvement along the way due to a failing test:
payableagainst a transaction that has a non-zerovalue, we weren't considering the case of"stateMutability": "nonpayable"... we were only checking thepayable: {boolean}part of the ABI. This change should clarify what I mean.Todo:
Cute Animal Picture