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.

Sign up for GitHub

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

Jump to bottom

fix: make ipfs the default 421 proto name #77

Merged
merged 3 commits into from
Dec 17, 2018
Merged

fix: make ipfs the default 421 proto name #77

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5850166
fix: make ipfs the default 421 proto name
jacobheun Dec 17, 2018
3a3baa6
chore: fix linting
jacobheun Dec 17, 2018
1d96972
chore: make 421 proto name order notes less confusing
jacobheun Dec 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/protocols-table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@ Protocols.table = [
[132, 16, 'sctp'],
// all of the below use varint for size
[302, 0, 'utp'],
[421, Protocols.lengthPrefixedVarSize, 'ipfs'],
// preferred name for 421 (added below ipfs, p2p takes precedence during table population)
[421, Protocols.lengthPrefixedVarSize, 'p2p'],
// preferred name for 421 (added below p2p, ipfs takes precedence during table population)
Copy link
Member

Choose a reason for hiding this comment

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

This comment isn't correct anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait. It is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did an update because I saw it wasn't correct when I made the change and fixed the comment. The push may have just taken a little bit as I'm on a train 🚆.

Copy link
Member

Choose a reason for hiding this comment

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

I find it confusing (as you can see on my comments). Why not putting a comment before the p2p saying:

// `p2p` is the preferred name for 421

And before ipfs:

// `ipfs` is intentionally below `p2p` so that `ipfs` takes precedence during table population
// this enables us ...

Perhaps in nicer English, but you get the point :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, let me know if that is clearer.

// this enables us to support p2p but allow for better backwards support of ipfs multiaddrs
// while migration happens to using p2p as the default
[421, Protocols.lengthPrefixedVarSize, 'ipfs'],
[480, 0, 'http'],
[443, 0, 'https'],
[460, 0, 'quic'],
Expand Down
80 changes: 74 additions & 6 deletions test/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,14 +215,21 @@ describe('variants', () => {
const str = '/ip4/127.0.0.1/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC/tcp/1234'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str.replace('/p2p/','/ipfs/'))
})

it('ip4 + ipfs', () => {
const str = '/ip4/127.0.0.1/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC/tcp/1234'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str)
})

it('ip6 + p2p', () => {
const str = '/ip6/2001:8a0:7ac5:4201:3ac9:86ff:fe31:7095/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC/tcp/1234'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str)
expect(addr.toString()).to.equal(str.replace('/p2p/','/ipfs/'))
})

it.skip('ip4 + dccp', () => {})
Expand Down Expand Up @@ -288,45 +295,80 @@ describe('variants', () => {
expect(addr.toString()).to.equal(str)
})

it('ip6 + tcp + websockets + ipfs', () => {
const str = '/ip6/2001:8a0:7ac5:4201:3ac9:86ff:fe31:7095/tcp/8000/ws/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str)
})

it('ip6 + tcp + websockets + p2p', () => {
const str = '/ip6/2001:8a0:7ac5:4201:3ac9:86ff:fe31:7095/tcp/8000/ws/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str.replace('/p2p/','/ipfs/'))
})

it('ip6 + udp + quic + ipfs', () => {
const str = '/ip6/2001:8a0:7ac5:4201:3ac9:86ff:fe31:7095/udp/4001/quic/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str)
})

it('ip6 + udp + quic + p2p', () => {
const str = '/ip6/2001:8a0:7ac5:4201:3ac9:86ff:fe31:7095/udp/4001/quic/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str)
expect(addr.toString()).to.equal(str.replace('/p2p/','/ipfs/'))
})

it('p2p', () => {
const str = '/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str.replace('/p2p/','/ipfs/'))
})

it('ipfs', () => {
const str = '/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str)
})

it('p2p-circuit', () => {
const str = '/p2p-circuit/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str)
expect(addr.toString()).to.equal(str.replace('/p2p/','/ipfs/'))
})

it('p2p-circuit p2p', () => {
const str = '/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC/p2p-circuit'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str.replace('/p2p/','/ipfs/'))
})

it('p2p-circuit ipfs', () => {
const str = '/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC/p2p-circuit'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str)
})

it('p2p-webrtc-star', () => {
const str = '/ip4/127.0.0.1/tcp/9090/ws/p2p-webrtc-star/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str.replace('/p2p/','/ipfs/'))
})

it('p2p-webrtc-star ipfs', () => {
const str = '/ip4/127.0.0.1/tcp/9090/ws/p2p-webrtc-star/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC'
const addr = multiaddr(str)
expect(addr).to.have.property('buffer')
expect(addr.toString()).to.equal(str)
})

Expand Down Expand Up @@ -381,7 +423,7 @@ describe('helpers', () => {
}])
})

it('works with ipfs/p2p', () => {
it('works with ipfs', () => {
expect(
multiaddr('/ip4/0.0.0.0/utp/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC').protos()
).to.be.eql([{
Expand All @@ -396,7 +438,28 @@ describe('helpers', () => {
resolvable: false
}, {
code: 421,
name: 'p2p',
name: 'ipfs',
size: -1,
resolvable: false
}])
})

it('works with p2p', () => {
expect(
multiaddr('/ip4/0.0.0.0/utp/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC').protos()
).to.be.eql([{
code: 4,
name: 'ip4',
size: 32,
resolvable: false
}, {
code: 302,
name: 'utp',
size: 0,
resolvable: false
}, {
code: 421,
name: 'ipfs',
size: -1,
resolvable: false
}])
Expand Down Expand Up @@ -586,11 +649,16 @@ describe('helpers', () => {
})

describe('.getPeerId should parse id from multiaddr', () => {
it('parses extracts the peer Id from a multiaddr', () => {
it('parses extracts the peer Id from a multiaddr, p2p', () => {
expect(
multiaddr('/p2p-circuit/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC').getPeerId()
).to.equal('QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC')
})
it('parses extracts the peer Id from a multiaddr, ipfs', () => {
expect(
multiaddr('/p2p-circuit/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC').getPeerId()
).to.equal('QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC')
})
})

describe('.getPeerId should return null on missing peer id in multiaddr', () => {
Expand Down