-
Notifications
You must be signed in to change notification settings - Fork 11
feat: support provider query parameter #242
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
base: main
Are you sure you want to change the base?
Conversation
9a4991e
to
997cf3a
Compare
blockBrokerRetrieveCalledWithProviders.resolve(options.providers) | ||
|
||
// attempt to read from the provider | ||
// eslint-disable-next-line |
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.
eslint complains there is too many nested callbacks 😬
b7df559
to
b7b8dc4
Compare
b7b8dc4
to
c30629a
Compare
// see https://github.com/vasco-santos/provider-hinted-uri | ||
// provider is a special case, the parameter MAY be repeated | ||
if (key === 'provider') { |
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.
Since this will impact verified-fetch and inbrowser.link
, are you planning to open IPIP about ?provider
in https://github.com/ipfs/specs/pulls?
I would softly block on having a spec PR that we can comment on :)
FYI the usual path towards extending Gateway interface (implemented by verfified-fetch) is to follow light IPIP process:
- Open PR with IPIP memo in https://github.com/ipfs/specs/ that describes rationale for new feature:
- older example: IPIP-402: Partial CAR Support on Trustless Gateways specs#402
- recent example: IPIP-484: Opt-in Filtering on Routing V1 HTTP API specs#484
- This does not have to be perfect, CI does not need to be gree, for now just open a Draft PR that documents the new parameter and syntax of accepted values so we have place to comment and discuss. Fine to copy your existing content from https://github.com/vasco-santos/provider-hinted-uri/blob/main/EXPLORATION.md#query-parameter-provider (the
?provider
alone, without/retrieval/
)
- Create implementation PR in GO (https://github.com/ipfs/boxo/tree/main/gateway used by Rainbow and Kubo/Desktop) and JS (Helia/verified-fetch) to ensure the spec is clear enough to be implemented in two different places
- Merge the spec once no concerns were raised and after both implementations shipped and work the same.
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.
For visibility, we discussed out of bound and agreed on a first stage:
- Make provider query parameter in helia be behind a flag
- Draft PR with IPIP to specs with query parameter (no protocol encoding for now)
- Enable inbrowser.link to be able to use this on its configuration
Hopefully, we can then iterate a next step after user validation and feedback for:
- Add this to Kubo/Go side
- Make it the default + Finish/Merge the IPIP
- (Eventually) extend this with protocol encoding in multiaddr
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.
Here is the IPIP draft to the specs repo ipfs/specs#504
…eter as experimental
183cf99
to
03a573d
Compare
Add support for provider query parameter
Description
This PR propagates provider from query parameters as described in https://github.com/vasco-santos/provider-hinted-uri/blob/main/EXPLORATION.md#-uri-design to be used as Hints by Helia's blockBrokers.
Regarding changes, it simply changes
parseUrlString
util function to verify if valid multiaddrs are in the query parameters and also returns them. They are then simply propagated to theoptions
parameter that makes its way to the plugins pipeline, where a broker session is created and is used.These feature is currently behind a feature flag while it is in experimental stage (
allowProviderParameter
)Notes & open questions
Currently does not embed proto in the multiaddrs (for instance
/retrieval/bitswap/retrieval/http
). We will as follow up discuss how to support something along these lines.@achingbrain while this is more experimental, opted to not add example in Readme. Should I add though?
Change checklist