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

ENS avatar resolution fails when querying using an address #2583

Closed
TomAFrench opened this issue Jan 23, 2022 · 4 comments
Closed

ENS avatar resolution fails when querying using an address #2583

TomAFrench opened this issue Jan 23, 2022 · 4 comments
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published.

Comments

@TomAFrench
Copy link

Describe the bug

Resolving an ENS avatar with provider.getAvatar(nameOrAddress) using an ENS name works correctly, however if the address this name resolves to is passed then the avatar will be returned as null.

async getAvatar(nameOrAddress: string): Promise<null | string> {
let resolver: Resolver = null;
if (isHexString(nameOrAddress)) {
// Address; reverse lookup
const address = this.formatter.address(nameOrAddress);
const reverseName = address.substring(2).toLowerCase() + ".addr.reverse";
const resolverAddress = await this._getResolver(reverseName);
if (!resolverAddress) { return null; }
resolver = new Resolver(this, resolverAddress, "_", address);
} else {
// ENS name; forward lookup
resolver = await this.getResolver(nameOrAddress);
if (!resolver) { return null; }
}
const avatar = await resolver.getAvatar();
if (avatar == null) { return null; }
return avatar.url;
}

It looks to me like we're looking up the reverse record for the address provided and getting its resolver, we then query for the avatar field on here. This makes the assumption that the reverse and forwards resolvers are the same which is unlikely to be correct.

We also pass "_" as the name and so we use an incorrect namehash when pulling the avatar field.

Reproduction steps

Failing test case here: TomAFrench@50c7832

Fix

Based on the fact that we need the user's namehash in order to query the avatar, I think we have to go all they way up to the ENS name and then query using that.

I've made a PR to do this at #2582

Search Terms
ens, avatar

@TomAFrench TomAFrench added the investigate Under investigation and may be a bug. label Jan 23, 2022
@TomAFrench
Copy link
Author

Just noticed this link from when you added avatar support: https://gist.github.com/Arachnid/9db60bd75277969ee1689c8742b75182

To determine the avatar URI for an Ethereum address, the client MUST reverse-resolve the address by querying the ENS registry for the resolver of <address>.addr.reverse, where <address> is the lowercase hex-encoded Ethereum address, without leading '0x'. Then, the client calls .text(namehash('<address>.addr.reverse'), 'avatar') to retrieve the avatar URI for the address.

If a resolver is returned for the reverse record, but calling text causes a revert or returns an empty string, the client MUST call .name(namehash('<address>.addr.reverse')).

Looks like it's correct to attempt to find an avatar on the reverse resolver first however we're not passing the correct name (<address>.addr.reverse) in order to calculate the namehash so this will always fail currently.

@TomAFrench
Copy link
Author

TomAFrench commented Jan 24, 2022

I've added an attempt to read an avatar from the reverse record, however we could save some calls if we create a getName method on Resolver so we don't need to look up the reverse record twice. I'm happy to do do this if you're fine with it.

@ricmoo
Copy link
Member

ricmoo commented Jan 24, 2022

I definitely need to look more into this and check with Nick too. This seems like it is a bug, but I want to make sure we do the right thing.

@ricmoo ricmoo added bug Verified to be an issue. on-deck This Enhancement or Bug is currently being worked on. and removed investigate Under investigation and may be a bug. labels Feb 18, 2022
@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels Mar 19, 2022
@ricmoo
Copy link
Member

ricmoo commented Mar 19, 2022

This should have been fixed in v5.6.0.

If you still have any problems, please re-open this issue.

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants