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

IPIP-417: Delegated Peer Routing HTTP API #417

Merged
merged 10 commits into from
Sep 25, 2023
Merged

IPIP-417: Delegated Peer Routing HTTP API #417

merged 10 commits into from
Sep 25, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented May 29, 2023

This IPIP introduces the /routing/v1/peers endpoint to the Routing HTTP V1 API for peer routing support.

@hacdias hacdias changed the base branch from main to issues/344 May 29, 2023 09:15
@hacdias hacdias changed the title ipip: Streaming Routing V1 HTTP API IPIP-0417 May 29, 2023
@hacdias hacdias changed the title IPIP-0417 IPIP-0417: Introduce Peer Schema in Routing API May 29, 2023
@hacdias hacdias self-assigned this May 29, 2023
@hacdias hacdias changed the title IPIP-0417: Introduce Peer Schema in Routing API IPIP-417: Delegated Peer Routing HTTP API May 30, 2023
@hacdias hacdias requested a review from lidel May 30, 2023 14:57
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.

@hacdias I had to rebase this one to be based on the latest versions of the other two, apologies for the noise.

src/routing/http-routing-v1.md Outdated Show resolved Hide resolved
src/routing/http-routing-v1.md Outdated Show resolved Hide resolved
@lidel lidel force-pushed the ipip/schema-peer branch 2 times, most recently from b12388d to 9790174 Compare May 31, 2023 19:50
@guillaumemichel
Copy link
Contributor

IMO it would be great to have delegated peer routing use signed peer records. It seems essential because without signatures you have to trust a middleman.

A simple "Signature" field could be added to the proposed schema. The signature can be the array of bytes resulting of the signature using the private key associated with the peer ID (and corresponding algorithm) of (all) the other (non-empty) fields of the schema.

@hacdias
Copy link
Member Author

hacdias commented Jun 14, 2023

@guillaumemichel I like the idea, but that'd require deeper changes to the way we implement things. Currently, the idea was to mostly "relay" the results of dht findpeer in Kubo for example. That doesn't give us signed records. Besides, this specification also indicates that the addresses and protocols may be incorrect.

All in all, we have to find the balance. For IPNS (#379), it is easy to send signed records, because that's literally what is passed around. Here it seems it'd be more complicated.

Base automatically changed from issues/344 to main June 15, 2023 20:27
@lidel lidel self-assigned this Jul 6, 2023
@hacdias hacdias force-pushed the ipip/schema-peer branch 3 times, most recently from 25d51dc to e66a70a Compare July 26, 2023 08:54
Comment on lines 120 to 122
#### Response Body

A [`peer` schema record](#peer).
Copy link
Member Author

Choose a reason for hiding this comment

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

Open Question: should the response body for /routing/v1/peers be a single entity? What we want to obtain is the addresses of the peer. But addresses can end up being discovered on the fly. Supporting streaming is reasonably easy and could be part of the spec.

Copy link
Member

@lidel lidel Jul 28, 2023

Choose a reason for hiding this comment

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

Should be multiple, and work the same as /providers – we may have more than one result for the same peerid coming from different backend services (e.g. if /routing/v1 queries both DHT and IPNI)

Copy link
Member

Choose a reason for hiding this comment

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

cid.contact offers both: /providers returns list of all providers and /providers/<peer-id> returns a specific one.

Copy link
Member Author

@hacdias hacdias Aug 1, 2023

Choose a reason for hiding this comment

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

I updated the Peers API to indicate it can return more than one. It will conform with the streaming API we already have too. Whether we add /peers and /providers too is TBD. Reasonably easy to add. Wdyt @masih @lidel?

@hacdias hacdias marked this pull request as ready for review July 26, 2023 09:00
@hacdias hacdias requested a review from a team as a code owner July 26, 2023 09:00
@hacdias hacdias requested review from lidel and masih August 1, 2023 11:13
src/routing/http-routing-v1.md Outdated Show resolved Hide resolved
@lidel
Copy link
Member

lidel commented Aug 25, 2023

FYSA we've got implementation in

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.

Applied final editorials, and included legacy schemas with guidance to parse them the same way as peer one (to leverage presence of ID and Addrs fields). This is what boxo does for Schema: bitswap entries.

This is ready for final reviews and ratification, will discuss during IPFS Implementers call today

@BigLep
Copy link
Contributor

BigLep commented Sep 20, 2023

@lidel : are we good to merge now?

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.

This has been waiting for ratification feedback for a few weeks now, and no concerns were raised.
Thank you @hacdias for landing this.

For anyone reading this in the future:

@lidel lidel merged commit 970b81d into main Sep 25, 2023
4 checks passed
@lidel lidel deleted the ipip/schema-peer branch September 25, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ratified
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants