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

Remove AttributeDict method formatters and opt for middleware #2805

Merged
merged 5 commits into from
Feb 7, 2023

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Feb 3, 2023

What was wrong?

  • AttributeDict leads to typing complications when properties are accessed via attribute, rather than key / value. This doesn't complicate too much since a user can opt for key / value every time but it isn't ideal to provide an option that breaks typing.

closes #1656

How was it fixed?

The approach suggested in #1656 does not feel like a great resolution since NamedTuple will rename any reserved python keywords to _0, _1, _2, etc... Seeing as JSON-RPC responses commonly have properties like from, for example, this would not lead to a very good user experience.

  • AttributeDict conversion was removed from the result formatters and moved back to being only in the middleware. A more inclusive recursive approach was taken to make sure all results get passed through the recursive checks for conversion of dict to AttributeDict. Users who wish to turn off all recursive conversion to AttributeDict can simply remove the attrdict_middleware (or async_attrdict_middleware) and not worry about possible typing complications.

  • In order for this to work for async as well, support for async_attrdict_middleware was introduced. This is also now one of the default middlewares for the EthereumTesterProvider (and AsyncEthereumTesterProvider) as dict -> AttributeDict was the default behavior, via result formatters, for EthereumTesterProvider.

Todo:

  • Add entry to the release notes
  • Fix some broken tests that expect the attrdict result formatter to be present
  • Update docs / add new documentation around these changes.
  • Add new tests for attrdict_middleware + async_attrdict_middleware

Cute Animal Picture

20230205_142431

- ``AttributeDict`` leads to typing complications when properties are accessed via attribute, rather than key / value. This doesn't complicate too much since a user can opt for key / value every time but it isn't ideal to provide an option that breaks typing. Those who wish to turn off all recursive conversion to ``AttributeDict`` can simply remove the ``attrdict_middleware`` (or ``async_attrdict_middleware``) and not worry about possible typing complications.

- In order for this to work for async as well, support for ``async_attrdict_middleware` was introduced. This is also now one of the default middlewares for the ``EthereumTesterProvider`` as this was the default behavior achieved via the result formatters.
- fix test_process_params to expect one result formatter
- add attribute dict middlewares to sync and async local filter middleware tests
- add attrdict middleware check to async http provider test
@fselmo fselmo requested review from kclowes and pacrob February 6, 2023 21:08
@fselmo fselmo marked this pull request as ready for review February 6, 2023 21:08
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! Just had one nit in the docs, feel free to take or leave.

docs/web3.eth.rst Outdated Show resolved Hide resolved
def middleware(method: RPCEndpoint, params: Any) -> RPCResponse:
if method in (
"eth_call",
"eth_estimateGas",
"eth_sendTransaction",
):
fill_default_from = fill_default("from", guess_from, w3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's wild that these are the only methods that need a default from 🤔

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 d917198 into ethereum:master Feb 7, 2023
@fselmo fselmo deleted the opt-in-attribute-dicts branch April 25, 2023 16:48
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.

Typing of AttributeDict return values is broken
3 participants