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

Feat/dnsaddr #50

Merged
merged 1 commit into from
Jan 7, 2018
Merged

Feat/dnsaddr #50

merged 1 commit into from
Jan 7, 2018

Conversation

dryajov
Copy link
Member

@dryajov dryajov commented Jan 2, 2018

Renaming /dns to /dnsaddr to conform with go

@coveralls
Copy link

coveralls commented Jan 2, 2018

Coverage Status

Coverage remained the same at 96.897% when pulling 66e39a9 on feat/dnsaddr into f6b52a1 on master.

@coveralls
Copy link

coveralls commented Jan 2, 2018

Coverage Status

Coverage remained the same at 96.897% when pulling 5890edc on feat/dnsaddr into f6b52a1 on master.

@daviddias
Copy link
Member

Why this change @lgierth @whyrusleeping ??

@ghost
Copy link

ghost commented Jan 7, 2018

Oh I see now what you meant @dryajov. The /dns in the protocol codes table is definitely correct as it is.

@diasdavid the context is that @dryajov noticed that there's /dnsaddr in go-ipfs, but not in js-ipfs, and that there's /dns in js-ipfs but not anywhere else.

I think I'd propose:

  • Remove /dns -- we haven't defined /dns, the only thing we've done for it is reserve its protocol code in the table.
  • Add /dnsaddrand make it a "resolvable" protocol. You will encounter /dnsaddr addresses and it'd be good to regard them as valid.

I see that Multiaddr.resolve() doesn't really do anything yet. Maybe go-multiaddr-dns can serve as inspiration.

@daviddias
Copy link
Member

@lgierth I'm not understanding what is the real difference between /dns and /dnsaddr other than it seems that go-ipfs shipped /dnsaddr in the latest version, although it was not present on the table.

@ghost
Copy link

ghost commented Jan 7, 2018

I'm not understanding what /dns is supposed to do -- we never defined what it'd do. We only defined /dns4 and /dns6 (and /dnsaddr)

@daviddias
Copy link
Member

Ack. Seems I never got the packet about /dnsaddr but yeah I remember we being unsure about /dns.

Looks good to me know. Merging and releasing. Thanks @lgierth and @dryajov :)

@daviddias daviddias merged commit 99a1aa4 into master Jan 7, 2018
@daviddias daviddias deleted the feat/dnsaddr branch January 7, 2018 14:01
dryajov added a commit to dryajov/js-multiaddr that referenced this pull request Jun 21, 2018
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.

3 participants