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 libp2p ContentRouting and PeerRouting #44

Merged
merged 7 commits into from
Oct 30, 2023

Conversation

achingbrain
Copy link
Member

Enables passing a client instance as a libp2p service which will detect it's ContentRouting and PeerRouting capabilities and configure them for use.

Obviates the need for modules like @libp2p/delegated-routing-v1-http-api-content-routing.

Enables passing a client instance as a libp2p service which will
detect it's `ContentRouting` and `PeerRouting` capabilities and
configure them for use.
@BigLep
Copy link

BigLep commented Oct 27, 2023

Conceptually makes sense to me. Thanks!

Maybe add a note about this in https://github.com/ipfs/helia-delegated-routing-v1-http-api/blob/main/packages/client/README.md or https://github.com/ipfs/helia-delegated-routing-v1-http-api/blob/main/README.md since this presumably be one of the main users of the client?

Copy link
Member

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

This is great. Just some comments around improvements for testing and documentation that could be made.

Comment on lines +106 to +107
export function createDelegatedRoutingV1HttpApiClient (url: URL | string, init: DelegatedRoutingV1HttpApiClientInit = {}): DelegatedRoutingV1HttpApiClient {
return new DefaultDelegatedRoutingV1HttpApiClient(new URL(url), init)
Copy link
Member

Choose a reason for hiding this comment

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

+1 for supporting strings

Copy link
Member Author

Choose a reason for hiding this comment

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

In general if we can push validation of an argument type out of a consumer it's a good thing. Otherwise this can snowball quickly and we have to guess what the contents of a string are, usually at several points in a module if we have multiple entry points.

We suffered from this previously with "what's a CID?" is it "QmFoo", is it "/ipfs/QMFoo", etc. Pushing all that into the CID class vastly simplified the codebase.

Of course for a user it's nicer to be able to pass 'http://example.org' instead of new URL('http://example.org') so it's probably worth the tradeoff for the create... factory functions.

Copy link
Member

Choose a reason for hiding this comment

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

this should be a straightforward one where it will fail quickly if an invalid string is passed.

throw new Error('PeerRouting not found')
}

await drain(routing.getClosestPeers(Uint8Array.from([0, 1, 2, 3, 4])))
Copy link
Member

Choose a reason for hiding this comment

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

the assertion here is not obvious. it seems like it should be either

Suggested change
await drain(routing.getClosestPeers(Uint8Array.from([0, 1, 2, 3, 4])))
expect(async () => await drain(routing.getClosestPeers(Uint8Array.from([0, 1, 2, 3, 4])))).toThrow()
// or
expect(async () => await drain(routing.getClosestPeers(Uint8Array.from([0, 1, 2, 3, 4])))).to.haveLength(0)

Copy link
Member Author

@achingbrain achingbrain Oct 30, 2023

Choose a reason for hiding this comment

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

I've added an assertion.

Note that you have to use the eventually keyword to make the kind of assertions you're trying to make above. See the chai-as-promised docs.

drain also has a return type of void (as it just empties the iterator) - to assert contents use all or just length if keeping all iterator contents around might make the process run out of memory:

// will not work because it'll try to assert that the promise (not the promised value)
// returned from the async function has a 'length' property of 0
expect(async () => await drain(routing.getClosestPeers(...))).to.haveLength(0)

// instead:
await expect(all(routing.getClosestPeers(...))).to.eventually.have.lengthOf(0)
// or:
await expect(all(routing.getClosestPeers(...))).to.eventually.be.empty()

// instead:
await expect(length(routing.getClosestPeers(...))).to.eventually.equal(0)

packages/client/test/routings.spec.ts Outdated Show resolved Hide resolved
packages/client/test/routings.spec.ts Outdated Show resolved Hide resolved
packages/client/test/routings.spec.ts Outdated Show resolved Hide resolved
packages/client/test/routings.spec.ts Outdated Show resolved Hide resolved
packages/client/test/routings.spec.ts Outdated Show resolved Hide resolved
packages/client/src/routings.ts Outdated Show resolved Hide resolved
packages/client/src/routings.ts Outdated Show resolved Hide resolved
@SgtPooki SgtPooki self-requested a review October 30, 2023 16:22
Copy link
Member

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

lgtm, thanks

