fix(@libp2p/utils): switch to @chainsafe/is-ip #1957
Conversation
…ddress The `@achingbrain/ip-address` module is a fork of `ip-address` which seems to have dependencies on older modules that break modern runtimes like react native. `@Chainsafe/is-ip` does not have these limitations so switch to that. Fixes #1926
| @@ -19,7 +19,7 @@ describe('IP and port to Multiaddr', () => { | |||
| it('creates multiaddr from valid IPv4 in IPv6 IP and port', () => { | |||
| const ip = '0:0:0:0:0:0:101.45.75.219' | |||
| const port = '9090' | |||
| expect(ipPortToMultiaddr(ip, port).toString()).to.equal(`/ip4/101.45.75.219/tcp/${port}`) | |||
| expect(ipPortToMultiaddr(ip, port).toString()).to.equal(`/ip6/::652d:4bdb/tcp/${port}`) | |||
There was a problem hiding this comment.
@chainsafe/is-ip doesn't support detecting IPv4 in IPv6 addresses so we lose support for returning an IPv4 address in this case.
Node.js recognises these sorts of addresses as IPv6 so I don't know if this is a problem or not.
E.g.:
% node
Welcome to Node.js v18.14.0.
Type ".help" for more information.
> const net = require('net')
undefined
> net.isIP('0:0:0:0:0:0:101.45.75.219')
6
There was a problem hiding this comment.
We should be able to detect dual ipv6 addresses by simply testing for octects (or their delimiter, a period)
There was a problem hiding this comment.
I need to test the browser usecase. if this is showing as ipv6, we should be able to just wrap in brackets for URL construction.
There was a problem hiding this comment.
> new URL('http://[::101.45.75.219]')
URL {
href: 'http://[::652d:4bdb]/',
origin: 'http://[::652d:4bdb]',
protocol: 'http:',
username: '',
password: '',
host: '[::652d:4bdb]',
hostname: '[::652d:4bdb]',
port: '',
pathname: '/',
search: '',
searchParams: URLSearchParams {},
hash: ''
}There was a problem hiding this comment.
> new URL('http://[0:0:0:0:0:0:101.45.75.219]')
URL {
href: 'http://[::652d:4bdb]/',
origin: 'http://[::652d:4bdb]',
protocol: 'http:',
username: '',
password: '',
host: '[::652d:4bdb]',
hostname: '[::652d:4bdb]',
port: '',
pathname: '/',
search: '',
searchParams: URLSearchParams {},
hash: ''
}seems like it should be fine
achingbrain
changed the title
| const errMsg = `invalid ip:port for creating a multiaddr: ${ip}:${port}` | ||
| log.error(errMsg) | ||
| throw new CodeError(errMsg, Errors.ERR_INVALID_IP) |
There was a problem hiding this comment.
This is so much cleaner than the previous code
| @@ -19,7 +19,7 @@ describe('IP and port to Multiaddr', () => { | |||
| it('creates multiaddr from valid IPv4 in IPv6 IP and port', () => { | |||
| const ip = '0:0:0:0:0:0:101.45.75.219' | |||
| const port = '9090' | |||
| expect(ipPortToMultiaddr(ip, port).toString()).to.equal(`/ip4/101.45.75.219/tcp/${port}`) | |||
| expect(ipPortToMultiaddr(ip, port).toString()).to.equal(`/ip6/::652d:4bdb/tcp/${port}`) | |||
There was a problem hiding this comment.
We should be able to detect dual ipv6 addresses by simply testing for octects (or their delimiter, a period)
| @@ -19,7 +19,7 @@ describe('IP and port to Multiaddr', () => { | |||
| it('creates multiaddr from valid IPv4 in IPv6 IP and port', () => { | |||
| const ip = '0:0:0:0:0:0:101.45.75.219' | |||
| const port = '9090' | |||
| expect(ipPortToMultiaddr(ip, port).toString()).to.equal(`/ip4/101.45.75.219/tcp/${port}`) | |||
| expect(ipPortToMultiaddr(ip, port).toString()).to.equal(`/ip6/::652d:4bdb/tcp/${port}`) | |||
There was a problem hiding this comment.
I need to test the browser usecase. if this is showing as ipv6, we should be able to just wrap in brackets for URL construction.
|
I gave approval, but I expect this will break one scenario: consumers of dualIpv6 addresses that don't support ipv6 used to get an ipv4 multiaddr they could use; they won't anymore. |
|
It seems weird to me that we’d accept a v6 address and return a v4 one, not what I would expect. |
The
@achingbrain/ip-addressmodule is a fork ofip-addresswhich seems to have dependencies on older modules that break modern runtimes like react native.@Chainsafe/is-ipdoes not have these limitations so switch to that.Fixes #1926