Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented May 23, 2025

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 the options 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)

const url = `ipfs://${cid}?${query}`
const resp = await verifiedFetch.fetch(url, {
  allowProviderParameter: true
})

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

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@vasco-santos vasco-santos force-pushed the feat/support-provider-query-parameter branch 2 times, most recently from 9a4991e to 997cf3a Compare May 23, 2025 15:41
blockBrokerRetrieveCalledWithProviders.resolve(options.providers)

// attempt to read from the provider
// eslint-disable-next-line
Copy link
Member Author

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 😬

@vasco-santos vasco-santos force-pushed the feat/support-provider-query-parameter branch 4 times, most recently from b7df559 to b7b8dc4 Compare May 23, 2025 19:40
@vasco-santos vasco-santos force-pushed the feat/support-provider-query-parameter branch from b7b8dc4 to c30629a Compare May 23, 2025 21:03
@vasco-santos vasco-santos marked this pull request as ready for review May 23, 2025 21:13
@vasco-santos vasco-santos requested a review from a team as a code owner May 23, 2025 21:13
Comment on lines +302 to +304
// see https://github.com/vasco-santos/provider-hinted-uri
// provider is a special case, the parameter MAY be repeated
if (key === 'provider') {
Copy link
Member

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:

  1. Open PR with IPIP memo in https://github.com/ipfs/specs/ that describes rationale for new feature:
  2. 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
  3. Merge the spec once no concerns were raised and after both implementations shipped and work the same.

Copy link
Member Author

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:

  1. Make provider query parameter in helia be behind a flag
  2. Draft PR with IPIP to specs with query parameter (no protocol encoding for now)
  3. 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:

  1. Add this to Kubo/Go side
  2. Make it the default + Finish/Merge the IPIP
  3. (Eventually) extend this with protocol encoding in multiaddr

Copy link
Member Author

@vasco-santos vasco-santos May 26, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants