-
Notifications
You must be signed in to change notification settings - Fork 103
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
routing/http!: support for IPIP-378 (delegated content and peer providing) #539
base: main
Are you sure you want to change the base?
Conversation
2dc9fee
to
14f79a9
Compare
14f79a9
to
ccda2a5
Compare
ccda2a5
to
a0b55c1
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #539 +/- ##
==========================================
+ Coverage 59.66% 59.69% +0.02%
==========================================
Files 238 237 -1
Lines 29883 30071 +188
==========================================
+ Hits 17831 17951 +120
- Misses 10433 10485 +52
- Partials 1619 1635 +16
|
3afb7a9
to
04714d1
Compare
There's still a few tests added, but I'm opening this as ready for review @lidel. It would be useful to go through the code and checking the TODOs that I added inline. |
b2e5365
to
5aa1e46
Compare
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.
Pushed some changes to IPIP-378 PR, tldr in ipfs/specs#378 (comment)
Some feedback on TODOs inline.
routing/http/client/client.go
Outdated
record := &types.AnnouncementRecord{ | ||
Schema: types.SchemaAnnouncement, | ||
Payload: types.AnnouncementPayload{ | ||
// TODO: CID, Scope not present for /routing/v1/peers, right? |
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.
Correct. But the fact you had to ask, means everyone implementing it will have to think about too 🤔
Should we reconsider and have two separate schemas instead of one (provider-record
and peer-record
) ?
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 I still think one is fine, I'm not exactly sure why I asked this. As long as it is stated in the specs, it is fine. I think it did not say anything when I wrote this.
I also see you introduced the error schema. That will make the code more complicated. Instead, I want to suggest a Announcement Response Schema
. It is a mix of the announcement schema and the error schema:
{
"Schema": "announcement-response",
"Error": "error in case there was error",
"TTL": 17280000
}
This perhaps allows better coupling between the announcement and its individual response. We can even add the CID and/or PID to have more context.
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.
@hacdias sounds like a sensible change. Are you able to apply this (remove error and andd this response one) to ipfs/specs#378?
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 made an update
8cdf7ce
to
2862dc2
Compare
e576b55
to
813b59f
Compare
also fixes an issue where NewPeerRecordsIter was actually not filtering the out the records with different schema. instead it was returning an empty result with no error. that could lead to errors.
770b75c
to
45d50f5
Compare
This is the client and server implementation for the delegated PUTs for peers and providers. See the specification on ipfs/specs#378. There are breaking changes. Check the changelog to know more about them.
Since the specification is not yet finished and set in stone, there are quite a few questions that I need answered in order to finish the code. They are written as TODO comments in line and I left them also in the specs PR.
TODO
, which are mostly related to specificationsProvide
(also test server)ProvidePeer
(also test server)Provide
outputProvidePeer
outputNot needed to merge this:
someguy
: feat: implement delegated PUTs someguy#40kubo
: feat: IPIP-378 (delegated provide over HTTP /routing/v1) kubo#10420ipni
: feat: update boxo with new Provide interface ipni/index-provider#441