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

feat: add support for ipns name resolve /ipns/<fqdn> #2002

Merged
merged 15 commits into from
Jun 26, 2019

Conversation

hugomrdias
Copy link
Member

@hugomrdias hugomrdias commented Apr 18, 2019

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

I am afraid the default behavior of go-ipfs changed since we discussed this last time:
https://blog.ipfs.io/83-go-ipfs-0-4-20/#changed-ipfs-resolve-ipfs-dns-ipfs-name-resolve

I believe we should switch behavior in this PR to be recursive by default, to align with go-ipfs 0.4.20 while we are at it.

Below is a summary of edge cases (:anger: indicates discrepancy on our end) when we have more than one level of dnslink (eg. ipfs.io → website.ipfs.io → /ipfs/)

raw hostnames

  • name resolve ipfs.io (recursive by default)

    • go-ipfs 0.4.20: /ipfs/QmeA4pdfRMq1ABThuTYTRgHzCu8xthuoUeK6DsRMMqj33b
    • this PR: /ipns/website.ipfs.io 💢
  • name resolve ipfs.io --recursive=true

    • go-ipfs 0.4.20: /ipfs/QmeA4pdfRMq1ABThuTYTRgHzCu8xthuoUeK6DsRMMqj33b
    • this PR: /ipns/website.ipfs.io 💢
  • name resolve ipfs.io --recursive=false

    • go-ipfs 0.4.20: /ipns/website.ipfs.io
    • this PR: /ipns/website.ipfs.io

IPNS paths

  • name resolve /ipns/ipfs.io/images/ipfs-logo.svg (recursive by default)

    • go-ipfs 0.4.20: /ipfs/QmeA4pdfRMq1ABThuTYTRgHzCu8xthuoUeK6DsRMMqj33b/images/ipfs-logo.svg
    • this PR: /ipns/website.ipfs.io 💢
  • name resolve /ipns/ipfs.io/images/ipfs-logo.svg --recursive=true

    • go-ipfs 0.4.20: /ipfs/QmeA4pdfRMq1ABThuTYTRgHzCu8xthuoUeK6DsRMMqj33b/images/ipfs-logo.svg
    • this PR: /ipns/website.ipfs.io 💢
  • name resolve /ipns/ipfs.io/images/ipfs-logo.svg --recursive=false

    • go-ipfs 0.4.20: /ipns/website.ipfs.io/images/ipfs-logo.svg
    • this PR: /ipns/website.ipfs.io 💢

@hugomrdias
Copy link
Member Author

hugomrdias commented Apr 22, 2019

We can't right now I already opened an issue about it (#2001), it breaks name pubsub. I want to merge this and then fix that.

@lidel
Copy link
Member

lidel commented Apr 22, 2019