Comment on lines +106 to +107
export function createDelegatedRoutingV1HttpApiClient (url: URL | string, init: DelegatedRoutingV1HttpApiClientInit = {}): DelegatedRoutingV1HttpApiClient {
return new DefaultDelegatedRoutingV1HttpApiClient(new URL(url), init)
Copy link
Member

Choose a reason for hiding this comment

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

this should be a straightforward one where it will fail quickly if an invalid string is passed.

@achingbrain achingbrain merged commit ddfff1b into main Oct 30, 2023
19 checks passed
@achingbrain achingbrain deleted the feat/add-libp2p-routings branch October 30, 2023 16:27
github-actions bot pushed a commit that referenced this pull request Oct 30, 2023
## [@helia/delegated-routing-v1-http-api-client-v1.1.0](https://github.com/ipfs/helia-delegated-routing-v1-http-api/compare/@helia/delegated-routing-v1-http-api-client-v1.0.1...@helia/delegated-routing-v1-http-api-client-v1.1.0) (2023-10-30)

### Features

* add support for libp2p ContentRouting and PeerRouting ([#44](#44)) ([ddfff1b](ddfff1b))

### Documentation

* update project config and readmes ([#47](#47)) ([31eb6f7](31eb6f7))
@SgtPooki SgtPooki mentioned this pull request Dec 6, 2023
github-actions bot pushed a commit that referenced this pull request Jun 20, 2024
## 1.0.0 (2024-06-20)

### ⚠ BREAKING CHANGES

* returns IPNS objects are from ipns@9.x.x
* bump multiformats from 12.1.3 to 13.0.0 (#75)
* method signatures have changed to be closer to the delegated http routing v1 API spec

### Features

* add support for libp2p ContentRouting and PeerRouting ([#44](#44)) ([ddfff1b](ddfff1b))
* allow skipping IPNS record validation ([#101](#101)) ([ec0ba89](ec0ba89))
* initial import ([d49fff6](d49fff6))

### Bug Fixes

* add CORS plugin ([5913f9d](5913f9d))
* conform to Delegated Routing V1 HTTP spec ([#41](#41)) ([41e7902](41e7902))
* conform to peer schema ([35f47da](35f47da))
* handle unparsable peer ids ([#118](#118)) ([9bdbe46](9bdbe46))
* increase listeners to silence node warnings ([#112](#112)) ([13f4084](13f4084))
* increase shutdown controller signal listeners ([#62](#62)) ([ab7afa7](ab7afa7))
* mark package as side-effect free ([551e0f2](551e0f2))
* PeerRecord Addrs and Protocols do not need to be optional ([#43](#43)) ([ec62768](ec62768))
* remove @helia/interface dep ([#59](#59)) ([aa8ffb8](aa8ffb8))

### Trivial Changes

* add or force update .github/workflows/js-test-and-release.yml ([#19](#19)) ([1af4a7a](1af4a7a))
* add project field to eslint config ([#40](#40)) ([7dac713](7dac713))
* add typedoc entry point ([5b16a12](5b16a12))
* delete templates [skip ci] ([#18](#18)) ([e1771ee](e1771ee))
* fix deps ([9aa177d](9aa177d))
* fix package name ([841d8f1](841d8f1))
* fix package name ([cd172da](cd172da))
* fix package name ([2c13d74](2c13d74))
* **release:** 1.0.0 [skip ci] ([a5b8290](a5b8290)), closes [#41](#41) [#40](#40) [#26](#26) [#8](#8) [#39](#39) [#26](#26) [#8](#8) [#39](#39) [#42](#42)
* **release:** 1.0.0 [skip ci] ([b027b54](b027b54)), closes [#41](#41) [#40](#40) [#26](#26) [#8](#8) [#39](#39) [#26](#26) [#8](#8) [#39](#39)
* **release:** 1.0.0 [skip ci] ([fd4871b](fd4871b))
* **release:** 1.0.0 [skip ci] ([867feb8](867feb8))
* **release:** 1.0.0 [skip ci] ([ed31f44](ed31f44))
* **release:** 1.0.1 [skip ci] ([39c92a5](39c92a5)), closes [#47](#47)
* **release:** 1.0.1 [skip ci] ([58d76be](58d76be)), closes [#43](#43)
* **release:** 1.0.1 [skip ci] ([7d3d708](7d3d708)), closes [#26](#26) [#8](#8)
* **release:** 1.0.1 [skip ci] ([4a7be80](4a7be80))
* **release:** 1.0.2 [skip ci] ([4214634](4214634)), closes [#48](#48)
* **release:** 1.0.2 [skip ci] ([8760907](8760907)), closes [#39](#39)
* **release:** 1.0.2 [skip ci] ([9b4b3e8](9b4b3e8)), closes [#26](#26) [#8](#8)
* **release:** 1.0.3 [skip ci] ([e8d3243](e8d3243)), closes [#49](#49)
* **release:** 1.0.3 [skip ci] ([1aa75e3](1aa75e3)), closes [#39](#39)
* **release:** 1.0.4 [skip ci] ([7e2bb0b](7e2bb0b)), closes [#58](#58)
* **release:** 1.0.5 [skip ci] ([5341fc1](5341fc1)), closes [#59](#59)
* **release:** 1.1.0 [skip ci] ([4f593ec](4f593ec)), closes [#44](#44) [#47](#47)
* **release:** 1.1.1 [skip ci] ([9ab3df4](9ab3df4)), closes [#58](#58)
* **release:** 1.1.2 [skip ci] ([1db87c0](1db87c0)), closes [#62](#62)
* **release:** 2.0.0 [skip ci] ([c9936ca](c9936ca)), closes [#75](#75) [#100](#100) [#75](#75) [#94](#94)
* **release:** 2.0.0 [skip ci] ([81281b6](81281b6)), closes [#75](#75) [#100](#100) [#75](#75) [#73](#73) [#77](#77) [#94](#94)
* **release:** 2.0.1 [skip ci] ([814f9ef](814f9ef)), closes [#91](#91)
* **release:** 2.0.1 [skip ci] ([7e4f169](7e4f169)), closes [#91](#91)
* **release:** 2.0.2 [skip ci] ([01f6b44](01f6b44))
* **release:** 2.0.3 [skip ci] ([0fdea42](0fdea42))
* **release:** 2.1.0 [skip ci] ([e3a2d8f](e3a2d8f)), closes [#101](#101)
* **release:** 3.0.0 [skip ci] ([be52e79](be52e79)), closes [#102](#102)
* **release:** 3.0.0 [skip ci] ([d2624dc](d2624dc)), closes [#102](#102)
* **release:** 3.0.1 [skip ci] ([a2f715a](a2f715a)), closes [#112](#112)
* **release:** 3.0.1 [skip ci] ([5f2faf6](5f2faf6)), closes [#104](#104)
* **release:** 3.0.2 [skip ci] ([6933d34](6933d34)), closes [#108](#108)
* **release:** 3.0.3 [skip ci] ([bfbba0f](bfbba0f)), closes [#112](#112)
* remove extra ci file ([#46](#46)) ([b130c17](b130c17))
* remove release from interop ([a3be11b](a3be11b))
* Update .github/pull_request_template.md [skip ci] ([c4fb76f](c4fb76f))
* Update .github/workflows/stale.yml [skip ci] ([30d5d80](30d5d80))
* Update .github/workflows/stale.yml [skip ci] ([a958569](a958569))
* Update .github/workflows/stale.yml [skip ci] ([b4f59b6](b4f59b6))
* update aegir ([c7c81b5](c7c81b5))
* update project config ([29bf459](29bf459))
* update project config ([#100](#100)) ([0bc6284](0bc6284))
* update sibling dependencies ([0c21765](0c21765))
* update sibling dependencies ([85cc65e](85cc65e))
* update sibling dependencies ([3fcc332](3fcc332))
* update sibling dependencies ([1b5d5dc](1b5d5dc))

### Documentation

* update project config and readmes ([#47](#47)) ([31eb6f7](31eb6f7))
* update readme examples to import correct symbols ([#58](#58)) ([bcfc785](bcfc785))
* update readmes and package descriptions ([11934bc](11934bc))

### Dependencies

* bump @fastify/cors from 8.5.0 to 9.0.1 ([#108](#108)) ([851b40f](851b40f))
* bump @libp2p/interface from 0.1.6 to 1.1.1 ([#91](#91)) ([50e4864](50e4864))
* bump helia from 1.3.12 to 2.0.1 ([#26](#26)) ([9160281](9160281))
* bump multiformats from 12.1.3 to 13.0.0 ([#75](#75)) ([1dabe16](1dabe16))
* bump p-queue from 7.4.1 to 8.0.1 ([#73](#73)) ([d575f73](d575f73))
* bump uint8arrays from 4.0.10 to 5.0.1 ([#77](#77)) ([e966c99](e966c99))
* **dev:** bump @helia/ipns from 5.0.0 to 6.0.0 ([#107](#107)) ([c4d9c8f](c4d9c8f))
* **dev:** bump @helia/ipns from 6.0.1 to 7.1.0 ([#110](#110)) ([bb10bf7](bb10bf7))
* **dev:** bump @types/sinon from 10.0.20 to 17.0.0 ([#49](#49)) ([c2d5bfb](c2d5bfb))
* **dev:** bump aegir from 39.0.13 to 40.0.8 ([#8](#8)) ([127bcc0](127bcc0))
* **dev:** bump aegir from 40.0.13 to 41.0.0 ([#39](#39)) ([c01a33e](c01a33e))
* **dev:** bump aegir from 41.3.5 to 42.1.1 ([#94](#94)) ([e34a142](e34a142))
* **dev:** bump helia from 3.0.1 to 4.0.0 ([#104](#104)) ([23cd638](23cd638))
* **dev:** bump sinon from 16.1.3 to 17.0.0 ([#42](#42)) ([43275c0](43275c0))
* **dev:** bump sinon-ts from 1.0.2 to 2.0.0 ([#48](#48)) ([7e0c7d3](7e0c7d3))
* update ipns to v9 ([#102](#102)) ([1097cb6](1097cb6))
* update sibling dependencies ([c2f7470](c2f7470))
* update sibling dependencies ([7d9cb15](7d9cb15))
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants