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 1 commit 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.

Notes & open questions

Currently does not embed proto in the multiaddrs (i.e. /retrieval/bitswap/retrieval/http) as this is pending multiformats/js-multiaddr#418 until it can be created such multiaddr. I can follow up adding those here when supported, but in reality they are not actually needed here as they should be used by helia's block brokers to better decide hints to use.

In addition, once multiformats/js-multiaddr#418 is merged/released, we can get a PR in block brokers to check if multiaddrs have retrieval hints in the providers to skip/prioritize them

@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.

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