-
Notifications
You must be signed in to change notification settings - Fork 414
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
Conversation
There was a problem hiding this 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about contenthash?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
test/utils/TestHexUtils.js
Outdated
expect(valid).to.equal(false) | ||
expect(address).to.equal('0x0000000000000000000000000000000000000000') |
There was a problem hiding this comment.
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 () { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 () { |
There was a problem hiding this comment.
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 ,
?
This rewrite incorporates new functionality, documented in the contract docstring, supporting address records for all coin types, as well as text records.