Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: add protocol list to ipfs id #3250

Merged
merged 6 commits into from
Sep 2, 2020

Conversation

tk26
Copy link
Contributor

@tk26 tk26 commented Aug 27, 2020

addresses issue #3219

@welcome
Copy link

welcome bot commented Aug 27, 2020

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
Copy link
Contributor Author

tk26 commented Aug 27, 2020

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

@achingbrain
Copy link
Member

Thanks for opening this, it looks great.

To get this over the line it needs:

  1. Exposing in the HTTP API /id/ endpoint
  2. 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)
  3. Tests added to the id interface test suite
  4. Those tests skipped in the HTTP API Client suite if not implemented in a shipping version of go-ipfs yet

@tk26
Copy link
Contributor Author

tk26 commented Aug 28, 2020

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

@achingbrain
Copy link
Member

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?

it('should have protocols property', async () => {
const res = await ipfs.id()

expect(res).to.have.a.property('protocols').that.is.an('array')
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 assert that there are protocols present, at a minimum I think /ipfs/id/1.0.0 should be supported

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tk26 tk26 Sep 1, 2020

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

tk26 commented Aug 28, 2020

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


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

Choose a reason for hiding this comment

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

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())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks @jacobheun

it('should have protocols property', async () => {
const res = await ipfs.id()

expect(res).to.have.a.property('protocols').that.is.an('array')
Copy link
Contributor

Choose a reason for hiding this comment

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

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 achingbrain merged commit 1b6cf60 into ipfs:master Sep 2, 2020
@achingbrain
Copy link
Member

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 tk26 deleted the feat/id-list-protocols branch September 3, 2020 15:22
@tk26
Copy link
Contributor Author

tk26 commented Sep 3, 2020

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.

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

Successfully merging this pull request may close these issues.

3 participants