@hugomrdias ack on not making it recursive by default in this PR, but unless I am missing some context, name resolve with explicit --recursive=true and --recursive=false should still produce the same result as in go-ipfs, but it does not right now (examples in #2002 (review))

@hugomrdias hugomrdias force-pushed the feat/name-resolve-dns branch from 314c70a to 4b70dc6 Compare May 6, 2019 09:46
@hugomrdias
Copy link
Member Author

@lidel i can only reproduce this one

name resolve /ipns/ipfs.io/images/ipfs-logo.svg --recursive=false

go-ipfs 0.4.20: /ipns/website.ipfs.io/images/ipfs-logo.svg
this PR: /ipns/website.ipfs.io 💢

all the others work as go does.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@hugomrdias I retested after rebase and you are right, that is the only discrepancy left (apart from switching the default, which is tracked in #2001)

lidel added a commit that referenced this pull request May 7, 2019
It requires below to PRs to land first:

#2002
ipfs/js-ipfs-http-response#19
ipfs-inactive/js-ipfs-mfs#48

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to lidel/js-ipfs that referenced this pull request May 7, 2019
It requires below to PRs to land first:

ipfs#2002
ipfs/js-ipfs-http-response#19
ipfs-inactive/js-ipfs-mfs#48

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@alanshaw
Copy link
Member

alanshaw commented May 8, 2019

@hugomrdias does this supercede #1970?

lidel added a commit to ipfs/ipfs-companion that referenced this pull request May 8, 2019
tl;dr opening /ipns/tr.wikipedia-on-ipfs.org/wiki/Mars.html works

Switched to js-ipfs with cherry-picked changes from:
ipfs/js-ipfs#2020
ipfs/js-ipfs#1989
ipfs/js-ipfs#2002
ipfs/js-ipfs-http-response#22
ipfs-inactive/js-ipfs-mfs#48
@hugomrdias
Copy link
Member Author

@hugomrdias does this supercede #1970?

yes for ipfs name resolve, i will handle ipfs resolve in another PR

lidel added a commit that referenced this pull request May 13, 2019
It requires below to PRs to land first:

#2002
ipfs/js-ipfs-http-response#19
ipfs-inactive/js-ipfs-mfs#48

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
`ipns name resolve` dns tests moved to interface-core
resolve call now return a string as per documention
enables ipns tests in the browser
normalizes interface tests over core with interface tests over http-client using http api
@hugomrdias hugomrdias force-pushed the feat/name-resolve-dns branch from ef824d3 to c88fced Compare June 18, 2019 16:03
@hugomrdias hugomrdias requested a review from lidel June 18, 2019 20:53
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Many tests have been removed?

package.json Outdated Show resolved Hide resolved
src/utils/tlru.js Outdated Show resolved Hide resolved
expect(res).to.satisfy(checkAll([cidAdded])) // value propagated to node B
})
})
})
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this test being removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

core has tests that do mostly the same thing, i discussed this with vasco and we decided to remove this.

@hugomrdias
Copy link
Member Author

Many tests have been removed?

has we already discussed this PR tries to keep all tests in interface-core or core and cli tests only test cli behaviour.

@alanshaw
Copy link
Member

as we already discussed this PR tries to keep all tests in interface-core or core and cli tests only test cli behaviour.

I think that’s the right way, but we need to make sure we have the same coverage as we did before (and preferably more)

@alanshaw alanshaw changed the title feat: add support to ipns resolve /ipns/<fqdn> feat: add support to ipns name resolve /ipns/<fqdn> Jun 24, 2019
@alanshaw alanshaw changed the title feat: add support to ipns name resolve /ipns/<fqdn> feat: add support for ipns name resolve /ipns/<fqdn> Jun 24, 2019
@alanshaw
Copy link
Member

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

I believe the coverage drop is false and because of this issue #2202

@alanshaw alanshaw merged commit 5044a30 into master Jun 26, 2019
lidel added a commit that referenced this pull request Jun 27, 2019
It requires below to PRs to land first:

#2002
ipfs/js-ipfs-http-response#19
ipfs-inactive/js-ipfs-mfs#48

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
alanshaw pushed a commit that referenced this pull request Jul 4, 2019
> Part of an effort to run embedded js-ipfs in Brave 🦁 ipfs/ipfs-companion#716
> Fixes #1918 

This PR will add support for `/ipns/` paths at HTTP Gateway.
Smoke test: [/ipns/tr.wikipedia-on-ipfs.org](http://127.0.0.1:9090/ipns/tr.wikipedia-on-ipfs.org/wiki/Anasayfa.html) (IPNS+DNSLink+HAMT-sharded website)
 
This PR depends on the following merged PRs:

  - Gateway Improvements from #1989
    (after merging #1989 I will rebase this PR, which will remove first two commits)
  - PeerID 
    eg. `/ipns/<PeerId-as-multihash-b58>`
    - requires #2002 to land first
  - `/ipns/<libp2p-key-in-cidv1>` 
    - requires multiformats/js-multicodec#45
  - DNSLink 
    eg. `/ipns/<fqdn>/path/file` like `/ipns/docs.ipfs.io/assets/logo.svg`
    - requires #2002 to land first
  - HAMT shard support 
    eg. `/ipns/tr.wikipedia-on-ipfs.org/wiki/Anasayfa.html` (`wiki` is a sharded directory)
    - requires ipfs/js-ipfs-http-response#22 and ipfs-inactive/js-ipfs-mfs#48  to land first
  - Tests for `/ipns/` 

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolving DNSLink Paths: /ipns/<fqdn>
3 participants