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.

Sign up for GitHub

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

Jump to bottom

Async name to address new approach #3012

Merged
merged 12 commits into from
Jul 7, 2023
Merged

Async name to address new approach #3012

merged 12 commits into from
Jul 7, 2023

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Jun 30, 2023 •
edited
Loading

What was wrong?

ENS name to address needs async support

How was it fixed?

  • Add async support for ENS name to address in a way that doesn't need to utilize functools as that was a messy approach for async (see Async name to address #2758).
  • Make tests more robust for both sync and async name to address middleware

Todo:

Cute Animal Picture

20230630_173246

@fselmo
Add async support for ENS name to address middleware
2158e54 
- Support asynchronous ENS name-to-address conversion via a new bit of logic since async does not play well with functools.
@fselmo
Add tests for async_name_to_address_middleware
2bdb55e 
- Update some of the testing for sync name_to_address_middleware as well since these tests can lead to false positives sometimes. Look for an actual result, not a mocked one.
@fselmo
Remove ENS check for mainnet
ce34259 
- ENS supports many chains now. We should remove the mainnet check.
@fselmo
Make name-to-address middleware tests more robust
c521167 
- Add tests for sequence arg types for methods as well as dict arg type
@fselmo fselmo force-pushed the async-name-to-address-new-approach branch from 9364d72 to bf396f9 Compare June 30, 2023 20:38
@fselmo
support address[] abi type in async name to address middleware
3ab1509 
- Look for ENS names in address lists (abi type ``address[]``) and convert to address
@fselmo fselmo force-pushed the async-name-to-address-new-approach branch from bf396f9 to 82383bc Compare June 30, 2023 23:11
@fselmo
Mirror sync logic for async name to address
c4bcdb7 
- There wasn't too much code copying in the end with this approach so this seems better. This provides proper recursion for nested address list types (address[][][], address[2][1], etc) and gives async and sync the same treatment as they basically follow the same approach.
@fselmo fselmo force-pushed the async-name-to-address-new-approach branch from 82383bc to c4bcdb7 Compare June 30, 2023 23:12
fselmo added a commit to fselmo/web3.py that referenced this pull request Jun 30, 2023
@fselmo
Add relevant newsfragments for ethereum#3012
ce5b2ae
@fselmo fselmo marked this pull request as ready for review June 30, 2023 23:21
@fselmo fselmo requested review from pacrob and reedsa June 30, 2023 23:21
fselmo added a commit to fselmo/web3.py that referenced this pull request Jun 30, 2023
@fselmo
Add relevant newsfragments for ethereum#3012
90da562
@fselmo fselmo force-pushed the async-name-to-address-new-approach branch from ce5b2ae to 90da562 Compare June 30, 2023 23:26
fselmo added a commit to fselmo/web3.py that referenced this pull request Jun 30, 2023
@fselmo
Add relevant newsfragments for ethereum#3012
79b04ae
@fselmo fselmo force-pushed the async-name-to-address-new-approach branch from 90da562 to 79b04ae Compare June 30, 2023 23:38
@fselmo fselmo force-pushed the async-name-to-address-new-approach branch from 79b04ae to 3d8cd36 Compare June 30, 2023 23:39
pacrob
pacrob reviewed Jul 5, 2023
View reviewed changes
docs/middleware.rst Outdated Show resolved Hide resolved
fselmo added a commit to fselmo/web3.py that referenced this pull request Jul 5, 2023
@fselmo
Address comments from PR ethereum#3012
56495a2
@fselmo fselmo force-pushed the async-name-to-address-new-approach branch from 5ff970a to 3ee4d17 Compare July 5, 2023 19:09
@fselmo fselmo force-pushed the async-name-to-address-new-approach branch from 3ee4d17 to 90fcf86 Compare July 5, 2023 19:35
reedsa
reedsa approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

Lgtm

kclowes
kclowes reviewed Jul 7, 2023
View reviewed changes
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.

This looks good to me! I left one nit and a comment around error backwards compatibility.

As a side note, when I was testing this out, I would see multiple warnings about ensip15_normalization, and it wasn't immediately clear how I could turn this off. I tried doing a w3.ens.ensip15_normalization = True, but that didn't work. It doesn't have to be in this PR, but it would be nice to add how to turn off that warning into the error message. I think it's confusing UX because when I was running w3.eth.get_balance('ethereum.eth'), I get a warning about normalize_name() which I know is being called under the hood, but it's not super clear how or why IMHO. Here's a screenshot since my comment is sort of confusing:
image

tests/core/middleware/test_name_to_address_middleware.py Show resolved Hide resolved
web3/_utils/normalizers.py Outdated Show resolved Hide resolved
@fselmo
Copy link
Collaborator Author

fselmo commented Jul 7, 2023

@kclowes regarding the ENSIP-15 warnings, I think we talked about this the other day and these are the changes I mentioned might be best in a separate PR. We're going to have to pass around a Web3 / AsyncWeb3 instance internally and make sure it can make its way to validate_address(). I think this technically constitutes a "bugfix" and would be better as a separate PR.

fselmo added a commit to fselmo/web3.py that referenced this pull request Jul 7, 2023
@fselmo
Address comments on PR ethereum#3012
1f1bd7f
@fselmo fselmo force-pushed the async-name-to-address-new-approach branch from 1f1bd7f to eeaba81 Compare July 7, 2023 19:56
@kclowes
Copy link
Collaborator

kclowes commented Jul 7, 2023

I think this technically constitutes a "bugfix" and would be better as a separate PR.

Yeah, a separate PR is fine by me. I just wanted to make sure it got flagged. I'll add an issue too so we can track.

@fselmo fselmo merged commit 3fb2630 into ethereum:main Jul 7, 2023
1 check passed
fselmo added a commit that referenced this pull request Jul 7, 2023
@fselmo
Add relevant newsfragments for #3012
e5b1ac4
fselmo added a commit that referenced this pull request Jul 7, 2023
@fselmo
Address comments from PR #3012
9d2387e
@fselmo fselmo deleted the async-name-to-address-new-approach branch July 7, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pacrob pacrob pacrob left review comments

@kclowes kclowes kclowes left review comments

@reedsa reedsa reedsa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Async curry? Async name_to_address middleware
4 participants
@fselmo @kclowes @reedsa @pacrob