feat: add protocol list to ipfs id by tk26 · Pull Request #3250 · ipfs/js-ipfs

tk26 · 2020-08-27T04:57:03Z

addresses issue #3219

welcome · 2020-08-27T04:57:05Z

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

The context for this PR is clear, with relevant discussion, decisions
and stakeholders linked/mentioned.
Your contribution itself is clear (code comments, self-review for the
rest) and in its best form. Follow the code contribution
guidelines
if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

A maintainer will triage and assign priority to this PR, commenting on
any missing things and potentially assigning a reviewer for high
priority items.
The PR gets reviews, discussed and approvals as needed.
The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

tk26 · 2020-08-27T04:59:35Z

I can update the test if the change looks good and makes sense. (was having some issue running the test locally)

achingbrain · 2020-08-27T09:51:32Z

Thanks for opening this, it looks great.

To get this over the line it needs:

Exposing in the HTTP API /id/ endpoint
Mapping in the HTTP API Client (this may not be necessary as it looks like the protocols will just come through as a string array)
Tests added to the id interface test suite
Those tests skipped in the HTTP API Client suite if not implemented in a shipping version of go-ipfs yet

tk26 · 2020-08-28T14:56:18Z

@achingbrain Thanks, done! please let me know how it looks.

achingbrain · 2020-08-28T17:14:30Z

The PR looks ok but the code doesn't seem to work - that is the protocols array is always empty whether the node is started or not.

@jacobheun @vasco-santos is the peer store the right place to be fetching supported protocols from for the running node?

achingbrain · 2020-08-28T17:17:06Z

packages/interface-ipfs-core/src/miscellaneous/id.js

+    it('should have protocols property', async () => {
+      const res = await ipfs.id()
+
+      expect(res).to.have.a.property('protocols').that.is.an('array')


This should assert that there are protocols present, at a minimum I think /ipfs/id/1.0.0 should be supported

There should be a handful of protocols, I'd assert them all as they shouldn't change very frequently. There may be differences in Node.js vs browser, depending on modules included, but I think they are currently equivalent.

Got ["/floodsub/1.0.0", "/ipfs/bitswap/1.0.0", "/ipfs/bitswap/1.1.0", "/ipfs/bitswap/1.2.0", "/ipfs/id/1.0.0", "/ipfs/id/push/1.0.0", "/ipfs/ping/1.0.0", "/libp2p/circuit/relay/0.1.0", "/meshsub/1.0.0", "/meshsub/1.1.0"], adding those to the assertions. Thanks guys!

tk26 · 2020-08-28T23:11:01Z

The PR looks ok but the code doesn't seem to work - that is the protocols array is always empty whether the node is started or not.

@jacobheun @vasco-santos is the peer store the right place to be fetching supported protocols from for the running node?

Thanks for checking. AFAIK peerstore seems to be having it (I referenced https://github.com/ipfs/go-ipfs/blob/master/core/commands/id.go#L154)
Also I had tried to run browser-script-tag example project and the protobook API seems to work fine.

jacobheun · 2020-08-31T11:00:34Z

packages/ipfs/src/core/components/id.js

    if (libp2p) {
      // only available while the node is running
      addresses = libp2p.transportManager.getAddrs()
+      protocols = libp2p.peerStore.protoBook.get(peerId) || []


Self protocols aren't currently stored in the peer store, so they need to be retrieved from the upgrader for now.

Suggested change

protocols = libp2p.peerStore.protoBook.get(peerId) || []

protocols = Array.from(libp2p.upgrader.protocols.keys())

Done! Thanks @jacobheun

jacobheun · 2020-08-31T11:03:36Z

packages/interface-ipfs-core/src/miscellaneous/id.js

+    it('should have protocols property', async () => {
+      const res = await ipfs.id()
+
+      expect(res).to.have.a.property('protocols').that.is.an('array')


There should be a handful of protocols, I'd assert them all as they shouldn't change very frequently. There may be differences in Node.js vs browser, depending on modules included, but I think they are currently equivalent.

achingbrain · 2020-09-02T10:22:20Z

Thanks again for submitting this, it'll go out in the next release.

go-ipfs lets you use the id command to fetch metadata about other nodes on the network - would you like to have a go at implementing that for js-ipfs too?

I think you should be able to use the protoBook, addressBook etc on the peerStore to do this, like how you'd originally implemented this PR.

tk26 · 2020-09-03T15:28:01Z

Thanks again for submitting this, it'll go out in the next release.

go-ipfs lets you use the id command to fetch metadata about other nodes on the network - would you like to have a go at implementing that for js-ipfs too?

I think you should be able to use the protoBook, addressBook etc on the peerStore to do this, like how you'd originally implemented this PR.

@achingbrain Thanks for merging this. Sounds interesting, I can take a look at id command for fetching metadata of other nodes.

Adds `.protocols` property to the output of `ipfs.id` which contains all libp2p protocols supported by the current node. Closes ipfs/js-ipfs#3219

achingbrain reviewed Aug 28, 2020

View reviewed changes

jacobheun reviewed Aug 31, 2020

View reviewed changes

tk26 added 4 commits September 1, 2020 09:05

feat: add protocol list to ipfs id

8f72202

Add protocols to http id

65e299d

Fallback to empty array

fe5c397

Add test to inteface and skip it

89a3691

achingbrain mentioned this pull request Sep 1, 2020

feat: return supported protocols in id output ipfs/kubo#7409

Merged

Address code review comments

d282a16

tk26 force-pushed the feat/id-list-protocols branch from dcb63c9 to d282a16 Compare September 1, 2020 16:36

Fix Linting issues

abb8d69

achingbrain merged commit 1b6cf60 into ipfs:master Sep 2, 2020

tk26 deleted the feat/id-list-protocols branch September 3, 2020 15:22

ljedrz mentioned this pull request Sep 18, 2020

Add the list of supported libp2p protocols to /id rs-ipfs/rust-ipfs#381

Draft

snyk-bot mentioned this pull request Jan 10, 2021

[Snyk] Security upgrade ipfs from 0.25.4 to 0.50.0 rtpro/js-ipfs#32

Open

snyk-bot mentioned this pull request Jan 17, 2021

[Snyk] Security upgrade ipfs from 0.25.4 to 0.50.0 rtpro/js-ipfs#33

Open

snyk-bot mentioned this pull request Apr 27, 2021

[Snyk] Security upgrade ipfs from 0.25.4 to 0.50.0 rtpro/js-ipfs#38

Open

snyk-bot mentioned this pull request May 5, 2021

[Snyk] Security upgrade ipfs from 0.25.4 to 0.50.0 rtpro/js-ipfs#40

Open

SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022

feat: add protocol list to ipfs id (#3250)

b075732

Adds `.protocols` property to the output of `ipfs.id` which contains all libp2p protocols supported by the current node. Closes ipfs/js-ipfs#3219

	protocols = libp2p.peerStore.protoBook.get(peerId) \|\| []
	protocols = Array.from(libp2p.upgrader.protocols.keys())

Conversation

tk26 commented Aug 27, 2020

Uh oh!

welcome bot commented Aug 27, 2020

Uh oh!

tk26 commented Aug 27, 2020

Uh oh!

achingbrain commented Aug 27, 2020

Uh oh!

tk26 commented Aug 28, 2020

Uh oh!

achingbrain commented Aug 28, 2020

Uh oh!

achingbrain Aug 28, 2020

Choose a reason for hiding this comment

Uh oh!

jacobheun Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

tk26 Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tk26 commented Aug 28, 2020

Uh oh!

jacobheun Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

tk26 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

jacobheun Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

achingbrain commented Sep 2, 2020

Uh oh!

tk26 commented Sep 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tk26 Sep 1, 2020 •

edited

Loading