Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Add ENS avatar and TXT records resolution #889

Merged
merged 11 commits into from
Feb 16, 2022
Merged

Add ENS avatar and TXT records resolution #889

merged 11 commits into from
Feb 16, 2022

Conversation

sbihel
Copy link
Contributor

@sbihel sbihel commented Feb 9, 2022

This PR adds support for resolution of text records and also adds a helper to resolve avatars and validate them (as well as provide a HTTP link to make it easier for the user). This is mostly a translation of ethers.js implementation.

I am not terribly happy with the current interface and place in the code, so I'm hoping you have an opinion on the matter.

Motivation

We simply need to resolve records and specifically avatars of ENS domains.

Solution

TXT records can be resolved using resolve_field (I have encountered both the terms "record" and "field", which isn't great), and the avatar record can also be resolved using resolve_avatar to automatically find the target image and validate ownership in the case of NFTs.

PR Checklist

  • Added Tests
    • Only for avatars, and only using live domains, which will inevitably be a problem.
  • Added Documentation
  • Updated the changelog

Questions

  1. Do you have an idea on how to split up the ERC-721/1155 code nicely?
  2. Would it be preferable to use an Option in the return type of resolve_avatar?
  3. Is there a configuration system in place right now to be able to modify the IPFS HTTP gateway?

As well as arbitrary fields.
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

Do you have an idea on how to split up the ERC-721/1155 code nicely?

few nits and suggestions, can improve once addressed.

Would it be preferable to use an Option in the return type of resolve_avatar?

how would this manifest itself in the response? statuscode? failed decoded?

Is there a configuration system in place right now to be able to modify the IPFS HTTP gateway?

think this is fixed to ipfs.io?

ethers-providers/src/provider.rs Outdated Show resolved Hide resolved
ethers-providers/src/provider.rs Show resolved Hide resolved
ethers-providers/src/provider.rs Outdated Show resolved Hide resolved
ethers-providers/src/provider.rs Outdated Show resolved Hide resolved
ethers-providers/src/provider.rs Show resolved Hide resolved
ethers-providers/src/provider.rs Outdated Show resolved Hide resolved
Comment on lines 269 to 273
async fn resolve_avatar(&self, ens_name: &str) -> Result<Url, Self::Error> {
self.inner().resolve_avatar(ens_name).await.map_err(FromErr::from)
}

async fn resolve_field(&self, ens_name: &str, field: &str) -> Result<String, Self::Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please document these trait functions, or link to the matching functions, that are document.

ethers-providers/src/provider.rs Show resolved Hide resolved
ethers-providers/src/provider.rs Show resolved Hide resolved
}
[0xc8, 0x7b, 0x56, 0xdd]
}
"erc1155" => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if possible, we should make them 2 separate functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

307bdcc adds a ERCToken type and another method to resolve a token. Not sure about how future this is though 🤔

@sbihel
Copy link
Contributor Author

sbihel commented Feb 10, 2022

Would it be preferable to use an Option in the return type of resolve_avatar?

how would this manifest itself in the response? statuscode? failed decoded?

I was thinking of scenarios like: unsupported scheme, or invalid ownership. They're not necessarily errors -- but I don't think it matters too much considering I'm mostly using the opaque CustomError.

Is there a configuration system in place right now to be able to modify the IPFS HTTP gateway?

think this is fixed to ipfs.io?

Anyone can deploy a gateway, another big one is Cloudflare's. So someone could want to use a different one (e.g. private one, or if ipfs.io is down). But I really don't think it's that important for now.

@sbihel
Copy link
Contributor Author

sbihel commented Feb 10, 2022

Oh and also, I'm curious why cargo fmt uses nightly?

@mattsse
Copy link
Collaborator

mattsse commented Feb 10, 2022

Oh and also, I'm curious why cargo fmt uses nightly?

we use cargo +nightly fmt because that includes more options

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm!
thanks!
needs nightly fmt

/// if not configured)
///
/// # Example
/// ```
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's do no_run here

Suggested change
/// ```
/// ```no_run

/// Returns the URL (not necesserily HTTP) of the image behind a token.
///
/// # Example
/// ```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// ```
/// ```no_run

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

nice work!

@gakonst
Copy link
Owner

gakonst commented Feb 15, 2022

Tests seem to be failing here, any clue why? Maybe the IPFS path changed?

  left: `Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("creature.mypinata.cloud")), port: None, path: "/ipfs/QmVHmNwVpiSebryKDZX7B3MXEUZVSACgTpDMPiD6j6Nbcj/9018.jpg", query: None, fragment: None }`,
 right: `Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("creature.mypinata.cloud")), port: None, path: "/ipfs/QmNwj3aUzXfG4twV3no7hJRYxLLAWNPk6RrfQaqJ6nVJFa/9018.jpg", query: None, fragment: None }`', ethers-providers/src/provider.rs:1520:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@sbihel
Copy link
Contributor Author

sbihel commented Feb 16, 2022

Indeed, hopefully the new one won't change for a while

@gakonst gakonst merged commit cd8a9b5 into gakonst:master Feb 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants