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

Question: Move r = await self.resolver(name) call into the else block in async_ens.py? #3582

Open
abhinay opened this issue Jan 17, 2025 · 3 comments · May be fixed by #3584
Open

Question: Move r = await self.resolver(name) call into the else block in async_ens.py? #3582

abhinay opened this issue Jan 17, 2025 · 3 comments · May be fixed by #3584

Comments

@abhinay
Copy link

abhinay commented Jan 17, 2025

What is the current behavior?

In async_ens.py, the address method calls:

r = await self.resolver(name)
if coin_type is None:
    return cast(ChecksumAddress, await self._resolve(name, "addr"))
else:
    ...

However, r is only used in the else branch (when coin_type is not None). This makes the call to await self.resolver(name) somewhat unnecessary when coin_type is None.

Proposed improvement

Move:

r = await self.resolver(name)

inside the else block so it’s only called when coin_type is not None, i.e.:

if coin_type is None:
    return cast(ChecksumAddress, await self._resolve(name, "addr"))
else:
    r = await self.resolver(name)
    ...

This should:

  • Prevent the extra await self.resolver(name) call when it’s not used.
  • Potentially be more readable, showing that r is only needed for multichain address lookups (coin_type != None).

Questions/Concerns

  1. Does _resolve(name, "addr") internally call self.resolver(name) again, making this change inconsequential?
  2. Are there any side effects (like caching) or known behaviors that rely on await self.resolver(name) being called regardless of coin_type?
  3. Would this refactor break any tests or expectations?

Open Discussion

  • Is this micro-optimization worth it for code clarity and performance?
  • Or is it clearer to always call await self.resolver(name) in one place, even if the method doesn’t end up using it in the if branch?

Any feedback or guidance would be appreciated. If this change seems reasonable, I’m happy to open a PR—or if it’s not necessary, feel free to close this issue.

Cute Animal Picture

A cute animal, just for fun

@fselmo
Copy link
Collaborator

fselmo commented Jan 17, 2025

Hey @abhinay. This looks like a good change. This is low priority but if you opened a PR we'd be happy to review it! Can you make a similar change in the address function of the ENS (sync) class in the same PR if you do decide to open one?

Thanks!

@abhinay
Copy link
Author

abhinay commented Jan 18, 2025

Hey @abhinay. This looks like a good change. This is low priority but if you opened a PR we'd be happy to review it! Can you make a similar change in the address function of the ENS (sync) class in the same PR if you do decide to open one?

Thanks!

Here's a PR: #3584

@abhinay
Copy link
Author

abhinay commented Jan 21, 2025

cc: @fselmo

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 a pull request may close this issue.

2 participants