Skip to content
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

Classes inheriting from Web3 did not attach modules appropriately #2587

Merged
merged 1 commit into from Jul 29, 2022

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Jul 26, 2022

What was wrong?

closes #2586

How was it fixed?

  • Import Web3 locally in order to avoid circular import. This allows us to make the check we wanted to in the first place which is to check that the parent_module is an instance of the Web3 class (or inherits from it) when w3 is None.

Todo:

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@fselmo fselmo force-pushed the bugfix-web3-inheritance branch 2 times, most recently from bbaf880 to f6192ed Compare July 26, 2022 21:42
@fselmo fselmo marked this pull request as ready for review July 26, 2022 21:44
* Bug fix: On the first run of ``attach_modules()``, ``w3`` should be ``None`` and the ``parent_module`` is the ``Web3`` class. We should look for this condition and set ``w3 = parent_module``. We couldn't use a simple ``isinstance()`` due to a circular import, but we can import it locally if ``w3`` is ``None``. This allows classes to inherit from ``Web3`` without needing to validate that the class name is ``Web3``. This also allows modules to not need to set a ``w3`` if they don't need one, as the older code was intended to do.
@@ -70,13 +70,13 @@ def test_implicitcontract_transact_default(web3, math_contract, get_transaction_
assert is_integer(start_count) # Verify correct type
# When a function is called that defaults to transact
blocknum, starting_txns = get_transaction_count("pending")
with pytest.warns(DeprecationWarning,
match='deprecated in favor of classic contract syntax') as warnings:
with pytest.warns(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests were looking for a particular number of warnings which can't be sustainable as our dependencies will eventually issue warnings (as eth-abi did which broke these tests).

@fselmo fselmo requested review from pacrob and kclowes July 26, 2022 21:57
@fselmo fselmo mentioned this pull request Jul 28, 2022
1 task
@fselmo fselmo requested a review from wolovim July 28, 2022 21:45
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

lgtm!

@fselmo fselmo merged commit 5bde144 into ethereum:v5 Jul 29, 2022
@fselmo fselmo deleted the bugfix-web3-inheritance branch April 3, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants