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

Jump to bottom

dnsEncode() throws too early #4543

Closed
adraffy opened this issue Jan 15, 2024 · 4 comments
Closed

dnsEncode() throws too early #4543

adraffy opened this issue Jan 15, 2024 · 4 comments
Assignees
@ricmoo
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6

Comments

@adraffy
Copy link

adraffy commented Jan 15, 2024 •
edited

Ethers Version

6.10

Search Terms

dnsEncode, ccip, ensip-10, wrapper

Describe the Problem

dnsEncode() throws on >63 but should throw for >255. This causes longer names to be unreachable via CCIP and can't decode NameWrapper names() function.

Possibly the function needs renamed if this was used for other purposes.

Example:

// throws "invalid DNS encoded entry; length exceeds 63 bytes"
ethers.dnsEncode('a'.repeat(64));

Suggested implementation:

function dnsEncode(name: string): string 
  const MAX_LABEL = 255;
  let v = ensNameSplit(name).map(u => {
      if (u.length > MAX_LABEL) throw new Error(`label too long: ${u.length} > ${MAX_LABEL}`);
      return u;
  });
  v.push(0);
  return hexlify(v);
}

You can also massively simplify ensNameSplit():

function ensNameSplit(name: string): Array<Uint8Array> {
    const norm = ensNormalize(name); // empty string or all labels nonempty
    return norm ? norm.split('.').map(toUtf8Bytes) : [];
}

Code Snippet

No response

Contract ABI

No response

Errors

invalid DNS encoded entry; length exceeds 63 bytes

Environment

node.js (v12 or newer)

Environment (Other)

No response
@adraffy adraffy added investigate Under investigation and may be a bug. v6 Issues regarding v6 labels Jan 15, 2024
@ricmoo
Copy link
Member

ricmoo commented Jan 15, 2024

DNS labels are restricted to a length of 63 (see RFC-1035.

Here is a short article by Microsoft on some of the nuances too. :)

@ricmoo ricmoo added enhancement New feature or improvement. on-deck This Enhancement or Bug is currently being worked on. minor-bump Planned for the next minor version bump. next-patch Issues scheduled for the next arch release. and removed investigate Under investigation and may be a bug. labels Jan 17, 2024
@ricmoo
Copy link
Member

ricmoo commented Jan 17, 2024

Just to keep everyone informed, we've had discussions with ENS and have concluded this is a-ok. :)

...the limit should be 255. Labels that are longer than 63 characters won't be valid in DNS integrations, but that's fine. ~Nick (via Twitter)

In the next minor bump, I'll make the following change:

// current; 6.10
function dnsEncode(name: string): string;

// next minor; 6.11
function dnsEncode(name: string, maxLength = 63): string;

This will ensure the function continues working for others that may be using it using its current defaults, but when used internally by the wildcard resolution logic, it can expand the label width.

Can you also work on updating the ENSIP, @adraffy?

@adraffy
Copy link
Author

adraffy commented Jan 17, 2024

Can you also work on updating the ENSIP, @adraffy?

Awesome. Yeah, I can submit that.

ricmoo added a commit that referenced this issue Jan 31, 2024
@ricmoo
Allow long dnsEncode names with optional length parameter (#4543).
a136348
ricmoo added a commit that referenced this issue Feb 14, 2024
@ricmoo
Allow ENS wildcards with labels up to 255 bytes wide; discussed with …
7f14bde 
…ENS and deemed safe (#4543).
@ricmoo
Copy link
Member

ricmoo commented Feb 14, 2024

This has been address in v6.11.1. Try it out and let me know if you have any more issues.

Thanks! :)

@ricmoo ricmoo closed this as completed Feb 14, 2024
@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. minor-bump Planned for the next minor version bump. next-patch Issues scheduled for the next arch release. labels Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ricmoo ricmoo

Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@adraffy @ricmoo