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
Spec proposal for Reframe #272
Spec proposal for Reframe #272
Changes from all commits
9440c8d
d497d2c
3bfb500
0ef44ee
bcf7d2e
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.
I'd like to meekly suggest
code String
here.A bikeshed to be sure, but that simple change would make it "just so happen" to align perfectly with the Serum convention. (It seems conceptually already perfectly aligned.)
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 don't have super strong feelings over whether this is
Bytes
,&Any
or some union.I've heard in the past strong feelings about exposing CIDs rather than multihashes in various APIs even when it is likely that only the multihash component of the CID will be utilized. Given that we'd have to give some clarifying information about what to do in this situation anyway writing that here seemed reasonable.
If some folks want to use this method for rendezvous without hashing they can by just using an identity multihash which only costs a few descriptor bytes (e.g. [0x01,0x55,0x00,(length varint)])
If people have strong feelings to the contrary let's talk about it
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
is a fine way to describe the CID case.I feel like maybe a union is indeed gonna be worth inserting here, though. Possibly even a keyed one. If I imagine myself writing destructuring code for handling a message like this, I'd probably want a pretty explicit logical marker for if the thing coming is a content identifier or a pubsub topic (or whatever else).
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.
Whether these should be optional or an empty set seems debatable. I went with the one that allows for sending back fewer bytes since it seemed easy enough
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.
When should someone use this field, what protocols should they be supporting at minimum? IIUC there are multiple versions of
/ipfs/graphsync
and/fil/data-transfer
that exist. Does this cover all of them via protocol negotiation (similar to the Bitswap case) or is this meant to cover a more limited subset?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 describes the more limited implementation of graphsyc/go-data-transfer using the voucher convention defined in the current filecoin fip spec
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 think for now this is ok. It might be that in the future we want to return other things like if the server knows about a more recent/better IPNS record (which is what the DHT code does), however that kind of situation also seems like something that'd be a reasonable error (i.e. indicating that the put is too old to be valid and that you can do a Get if you want the latest value).
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 don't know if within IPLD land we have any precedence for this type of namespacing we want to be consistent with.
In libp2p land we use paths (e.g.
/ipfs/bitswap/1.2.0
) and HTTP land has X Headers. Anything different fromMyProject-...
we want to encourage here @mvdan?