Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

Discovery based Content Routing #27

Merged
merged 2 commits into from Oct 16, 2019
Merged

Discovery based Content Routing #27

merged 2 commits into from Oct 16, 2019

Conversation

aschmahmann
Copy link
Contributor

The Discovery and Content Routing interfaces are almost exactly the same as described in libp2p/go-libp2p#707, with Discovery being a more generalized form of Content Routing.

We already have a translator from Content Routing to Discovery. This implements the reverse.

The mock discovery is already in #26, so if that gets merged first it should be removed from here and vice versa. Alternatively, it might make sense to have the mocks in go-libp2p-core/test.

@aschmahmann aschmahmann self-assigned this Sep 6, 2019
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Just the prefix for the namespace, other than that looks good.

}

func cidToNs(c cid.Cid) string {
return "/provider/" + c.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps use /cid as the prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong preferences here. /cid is defining based on the key type (CID) and /provider is based on the value type (multiaddr/provider info).

My understanding is that we wanted to get away from advertising CIDs anyway (and instead advertise multihashes) in which case /cid probably isn't the way to go.

@Stebalien thoughts?

Copy link

Choose a reason for hiding this comment

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

let's go with /cid

Copy link
Member

Choose a reason for hiding this comment

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

So, @aschmahmann is right. We'd like to move away from advertising CIDs to advertising hashes. Actually, what if we just went ahead and did /content/$multihash from the start? Or /block/$multihash?

Copy link

@bigs bigs left a comment

Choose a reason for hiding this comment

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

looks good. once we decide on a namespace i'm good to merge

}

func cidToNs(c cid.Cid) string {
return "/provider/" + c.String()
Copy link

Choose a reason for hiding this comment

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

let's go with /cid

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM. Let's just ship this and not bikeshed it to death.

@aschmahmann aschmahmann merged commit 998fc35 into master Oct 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants