feat(routing/http): IPIP-518: HTTP(S) URLs in Routing V1 API #1051
+939
−141
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ing API Implements schema-agnostic support for HTTP(S) URLs alongside multiaddrs in the Delegated Routing V1 HTTP API as specified in IPIP-518. Key changes: - Add new Address type that accepts both multiaddrs (starting with /) and URIs - Update PeerRecord and BitswapRecord to use Addresses type instead of []Multiaddr - Implement defensive programming: unsupported addresses are skipped, not errors - Add special protocol filtering for http/https/tls matching - Maintain full backward compatibility with multiaddr-only implementations The implementation is schema-agnostic, accepting any valid URI scheme for future extensibility. URLs are preserved as-is without lossy conversion to multiaddrs, avoiding conversion issues documented in multiaddr#63. Ref: ipfs/specs#518
8 tasks
lidel
changed the title
Implements backward compatibility layer for HTTP(S) URL to multiaddr conversion during transition period. Uses /https (not /tls/http) to match IPNI convention and /dns for dual-stack support. - Add ToMultiaddr() method and String() for Addresses type - Replace TODOs in contentrouter with actual conversion - Add comprehensive tests Temporary compatibility layer until ecosystem adopts IPIP-518 URLs.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1051 +/- ##
==========================================
+ Coverage 60.75% 60.85% +0.09%
==========================================
Files 268 269 +1
Lines 33593 33805 +212
==========================================
+ Hits 20411 20572 +161
- Misses 11510 11554 +44
- Partials 1672 1679 +7
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
gammazero
approved these changes
Oct 31, 2025
Comment on lines
+26
to
+51
| func NewAddress(s string) (*Address, error) { | ||
| addr := &Address{raw: s} | ||
|
|
||
| // IPIP-518 parsing logic | ||
| if strings.HasPrefix(s, "/") { | ||
| // Parse as multiaddr | ||
| ma, err := multiaddr.NewMultiaddr(s) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid multiaddr: %w", err) | ||
| } | ||
| addr.multiaddr = ma | ||
| } else { | ||
| // Parse as URI - accept any valid URI scheme | ||
| u, err := url.Parse(s) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid URI: %w", err) | ||
| } | ||
| // Must be absolute URL | ||
| if !u.IsAbs() { | ||
| return nil, fmt.Errorf("URI must be absolute") | ||
| } | ||
| addr.url = u | ||
| } | ||
|
|
||
| return addr, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Every time I see this called, the returned *Address is immediately dereferenced. This indicates that it would be better to return an Address struct directly, and not a pointer to said struct. Also, all the types inside are effectively pointers, so return the struct literal does not incur any overhead to worry about.
Suggested change
| func NewAddress(s string) (*Address, error) { | |
| addr := &Address{raw: s} | |
| // IPIP-518 parsing logic | |
| if strings.HasPrefix(s, "/") { | |
| // Parse as multiaddr | |
| ma, err := multiaddr.NewMultiaddr(s) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid multiaddr: %w", err) | |
| } | |
| addr.multiaddr = ma | |
| } else { | |
| // Parse as URI - accept any valid URI scheme | |
| u, err := url.Parse(s) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid URI: %w", err) | |
| } | |
| // Must be absolute URL | |
| if !u.IsAbs() { | |
| return nil, fmt.Errorf("URI must be absolute") | |
| } | |
| addr.url = u | |
| } | |
| return addr, nil | |
| } | |
| func NewAddress(s string) (Address, error) { | |
| addr := Address{raw: s} | |
| // IPIP-518 parsing logic | |
| if strings.HasPrefix(s, "/") { | |
| // Parse as multiaddr | |
| ma, err := multiaddr.NewMultiaddr(s) | |
| if err != nil { | |
| return Address{}, fmt.Errorf("invalid multiaddr: %w", err) | |
| } | |
| addr.multiaddr = ma | |
| } else { | |
| // Parse as URI - accept any valid URI scheme | |
| u, err := url.Parse(s) | |
| if err != nil { | |
| return Address{}, fmt.Errorf("invalid uri: %w", err) | |
| } | |
| // Must be absolute URL | |
| if !u.IsAbs() { | |
| return Address{}, fmt.Errorf("uri must be absolute") | |
| } | |
| addr.url = u | |
| } | |
| return addr, nil | |
| } |
nit: Do not capitalize "URL" and "URI" in error message as that is not consistent with golang stdlib:
https://cs.opensource.google/go/go/+/refs/tags/go1.25.3:src/net/url/url.go;l=521
| func (a *Address) Protocols() []string { | ||
| if a.url != nil { | ||
| return []string{a.url.Scheme} | ||
| } else if a.multiaddr != nil { |
Contributor
There was a problem hiding this comment.
Suggested change
| } else if a.multiaddr != nil { | |
| } | |
| if a.multiaddr != nil { |
Comment on lines
+187
to
+189
| // "tls" matches any multiaddr with /tls | ||
| for _, p := range protocols { | ||
| if p == "tls" { |
Contributor
There was a problem hiding this comment.
Should this also match "https"?
Comment on lines
+175
to
+183
| if p == "https" { | ||
| return true | ||
| } | ||
| if p == "tls" { | ||
| hasTLS = true | ||
| } | ||
| if p == "http" { | ||
| hasHTTP = true | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| if p == "https" { | |
| return true | |
| } | |
| if p == "tls" { | |
| hasTLS = true | |
| } | |
| if p == "http" { | |
| hasHTTP = true | |
| } | |
| if p == "https" { | |
| switch p { | |
| case "https": | |
| return true | |
| case "tls": | |
| hasTLS = true | |
| case "http": | |
| hasHTTP = true | |
| } |
Comment on lines
+254
to
+262
| var maStr string | ||
| if scheme == "https" { | ||
| // For HTTPS, use /https as this is what existing HTTP providers | ||
| // announce on IPNI, so we follow same convention for backward-compatibility | ||
| maStr = fmt.Sprintf("/%s/%s/tcp/%s/https", addrProto, host, port) | ||
| } else { | ||
| // For HTTP, use /http | ||
| maStr = fmt.Sprintf("/%s/%s/tcp/%s/http", addrProto, host, port) | ||
| } |
Contributor
There was a problem hiding this comment.
Since scheme can only be "http" or "https" just use scheme, instead of the if-else.
Suggested change
| var maStr string | |
| if scheme == "https" { | |
| // For HTTPS, use /https as this is what existing HTTP providers | |
| // announce on IPNI, so we follow same convention for backward-compatibility | |
| maStr = fmt.Sprintf("/%s/%s/tcp/%s/https", addrProto, host, port) | |
| } else { | |
| // For HTTP, use /http | |
| maStr = fmt.Sprintf("/%s/%s/tcp/%s/http", addrProto, host, port) | |
| } | |
| // For HTTPS, use /https as this is what existing HTTP providers | |
| // announce on IPNI, so we follow same convention for backward-compatibility | |
| maStr := fmt.Sprintf("/%s/%s/tcp/%s/%s", addrProto, host, port, scheme) |
| // Returns nil if the address cannot be converted (e.g., non-HTTP schemes or invalid addresses). | ||
| // This is a temporary compatibility layer for the transition period while existing software | ||
| // expects multiaddrs with /http protocol to signal HTTP retrieval support. | ||
| func (a *Address) ToMultiaddr() multiaddr.Multiaddr { |
Contributor
There was a problem hiding this comment.
An alternative to this that avoids hardcoded values for addrProto can be found here:
https://github.com/ipni/go-libipni/blob/main/maurl/convert.go#L110-L156
Both approaches work. The go-libipni version has had years of testing and operation if you want to cut and paste that one, but that does not mean it is necessarily better. I think perhaps using megular expressions would be better still.
Contributor
|
Triage: Consider updating IPIP to suggest different peer schema. Note: Should not assume trustless gateway when HTTP URI with no metadata |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements IPIP-518 which extends
/routing/v1with schema-agnostic support for URIs alongside multiaddrs.Key changes:
Addresstype that accepts both multiaddrs (starting with/) and URIsPeerRecordandBitswapRecordto useAddressestype instead of[]Multiaddrhttps://to be used more and more across the stack)https://urls into/httpsmultiaddrs/httpsmultiaddrsThe implementation is not limited to
https://, is schema-agnostic, accepting any valid URI scheme for future extensibility. URLs are preserved as-is without lossy conversion to multiaddrs, avoiding historical conversion issues documented in the IPIP.IPIP:
TODO