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: libp2phttp /http-path #2850

Merged
merged 10 commits into from
Jul 16, 2024
Merged

feat: libp2phttp /http-path #2850

merged 10 commits into from
Jul 16, 2024

Conversation

MarcoPolo
Copy link
Collaborator

Adds support for multiaddr URIs that contain /http-path components. In other words, this works now:

client := http.Client{Transport: &libp2phttp.Host{ options... }}
client.Get("multiaddr:/dns/example.com/tls/http/http-path/some%2fpath")
// equivalent to
client.Get("https://example.com/some/path")
// And of course works with any libp2p stream transport:
client.Get("multiaddr:/dnsaddr/example.com/p2p/QmFoo/http-path/some%2fpath")

p2p/http/libp2phttp.go Outdated Show resolved Hide resolved
p2p/http/libp2phttp.go Show resolved Hide resolved
@@ -657,12 +660,22 @@ func (h *Host) RoundTrip(r *http.Request) (*http.Response, error) {
return nil, err
}
addr, isHTTP := normalizeHTTPMultiaddr(addr)
parsed, err := parseMultiaddr(addr)
Copy link
Member

@sukunrt sukunrt Jun 27, 2024

Choose a reason for hiding this comment

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

We need this parsing only if it's a http multiaddr, right? Otherwise we can just use the addr?

I would prefer calling this only when it's an /http multiaddr because parseMultiaddr looks specific to parsing /http multiaddrs. Having said that, I haven't been able to find any multiaddr that would make parseMultiaddr do something funny.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need this parsing only if it's a http multiaddr, right?

Both. We use the http-path, host, and port in both code paths.

Would you prefer we name this something else?

Copy link
Member

Choose a reason for hiding this comment

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

You only need parsed.peer in that code path so I was wondering if we should just get the peerID from the addr in that code path.

This is fine too though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need http path as well

@@ -657,12 +660,22 @@ func (h *Host) RoundTrip(r *http.Request) (*http.Response, error) {
return nil, err
}
addr, isHTTP := normalizeHTTPMultiaddr(addr)
parsed, err := parseMultiaddr(addr)
Copy link
Member

Choose a reason for hiding this comment

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

You only need parsed.peer in that code path so I was wondering if we should just get the peerID from the addr in that code path.

This is fine too though.

// multiaddr://here-instead.
if relative.OmitHost {
// Not relative (at least we can't tell). Nothing we can do here
return nil, errors.New("not relative")
Copy link
Member

Choose a reason for hiding this comment

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

You can reuse this error and avoid allocation for all the calls. This looks like it'll be used often.

@MarcoPolo MarcoPolo merged commit d84736f into master Jul 16, 2024
11 checks passed
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.

2 participants