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: BlockBroker factory support #284

Merged
merged 6 commits into from
Oct 13, 2023

Conversation

SgtPooki
Copy link
Member

Adds on to changes from #280 and #281

This makes it much easier to support constructing helia with custom blockBrokers while still allowing consumers easy access to the default BitswapBlockBroker. One particular use-case where this comes in handy: our tests.

Take the code in packages/helia/test/fixtures/create-helia.ts for example. Without these changes, you have to do something like:

import { webSockets } from '@libp2p/websockets'
import * as Filters from '@libp2p/websockets/filters'
import { MemoryBlockstore } from 'blockstore-core' // new import
import { MemoryDatastore } from 'datastore-core' // new import
import { circuitRelayTransport } from 'libp2p/circuit-relay'
import { identifyService } from 'libp2p/identify'
import { BitswapBlockBroker } from '../../src/block-brokers/index.js' // new import
import { createHelia as createNode } from '../../src/index.js'
import { defaultHashers } from '../../src/utils/default-hashers.js' // new import
import { createLibp2p } from '../../src/utils/libp2p.js' // new import
import type { Helia } from '@helia/interface'

export async function createHelia (): Promise<Helia> {
  const datastore = new MemoryDatastore()
  const blockstore = new MemoryBlockstore()
  const libp2p = await createLibp2p(datastore, {
    addresses: {
      listen: [
        `${process.env.RELAY_SERVER}/p2p-circuit`
      ]
    },
    transports: [
      webSockets({
        filter: Filters.all
      }),
      circuitRelayTransport()
    ],
    connectionGater: {
      denyDialMultiaddr: async () => false
    },
    services: {
      identify: identifyService()
    }
  })
  return createNode({
    blockstore,
    blockBrokers: [
      new BitswapBlockBroker(libp2p, blockstore, defaultHashers())
    ],
    datastore,
    libp2p
  })
}

However, with these changes, we can now do this:

import { webSockets } from '@libp2p/websockets'
import * as Filters from '@libp2p/websockets/filters'
import { circuitRelayTransport } from 'libp2p/circuit-relay'
import { identifyService } from 'libp2p/identify'
import { BitswapBlockBrokerFactory } from '../../src/block-brokers/bitswap-block-broker.js' // new import
import { createHelia as createNode } from '../../src/index.js'
import type { Helia } from '@helia/interface'

export async function createHelia (): Promise<Helia> {
  return createNode({
    blockBrokers: [
      BitswapBlockBrokerFactory
    ],
    libp2p: {
      addresses: {
        listen: [
        `${process.env.RELAY_SERVER}/p2p-circuit`
        ]
      },
      transports: [
        webSockets({
          filter: Filters.all
        }),
        circuitRelayTransport()
      ],
      connectionGater: {
        denyDialMultiaddr: async () => false
      },
      services: {
        identify: identifyService()
      }
    }
  })
}

Some key benefits:

  1. No need to construct libp2p ourselves and pass to BlockBrokers
  2. No need to construct our own blockstore for passing to BitswapBlockBroker
  3. No need to construct our own datastore for passing to createLibp2p
  4. additional hashers can be added easily to heliaInit config and passed to all blockBrokers.

@SgtPooki SgtPooki requested a review from a team as a code owner October 10, 2023 19:42
@SgtPooki SgtPooki changed the base branch from main to feat/block-providers-sgtpooki October 10, 2023 19:44
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

@@ -19,6 +20,7 @@ const log = logger('helia')
interface HeliaImplInit<T extends Libp2p = Libp2p> extends HeliaInit<T> {
libp2p: T
blockstore: Blockstore
blockBrokers: BlockBroker[]
Copy link
Member Author

Choose a reason for hiding this comment

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

needed to ensure it's not using the Array<BlockBroker | BlockBrokerFactoryFunction> type from HeliaInit

Comment on lines +166 to +171
return blockBroker({
blockstore,
datastore,
libp2p,
hashers
}) satisfies BlockBroker
Copy link
Member Author

Choose a reason for hiding this comment

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

are there other things that blockBrokers may need?

@@ -98,7 +98,7 @@ export interface HeliaInit<T extends Libp2p = Libp2p> {
* A list of strategies used to fetch blocks when they are not present in
* the local blockstore
*/
blockBrokers?: BlockBroker[]
blockBrokers?: Array<BlockBroker | BlockBrokerFactoryFunction>
Copy link
Member Author

Choose a reason for hiding this comment

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

do we just want to accept BlockBrokerFactoryFunction[] instead of BlockBrokers directly?

Comment on lines 10 to +13
return createNode({
blockBrokers: [
BitswapBlockBrokerFactory
],
Copy link
Member Author

Choose a reason for hiding this comment

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

We should not be making external network requests (i.e. not calling default trustlessGateways) during tests.

Comment on lines +78 to +88
/**
* A function that receives some {@link Helia} components and returns a
* {@link BlockBroker}.
*
* This is needed in order to re-use some of the internal components Helia
* constructs without having to hoist each required component into the top-level
* scope.
*/
export interface BlockBrokerFactoryFunction {
(heliaComponents: BlockBrokerFactoryComponents): BlockBroker
}
Copy link
Member Author

Choose a reason for hiding this comment

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

seemed like the best place for this but lmk if somewhere else is better.

@@ -15,7 +15,7 @@ import type { BitswapBlockBroker } from '../../src/block-brokers/bitswap-block-b
import type { Blockstore } from 'interface-blockstore'
import type { CID } from 'multiformats/cid'

describe('storage', () => {
describe('networked-storage', () => {
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 has the same describe namespace as packages/helia/test/storage.spec.ts and it bugs me

@@ -25,6 +25,7 @@ describe('pins (recursive)', () => {
dag = await createDag(codec, blockstore, 2, 3)

helia = await createHelia({
blockBrokers: [],
Copy link
Member Author

Choose a reason for hiding this comment

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

Prevent blockBroker networking when not needed

@@ -27,6 +27,7 @@ describe('pins (depth limited)', () => {
dag = await createDag(codec, blockstore, MAX_DEPTH, 3)

helia = await createHelia({
blockBrokers: [],
Copy link
Member Author

Choose a reason for hiding this comment

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

Prevent blockBroker networking when not needed

@SgtPooki SgtPooki merged commit a3260ff into feat/block-providers-sgtpooki Oct 13, 2023
15 checks passed
@SgtPooki SgtPooki deleted the fix/block-broker-testing branch October 13, 2023 02:04
achingbrain added a commit that referenced this pull request Oct 13, 2023
This approach is consistent with libp2p components, unixfs, ipns, etc.

Since #281 and #284 were merged without review, this PR implements
suggsestions that would have been in the review of those PRs.

1. Creation of block brokers is done by exported function. If your broker
    takes arguments, pass them to the factory function.  The factory then
    returns a function that accepts helia components and returns the
    broker.
2. Removes BitswapBrokerFactory as it is redunant

The internal API may need some more work but the external API should be
relatively stable.
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
Development

Successfully merging this pull request may close these issues.

1 participant