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

Rewrite ExtendedDNSResolver to support more record types #331

Merged
merged 13 commits into from May 1, 2024

Conversation

Arachnid
Copy link
Member

This rewrite incorporates new functionality, documented in the contract docstring, supporting address records for all coin types, as well as text records.

Copy link
Member

@mdtanrikulu mdtanrikulu left a comment

Choose a reason for hiding this comment

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

just a little comment, overall lgtm

* - in which case they may not contain spaces - or single-quoted. Single quotes in
* a quoted value may be backslash-escaped.
*
* Record types:
Copy link
Member

Choose a reason for hiding this comment

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

How about contenthash?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no browsers that would prioritize a contenthash in ENS over the A records of the domain right now, and no reasonable prospect that that'll change any time soon.

Choose a reason for hiding this comment

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

true, browsers won't care about any of these records, but some people still want to associate IPFS with DNS names. it would be easy to implement because it doesn't require a parameter, (c=<hash>), and then you have complete resolver support.

ps. I know these records need to be evm-readable, but you have 255 chars to work with and humans involved. how about addr[60]=0x00, text[avatar]='http...', and contenthash=<hash>. maybe support addr=0x00 and a=0x00 as well.

pps. dnsname.ens.eth is a mouthful, dns.resolver would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

true, browsers won't care about any of these records, but some people still want to associate IPFS with DNS names. it would be easy to implement because it doesn't require a parameter, (c=), and then you have complete resolver support.

If there are any integrations that want to read this field from ENS names, I'll happily add support here.

ps. I know these records need to be evm-readable, but you have 255 chars to work with and humans involved. how about addr[60]=0x00, text[avatar]='http...', and contenthash=. maybe support addr=0x00 and a=0x00 as well.

256 characters is only 5 addresses. We don't have a lot of spare space to work with here.

expect(valid).to.equal(false)
expect(address).to.equal('0x0000000000000000000000000000000000000000')
Copy link
Member

Choose a reason for hiding this comment

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

ZERO_ADDRESS

)
})

it('handles escaped quotes', async function () {
Copy link
Member

Choose a reason for hiding this comment

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

Are there any invalid texts or it literally accept any text?

Copy link
Member

@makoto makoto Mar 7, 2024

Choose a reason for hiding this comment

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

Also does it work with any length of the string as long as DNS record can store?

expect(result).to.equal(testAddress.toLowerCase())
})

it('handles multiple spaces between records', async function () {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if someone put comma , ?

@Arachnid Arachnid requested a review from makoto April 4, 2024 08:21
@Arachnid Arachnid merged commit 8e8cf71 into staging May 1, 2024
3 checks passed
@Arachnid Arachnid deleted the feature/better-offchain-dns branch May 1, 2024 10:49
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.

None yet

4 participants