-
Notifications
You must be signed in to change notification settings - Fork 231
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-379: Delegated IPNS HTTP API #379
Conversation
9a19add
to
6414a81
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.
Thank you for submitting this @masih 🙏
Dropped quick feedback around JSON and Signatures, and including canonical serialization protobuf format as mandatory part of spec (need to support canonical serialization because entire ecosystem expects that).
We could also have JSON as default for quick eyeballing, don't feel strongly about it nor find much utility: it feels like a distraction/security UX/DX footgun but lmk if you had some specific use cases that depend on introducing this additional JSON serialization format
221319f
to
c5c9649
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.
@masih apologies for slow turnaround on this one, wanted to give it a propser thought when i have more time, but it never happened :(
To move things forward, I gave it another pass.
I am ok with making JSON as human-readable as you want – we already made that call in existing /routing/v1/providers
, but we can't compromise on signature.
My proposal is to reuse signature and Record Verification steps from the IPNS spec and avoid inventing new things to keep the number of things IPNS implementer needs to do at minimum – details inline.
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 i'd like to pick this up and move forward. Are you ok with changes suggested above?
Especially my concerns around signatures in JSON (#379 (comment)) which lack useful spec or use case (who will be using JSON?). Perhaps we could remove JSON from this IPIP for now, and add it in future IPIP, if anyone has use case / need for it?
FYSA me and @hacdias plan to support IPNS GET and PUT in Kubo if cid.contact intents to support it.
- Kubo will use the original
application/vnd.ipfs.ipns-record
for both GETs or PUTs, we will never use JSON.- The ipns-record protobuf from the IPNS spec is what other already implemented IPNS routers already use (DHT, ipns over pubsub), and it is true for the rest of ecosystem too:
?format=ipns-record
on Gateways, we also have multicodec and IANA registration pending -- all use the well-definedapplication/vnd.ipfs.ipns-record
.
- The ipns-record protobuf from the IPNS spec is what other already implemented IPNS routers already use (DHT, ipns over pubsub), and it is true for the rest of ecosystem too:
@lidel please go ahead we can iterate on remaining issues in separate IPIPs thanks 🙏 |
Ok! We will circle back to this when @hacdias is back next week and we will make it limited to TODOs:
|
49fef6c
to
f6fedc5
Compare
@lidel I rebased the PR and made the following updates:
|
f6fedc5
to
e1b937e
Compare
d9819e1
to
8d73d72
Compare
1aca148
to
204c4d7
Compare
8ff9b91
to
8394e23
Compare
- suggestions from code review - removes application/json from ipip-379 - removes custom headers from ipip-379 - cleans duplicated errors - punctuation and others
8394e23
to
4a03ecd
Compare
9e74c45
to
b1149d8
Compare
b1149d8
to
6d49dfb
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.
Applied final editorial tweaks, this looks ready from my end.
Moving to "ready for final review" stage.
Added to the agenda of IPFS Implementers WG call on 2023-06-01 but don't expect issues, since we've reduced the scope to the bare minimum:
/routing/v1/ipns
GET
andPUT
of serialized IPNS record in existing format, no special headers, no JSON, no streaming
I opened a Boxo implementation PR here: ipfs/boxo#333 |
No concerns raised during final ratification during IPFS-Implementers-Sync-2023-06-15, ratified, merging. |
Propose HTTP APIs for delegated naming system that enables offloading of naming system to other processes or servers.
Relates to:
Fixes #343