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: add support for arbitrary service modules #1563

Merged
merged 6 commits into from
Apr 29, 2023

Conversation

saul-jb
Copy link
Contributor

@saul-jb saul-jb commented Jan 18, 2023

It would be nice if libp2p support arbitrary modules so that they can automatically be initialized with the libp2p instance so this PR adds this functionality. This is useful because there are many modules one might want to use that do not fit in the predefined categories and it allows them to add those modules alongside the standard ones.

This PR adds a configuration option to the createLibp2p method allowing you to add custom modules:

const node = await createLibp2p({
  // Normal libp2p modules
  // ...

  // Custom libp2p modules
  modules: {
    myModule: createMyModule(options)
  }
})

// Use 'myModule' later on.
node.modules.mymodule.doSomething();

@p-shahi p-shahi added this to the Best Effort Track milestone Jan 24, 2023
Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

This makes sense to me, but docs should be updated^^
I'm also curious about what @achingbrain thinks about this^^

@achingbrain
Copy link
Member

I like this idea and would ideally extend it to other components that are or should be optional.

For example in helia I'm creating disposable dial-only libp2p nodes for use as an RPC client, so they don't need the protocols that are enabled by default - identify, ping etc. Identify in particular causes loads of noise in the logs as the connection tends to get torn down before identify has completed so it'd be great to not have that enabled on the dial-only node at all.

Also I think there was interest from lodestar and others to generally have a more minimal libp2p with a smaller surface area.

Given that we have things like FetchService and PingService already and not module, I'm open to ideas but I think the API I'd like to see is something like:

const libp2p = await createLibp2p({
  transports: [
    tcp()
  ],
  connectionEncryption: [
    noise()
  ],
  streamMuxers: [
    yamux()
  ],
  services: [
    ping(),
    identify(),
    fetch(),
    nat(),
    myCustomService()
  ]
})

// then later
const value = await libp2p.services.fetch(someKey)
const foo = await libp2p.services.myCustomService.someMethod(bar)

We'd then remove .fetch, .ping etc from the libp2p interface as you'd access them via the .services key.

I'm not sure how this would work with the types though? As in, how to derive the type of the .services key from the configuration passed to createLibp2p?

@mpetrunic
Copy link
Member

I'm not sure how this would work with the types though? As in, how to derive the type of the .services key from the configuration passed to createLibp2p?

With your example, it's probably impossible, but with original proposal where you pass service map it could be done

@saul-jb
Copy link
Contributor Author

saul-jb commented Jan 27, 2023

I have updated the API to use a services configuration field instead of modules and I have gotten types to work for these services.

const libp2p = await createLibp2p({
  transports: [
    tcp()
  ],
  connectionEncryption: [
    noise()
  ],
  streamMuxers: [
    yamux()
  ],
  services: {
    ping: ping(),
    identify: identify(),
    fetch: fetch(),
    nat: nat(),
    myCustomService: myCustomService()
  }
})

const value = await libp2p.services.fetch(someKey)
const foo = await libp2p.services.myCustomService.someMethod(bar)

Note that I have expanded the return type of createLibp2p from Libp2p to include the services property but this should probably be added to the @libp2p/interface-libp2p interface.

@achingbrain, if it were possible to have services as an array instead of a map and still have types work, how would we reference a service from the node by name? E.g. how would the following array map to the following object:

[ ping(), fetch() ] => { ping: (service), fetch: (service) }

Would services be required to return an object with a property that identities the name of the service? If this were the case you could also require they have a type property the identifies the type of service (or perhaps just infer it) and it might be possible to move transports, muxers, etc. to the services list.

@achingbrain
Copy link
Member

This is a good idea and it's something we're going to do, I just shy away from it a little because it's going to be quite disruptive. Maybe after the current round of breaking changes.

@p-shahi
Copy link
Member

p-shahi commented Mar 21, 2023

@saul-jb This PR was discussed in today's js-libp2p Triage meeting. In terms of timeline, we are thinking of including this in a js-libp2p release after IPFS Thing (April 15-19). Until then, we want to limit disruptive changes to js-libp2p and keep focus on bugfixes/hardening and general WebRTC related upkeep (new features landing soon in js-libp2p-webrtc).
Thanks for your patience!

@saul-jb
Copy link
Contributor Author

saul-jb commented Apr 23, 2023

@achingbrain, @p-shahi, What is the state of this, is there anything more I can do to help?

@p-shahi
Copy link
Member

p-shahi commented Apr 23, 2023

@saul-jb Per this comment #1707 (comment) @achingbrain intends to include this in the upcoming release (0.45.0)

@achingbrain achingbrain mentioned this pull request Apr 25, 2023
achingbrain added a commit to libp2p/js-libp2p-interfaces that referenced this pull request Apr 26, 2023
When libp2p is [configured with arbitrary services](libp2p/js-libp2p#1563)
some of those services may be content routers or peer routers or
both.  An example of this is the `@libp2p/kad-dht` module.

In order to communicate to libp2p that they can provide content/peer
routing cabapiliy, add well-known symbols that libp2p can use to
get references to the routing implementations.
achingbrain added a commit to libp2p/js-libp2p-interfaces that referenced this pull request Apr 27, 2023
When libp2p is [configured with arbitrary services](libp2p/js-libp2p#1563)
some of those services may be content routers or peer routers or
both.  An example of this is the `@libp2p/kad-dht` module.

In order to communicate to libp2p that they can provide content/peer
routing cabapiliy, add well-known symbols that libp2p can use to
get references to the routing implementations.
achingbrain added a commit to libp2p/js-libp2p-kad-dht that referenced this pull request Apr 27, 2023
The DHT interface is similar to the content/peer routing interfaces
but different.

In order to detect content/peer routers and peer discovery implementations
in [arbitrary libp2p modules](libp2p/js-libp2p#1563)
use the exported symbols to create getters for them.

The DHTContentRouting and DHTPeerRouting classes have been ported over
from libp2p.
achingbrain added a commit to libp2p/js-libp2p-kad-dht that referenced this pull request Apr 27, 2023
The DHT interface is similar to the content/peer routing interfaces
but different.

In order to detect content/peer routers and peer discovery implementations
in [arbitrary libp2p modules](libp2p/js-libp2p#1563)
use the exported symbols to create getters for them.

The DHTContentRouting and DHTPeerRouting classes have been ported over
from libp2p.
@achingbrain achingbrain force-pushed the arbitrary-modules branch 2 times, most recently from 803609b to c071ddf Compare April 27, 2023 17:44
@achingbrain achingbrain changed the title feat: Arbitrary/Custom Modules feat: add support for arbitrary service modules Apr 27, 2023
Updates the libp2p init args to accept an object of service
factory functions that can use internal libp2p components.

The returned libp2p object has a `.services` key that corresponds
to the service factory keys.
src/libp2p.ts Outdated Show resolved Hide resolved
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

This is going to really improve the flexibility, security and size of js-libp2p so thanks a lot for this @saul-jb and @achingbrain . It looks good to me, I only have two comments. I also wonder if we should include #1684 in this PR but it's not a requirement to land this.

.aegir.js Show resolved Hide resolved
src/components.ts Outdated Show resolved Hide resolved
Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

LGTM
I didn't check it out and run it, but just looking at the structure it looks good.

Would still love IdentifyService#identify to return the result but I guess thats a separate discussion.

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

Successfully merging this pull request may close these issues.

6 participants