-
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
Publish existing Bitswap specs at specs.ipfs.tech #437
Conversation
I've created Preview: @aschmahmann any concerns or ok to merge? |
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.
Mostly LGTM, one small change requested (removing the unnecessary text after switching the section from content routing -> routing). The rest are optional.
Thanks for doing this 🙏
src/exchange/bitswap.md
Outdated
} | ||
``` | ||
|
||
## Implementations |
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 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 know several, but I am not familiar enough with either to be able to endorse one.
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 there are multiple I think fine to include them.
Just would prefer to not add in abandoned, non functional, etc. implementations where people will likely be better off not relying on them. People can still find those with a search engine if they want to build an implementation and see where others left off.
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.
@b5 @dignifiedquire do you feel we could include bitswap crate from Beetle here (in this PR or future one), or is it not something you plan to maintain going forward?
src/bitswap-protocol.md
Outdated
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.
@achingbrain: any learnings from maintaining https://github.com/ipfs/js-ipfs-bitswap?
@hannahhoward @gammazero @masih: I know you've been using bitswap (1.2.0?) for retrieval in Lassie and for content routing in Cassette this year.
This is the revamped spec for basics of Bitswap Protocol (it does not include things like Bitswap Sessions, which are implementation-specific).
Is there anything you feel is missing from this document? Any "Notes for implementers" we could/should include as an Appendix to this document, while we are at it?
If not, perfectly ok, just thought I'll send a ping in your direction in case there was something you've noticed.
incl. cosmetic editorials
it does both exchange and routing
this allows us to have separate spec for sessions if we want to write 'rules of thumb' at some time in the future
2023-10-17 maintainer conversation: @aschmahmann will approve and @lidel will merge |
As per what I discussed with @lidel, this PR moves the current Bitswap specs to the website. Merge should be done using rebase or merge commit in order to preserve the history for the Bitswap specifications.
Note that I put this under
architecture
. We may want to either create a new section, or put it somewhere else.