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.
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-378: Delegated Routing HTTP POST API #378
base: main
Are you sure you want to change the base?
IPIP-378: Delegated Routing HTTP POST API #378
Changes from 1 commit
6336689
68c0dff
5bc8b14
4db804a
d9287e5
0f74972
82aa9d9
921b0b6
94429a5
05b000e
30a5e3b
7b32f53
fbefb1b
f6f0d9b
cf83c02
da12e11
05dceba
ccbc085
a69d2b8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
💭 this section grows way faster than the rest of the spec.
Would it be ok to move "Known Transfer Protocols" to a separate file (
KNOWN_TRANSFER_PROTOCOLS.md
)?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 recommend doing this in a separate PR to reduce lines changes what are irrelevant to what this PR is trying to introduce.
If this is not a blocker for this PR, I'd be happy to take this as a TODO and get it done once this PR is merged.
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.
💭 this creates one
bitswap
schema forGET
and the otherbitswap
forPUT
. Both under the same name, but with different fields. Bit nasty and error prone.@masih is this ossified, or is there still time to fix this and use different schema for PUTs?
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.
There is still time. Changing thanks
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.
@masih is cid.contact doing verification of this?
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.
cid.contact does not support any PUT over http delegated routing.
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.
@masih what is the max length of this list?
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.
My thinking is to make the maximum length be derived from whatever the server supports as max request body. A user should be able to find this out via
OPTIONS
request usually?If that makes sense I am happy to document this in the spec. Unless you think we need some explicit max length for that array?
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.
What is the format? Unix epoch? What resolution (ms? ns?)
Would ASCII string that follows notation from [rfc3339] (
1970-01-01T00:00:00.000000001Z
) be less ambiguous?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.
Agreed on following standardized format that is easily human-parseable and doesn't have ambiguity.
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.
Is this a timestamp of a duration? If a duration, is it miliseconds?
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.
If a duration, maybe get that into the name.
Also embed the units in the parameter name so there is no ambiguity?
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.
💭 there are 3 peerid types, two of them are legacy (see the end of this spec).
I'd like to avoid legacy from
/routing/v1
and use CIDv1 with libp2p-key codec everywhere. @masih any concerns around this?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.
No concerns 👍
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.
Any reason why we don't include
PublicKey
field from the start?If we make it an optional, opaque bytes field, and say it should be deserialized as the protobuf from libp2p peerid specs, we have both solved RSA problem, and don't need to maintain
PublicKeyType
registry.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.
Alternatively, we could include
PublicKey
in thepeer
schema on out-of-band GET to/routing/v1/peers/{peerid}
(IPIP-417).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'm okay with either of these paths.
@masih are you okay with this suggestion?
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.
Seems fine to me 👍
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.
If we have to mangle
Payload
JSON anyway, what is the benefit of introducing it as a new struct?It was probably discussed before elsewhere, but why can't we use exactly the same protobuf and signature DHT uses?
Reusing DHT record payload would save CPU time on IPFS nodes that publish to both DHT and IPNI (signature would be generated only once, not twice).
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 suspect the reason for this is better human readability? The beginning of this document (merged as part of IPIP-337 states "human-readable encodings of types are preferred".
We can make the same argument about HTTP delegated routing itself. Unless there is evidence to suggest that we should be heavily optimising this, my vote would be to favour human readability.
@guseggert what are your thoughts?
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.
@lidel can I ask where I can find the DHT record payload specification? I think we can add this to the spec via a specific request
Content-Type
media type, similar to #379.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.
@guillaumemichel this is related to #345 – can you point us where up-to-date protobufs related to CID announcements on DHT lives these days? Unsure how feasible it is to reuse the announcement's wire format here.
Could we write down and publish the basic wire format information like protobuf section about ipns record, so this IPIP can refer to spec website, and not some go code 🙏
@masih I think
Content-Type
reuse similar to what we did #379 would help in some cases, but not a silver bullet here. There is way way more CIDs than IPNS records, so some way of doing bulk PUT is still desired, but it should not be tied to a single protocol.When IPFS node speaks more than bitswap (we already plan to do HTTP next to bitswap in many places), doing a PUT of the same CIDs once for every protocol duplicates the cost on the client.
I don't think there is anything special about
Schema: bitswap-announce
, having dedicated PUT for this one protocol feels odd, PUT for HTTP will look exactly the same.Maybe instead of having
Schema: bitswap-announce
we should haveSchema: provider-announce
withProtocols
list, which could then have both bitswap and HTTP?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 agree 👍
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.
After IPIP-417 we are in a better place now.
I think we should avoid protocol-specific schema for bitswap and use peer schema from IPIP-417. It allows for passing optional protocol-specific metadata for things other than bitswap, allowing client to do single PUT with all info.