-
Notifications
You must be signed in to change notification settings - Fork 232
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
IPIP-0388: Routing HTTP API Support for Querying Multiple Routers #388
base: main
Are you sure you want to change the base?
Conversation
e4f0cfd
to
ec8ea54
Compare
2023-04-19 conversation:
|
routing/ROUTING_V1_HTTP.md
Outdated
|
||
This section contains a non-exhaustive list of known routers that may be supported by clients and servers. | ||
|
||
- "ipfs-public-dht" - The IPFS Public DHT corresponding to the libp2p protocol ID `/ipfs/kad/1.0.0` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we include / how should we name 'server's connected bitswap peers'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ipfs-bitswap-peers
?
I think it is ok to define some canonical names for popular things here, but the spec should be clear that it is at liberty of implementer to use other, opaque strings (for custom / future routing systems)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this list from the IPIP and added sentence that these are opaque strings controlled by implementation/instance.
Co-authored-by: Adin Schmahmann <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, we need this, as noted in cc'd threads
GET /routing/v1/providers?routers=all
(less ambiguous than *
) sounds sensible, there is some UX gap around learning what routers are available, but grabby catch-all that enables all optional backend routers is already usefull.
In a spare moment this PR needs to be rebased to use specs.ipfs.tech, but for now some thoughts inline.
routing/ROUTING_V1_HTTP.md
Outdated
|
||
To select all routers use `?routers=*`. To determine the list of available routers see the `OPTIONS` API below | ||
|
||
### `OPTIONS /routing/v1/providers` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some quick thoughts:
- Learning about available routers via OPTIONS header is ok, but just for the record, it introduces some UX challenges:
- can't paste link in address bar: need to pass custom method to
curl -X OPTIONS
, JS may need explicit entry in Access-Control-Expose-Headers for the value to be readable via JS.
- can't paste link in address bar: need to pass custom method to
- Given the design decisions made when we abandoned Reframe and prioritized user-friendliness and human-readablinity of API, it may be a better UX and less headache to support listing available routers via
GET
and return them as JSON.
Alternative idea just to see if it resonates better:
Request: GET /routing/v1/routers
(dedicated endpoint)
Response: JSON with list of items following router
schema
{
"Schema": "router",
"Name": "ipfs-public-dht",
"Default": false, // is it used when `?routers` is not passed
"RoutingType": ["providers", "ipns", "peers"], // where can it be used
"Details": { .. } // optional opaque JSON with router-type-specific values
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased this IPIP on top of latest spec, and replaced OPTIONS with GET /routing/v1/routers
and a router
schema above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased on top of latest /routing/v1
spec and applied changes from comments.
Main one is dedicated router
schema used for returning enumeration results for GET /routing/v1/routers
(and removal of HTTP OPTIONS
).
routing/ROUTING_V1_HTTP.md
Outdated
|
||
This section contains a non-exhaustive list of known routers that may be supported by clients and servers. | ||
|
||
- "ipfs-public-dht" - The IPFS Public DHT corresponding to the libp2p protocol ID `/ipfs/kad/1.0.0` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this list from the IPIP and added sentence that these are opaque strings controlled by implementation/instance.
routing/ROUTING_V1_HTTP.md
Outdated
|
||
To select all routers use `?routers=*`. To determine the list of available routers see the `OPTIONS` API below | ||
|
||
### `OPTIONS /routing/v1/providers` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased this IPIP on top of latest spec, and replaced OPTIONS with GET /routing/v1/routers
and a router
schema above.
added issue: ipni/storetheindex#2215 to backlog of IPNI repo cc @masih , @gammazero |
Adds support for querying for multiple routers via query parameter. cc @lidel @masih @guseggert
Marked as draft since it's point to the branch renaming delegated routing to routing v1