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

Add multiaddr checks #27

Merged
merged 6 commits into from
Mar 3, 2019
Merged

Add multiaddr checks #27

merged 6 commits into from
Mar 3, 2019

Conversation

lidel
Copy link
Member

@lidel lidel commented Feb 24, 2019

This PR introduces utility methods for multiaddr detection:

isIPFS.multiaddr(input)

Generic multiaddr checks if input is a valid multiaddr.
(It does not care about its type, just validity)

isIPFS.peerMultiaddr(input)

peerMultiaddr provides easy validation of IPFS Peer addresses (direct or encapsulated).

It is a thin wrapper on top of generic isIPFS.multiaddr(input) check coupled with mafmt.IPFS.matches(input) from mafmt (low-level multiaddr validation library)

Details behind mafmt.IPFS can be found at https://github.com/multiformats/js-mafmt#api

Note: I've added only this check because is-ipfs is IPFS-centric utility.
If one needs more nuanced multiaddr validation, then mafmt should be used directly.


Closes #26

@hacdias, @obo20 – thoughts?

This adds `multiaddr` check if input is a valid multiaddr.
A separate check for IPFS peer multiaddr will be added in next commit.

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
This adds `peerMultiaddr` check for easy validation of IPFS peer
addresses.

It is a think wrapper on top of generic `multiaddr` check
coupled with `mafmt.IPFS.matches(multiaddr)`

Details behind `mafmt.IPFS` can be found at
https://github.com/multiformats/js-mafmt#api

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@ghost ghost assigned lidel Feb 24, 2019
@ghost ghost added the status/in-progress In progress label Feb 24, 2019
@obo20
Copy link

obo20 commented Feb 25, 2019

@lidel This looks great. I do have one additional thought though.

For the peer multi address check, I wonder if it would be beneficial to have an "fullPeerAddr" flag to add to the check.

While both of these are valid:
isIPFS.peerMultiaddr('/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSoooo4')
and
isIPFS.peerMultiaddr('/ip4/127.0.0.1/tcp/1234/ws/ipfs/QmUjNmr8TgJCn1Ao7DvMy4cjoZU15b9bwSCBLE3vwXiwgj')
It's my understanding when using "ipfs swarm connect", the second entry would resolve faster due to having the direct location of the peer's ID.

In our use case, I know I would require the full version of the peer address if I had the option to.

Copy link
Member

@hacdias hacdias 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 agree that we should only add these two verifications since is-ipfs is directed to IPFS addresses.

I also think that @obo20's idea is great: although I'm not sure if easily feasible with the current code.

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
Copy link
Member Author

lidel commented Feb 25, 2019

@obo20 "full peer addr" is a bit vague term, some long multiaddrs still require additional resolving:

For example, peer multiaddrs over relays (/p2p-circuit) may or may not require additional resolving (depends if relay already knows multiaddrs of target peer or not):
/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSoooo4/p2p-circuit/ipfs/QmUjNmr8TgJCn1Ao7DvMy4cjoZU15b9bwSCBLE3vwXiwgj

Is my understanding correct that what you really want to find "direct" multiaddrs resolved to IP & PORT, to guarantee no additional resolve step is necessary?

A crude but "good enough" approach for finding such multiaddrs would be to check if multiaddr starts with /ip4/ or /ip6/ and has /ipfs/ occurring exactly once:

// this is something you could do in your app
const isIPFS = require('is-ipfs')
function isRawIpPeerMultiaddr(addr) {
  if (!isIPFS.peerMultiaddr(addr)) return false
  if ((addr.startsWith('/ip4/') || addr.startsWith('/ip6/')) && addr.split('/ipfs/').length === 2) return true
  return false
}

I'd prefer to keep is-ipfs transport-agnostic, and merge this PR as-is.
(but added a note that complex multiaddr checks can be built using isIPFS.multiaddr and mafmt)

src/index.js Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
test/test-cid.spec.js Show resolved Hide resolved
@@ -116,6 +141,8 @@ const ipnsSubdomain = (url) => isIpns(url, fqdnPattern, fqdnProtocolMatch, fqdnH

module.exports = {
multihash: isMultihash,
multiaddr: isMultiaddr,
peerMultiaddr: isPeerMultiaddr,
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR but I'm musing on naming, I would ideally like to do something like const { isMultiaddr } = require('is-ipfs') or import { isMultiaddr } from 'is-ipfs' rather than have to import the whole library.

If we eventually start using es-modules we can tree shake the stuff we're not using but as it is now we're encouraging importing the whole library since (for example) const { cid } = require('is-ipfs') would likely conflict with cid instance vars we have in code and it's not descriptive to validate a cid like if (cid(myCid)).

Copy link
Member Author

@lidel lidel Feb 25, 2019

Choose a reason for hiding this comment

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

This is a good point, let's track this in #28

@lidel lidel mentioned this pull request Feb 25, 2019
@obo20
Copy link

obo20 commented Feb 25, 2019

Is my understanding correct that what you really want to find "direct" multiaddrs resolved to IP & PORT, to guarantee no additional resolve step is necessary?

Yes this what I was getting at. The goal of providing a multiaddr was to speed up content discovery. I'd imagine the speed benefits of these two series of actions would be about the same:

  • "ipfs pin add {hash}" - The node now searches for the content on the DHT and pins it upon discovery
  • "ipfs swarm connect {nonDirectMultiAddr}" then "ipfs pin add {hash}" - The node would have to first search for the node by its peer ID (essentially performing a ipfs dht findpeer). Once it was connected, then the "ipfs pin add {hash} action would take place.

However
In this scenario, I imagine things would go much faster:

  • "ipfs swarm connect {directMultiAddr}" then "ipfs pin add {hash}" - In this scenario, the node would essentially instantly connect to the peer and then start pinning the content. Since we know the direct IP of the node, we've essentially sidestepped the time of searching for the node on the DHT.

I'd prefer to keep is-ipfs transport-agnostic, and merge this PR as-is.
(but added a note that complex multiaddr checks can be built using isIPFS.multiaddr and mafmt)

I'm perfectly happy with this @lidel. The additional functionality I suggested is merely a nice to have.

@lidel lidel mentioned this pull request Feb 25, 2019
3 tasks
@lidel
Copy link
Member Author

lidel commented Feb 25, 2019

Applied some suggestions from Alan.

Parking this PR until multiformats/js-mafmt#39 is released and ipfs/aegir#336 is closed.

This simplifies code by moving some types of validation into upstream libraries
We also switch to latest mafmt which accepts Buffer input

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
We moved to Travis

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel merged commit 75962d0 into master Mar 3, 2019
@ghost ghost removed the status/in-progress In progress label Mar 3, 2019
@lidel lidel deleted the feat/multiaddr branch March 3, 2019 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants