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 #2592

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Aug 1, 2022

What was wrong?

v6 version of PR #2587

How was it fixed?

  • 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.

Todo:

Cute Animal Picture

Screen Shot 2022-08-01 at 09 39 43

* 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.
@fselmo fselmo marked this pull request as ready for review August 1, 2022 15:45
@fselmo fselmo requested review from kclowes and pacrob August 1, 2022 15: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!

Copy link
Collaborator

@kclowes kclowes 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 d594b52 into ethereum:master Aug 2, 2022
@fselmo fselmo deleted the v6-PR-for-2587 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.

3 participants