Skip to content
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

feat: customize ipns dnsResolvers #445

Merged
merged 14 commits into from
Feb 26, 2024
Merged

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Feb 21, 2024

Title

feat(verified-fetch): customize ipns dnsResolvers

Description

Adds the ability to customize dns resolvers to verified-fetch.

Users should then be able to add custom doh resolvers to helia-service-worker-gateway as mentioned at ipfs/service-worker-gateway#22

Notes & open questions

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

@SgtPooki SgtPooki requested a review from a team as a code owner February 21, 2024 18:25
Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self review

@SgtPooki SgtPooki self-assigned this Feb 21, 2024
Co-authored-by: Alex Potsides <alex@achingbrain.net>
@achingbrain achingbrain changed the title feat(verified-fetch): customize ipns dnsResolvers feat: customize ipns dnsResolvers Feb 21, 2024
The test was passing when ran in isolation, but not in the group. dns caching was causing problems so i customized the url
Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self review. ready for peers

dnsResolvers: [customDnsResolver]
})
// error of walking the CID/dag because we haven't actually added the block to the blockstore
await expect(verifiedFetch.fetch('ipns://some-non-cached-domain.com')).to.eventually.be.rejected.with.property('errors').that.has.lengthOf(0)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this aggregate error should probably have a length greater than zero 🤔

Looks like we're logging the error, but not returning them:

this.log.error('Error walking path %s', path, err)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, AggregateError has really poor DX as the default message logged to the console doesn't include any of the actual errors which can lead to spurious issues being reported and a lot of wasted time.

In other places where there's only one entry in .errors we unwind it to just throw that error instead.

In this case, if there's an error, it should be made available, otherwise no error should be thrown?

packages/verified-fetch/test/index.spec.ts Outdated Show resolved Hide resolved
packages/verified-fetch/test/index.spec.ts Outdated Show resolved Hide resolved
packages/verified-fetch/test/verified-fetch.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: Alex Potsides <alex@achingbrain.net>
Comment on lines 153 to 161
* const fetch = await createVerifiedFetch({
* gateways: ['https://trustless-gateway.link'],
* routers: ['http://delegated-ipfs.dev']
* }, {
* dnsResolvers: [
* dnsJsonOverHttps('https://my-dns-resolver.example.com/dns-json'),
* dnsOverHttps('https://my-dns-resolver.example.com/dns-query')
* ]
* })
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question here. Do we want to add dnsResolvers to the first object?

to stick to the pattern of "components in first object, options that alter how things work in the second" I think it would make sense in either place.. 🤔

Copy link
Member

@2color 2color Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dnsResolvers are deserving of the same importance as routers which makes the case to move it to the first object.

routers is currently used for IPNS routing (and CID routing in the future). Assuming that we see IPNS and DNSLink as being of equal importance dnsResolvers should live beside routers in my humble opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, customising dnsResolvers requires importing the constructors for dnsOverHttps and dnsJsonOverHttps which does reduce ergonomics.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'll still be optional either way, but I think I agree with you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe in the first argument, keeping the routers/gateways/resolvers together makes sense to me.

The only thing we have in the second arg is contentTypeParser, not 100% on why we have two args right now anyway!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at this for a bit and starting to change both args to a single object, but contentTypeParser has to be in the second obj, or we need to accept a possible .helia option in the first obj, or else users can't pass both a custom Helia & contentTypeParser

SgtPooki and others added 2 commits February 26, 2024 09:16
@SgtPooki SgtPooki merged commit 1ee6a4a into main Feb 26, 2024
18 checks passed
@SgtPooki SgtPooki deleted the feat/customize-dns-resolvers branch February 26, 2024 21:15
@achingbrain achingbrain mentioned this pull request Feb 26, 2024
@2color
Copy link
Member

2color commented Mar 6, 2024

I just realised we have another problem now. We cannot instantiate @helia/verified-fetch with a custom instance of Helia and also customising dnsResolver.

This came up in the SW Gateway, where we now are faced with a choice:

  1. Either we instantiate Helia with the IDBBlockstore in the service worker but have no way to customise the dnsResolvers
  2. We customise the dnsResolvers (along with gateways and routers) but cannot use the IDBBlockstore.

This is how they compare:

// option 1
  const helia = await createHeliaHTTP({
    blockstore,
    datastore,
    blockBrokers: [
      trustlessGateway({
        gateways: [...config.gateways, ]
      })
    ],
    routers: [...config.routers, 'https://delegated-ipfs.dev'].map(rUrl => delegatedHTTPRouting(rUrl))
  })

  const verifiedFetch = await createVerifiedFetch(helia, { contentTypeParser })

// ---- OR

// option 2
  const verifiedFetch = await createVerifiedFetch({
    gateways: [...config.gateways, 'https://trustless-gateway.link'],
    routers: [...config.routers, 'https://delegated-ipfs.dev'],
    dnsResolvers: ['https://delegated-ipfs.dev/dns-query'].map(dnsJsonOverHttps)
  }, {
    contentTypeParser
  })

This seems like something we'd want to address. Naively we could move dnsResolvers to the second parameter and frame it as follows: the first config param is to customise Helia or pass a Helia instance, while the second is to customise verified-fetch specific config.

Any thoughts @SgtPooki @achingbrain ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

3 participants