[WIP] feat: dns support for WS #47
Conversation
|
tests are failing because of libp2p/interface-transport#23 |
test/node.js
Outdated
| @@ -179,11 +185,15 @@ describe('filter addrs', () => { | |||
| const mh2 = multiaddr('/ip4/127.0.0.1/udp/9090') | |||
| const mh3 = multiaddr('/ip4/127.0.0.1/tcp/9090/ws') | |||
| const mh4 = multiaddr('/ip4/127.0.0.1/tcp/9090/ws/ipfs/Qmb6owHp6eaWArVbcJJbQSyifyJBttMMjYV76N2hMbf5Vw') | |||
| const mh5 = multiaddr('/dns/ipfs.io/ws') | |||
| const mh6 = multiaddr('/dns/ipfs.io/ws/ipfs/Qmb6owHp6eaWArVbcJJbQSyifyJBttMMjYV76N2hMbf5Vw') | |||
There was a problem hiding this comment.
I think we need to also account for addresses like
/dns/ipfs.io/tcp/80/ws/ipfs/Qmb6owHp6eaWArVbcJJbQSyifyJBttMMjYV76N2hMbf5Vw
otherwise you can't specify the port
There was a problem hiding this comment.
Yes these need /tcp/80/wss or /tcp/443/wss, and it needs to be /dns4 or /dns6
There was a problem hiding this comment.
Well, actually, default for just /ws is port 80 and /wss is port 443, that's it.
@dryajov still up to finish this? It would be sweet for the 0.23 release! :)
There was a problem hiding this comment.
@diasdavid yep, I'll get this in.
There was a problem hiding this comment.
@diasdavid @lgierth I think we're talking about having both /tcp/80/wss and /tcp/443/ws for non standard ports? They are both valid cases and should be supported, right?
There was a problem hiding this comment.
Turns out that we apparently don't support ports in DNS address in js-mafmt, here is a PR that addresses it - multiformats/js-mafmt#15. I'm not entirely sure its the right approach here, but it seems to work.
With this PR in mafmt, I have most of the tests passing here, I should be able to get them all passing soon. I'll push as soon as I have everything working.
There was a problem hiding this comment.
@dryajov published mafmt for you :)
| multiaddr('/ip4/127.0.0.1/tcp/9093/ws') | ||
| multiaddr('/ip4/127.0.0.1/tcp/9092/wss'), | ||
| multiaddr('/dns4/awesome-dns-server.com/tcp/9092/ws'), | ||
| multiaddr('/dns4/awesome-dns-server.com/tcp/9092/wss') |
There was a problem hiding this comment.
This will fail as these hosts don't exist.
There was a problem hiding this comment.
Yep, those should fail... They are being intentionally filtered out, according to the test.
Not the clearest test case, some refactoring might make sense.
There was a problem hiding this comment.
Everything should be passing right now. I also refactored the existing tests.
There was a problem hiding this comment.
Re: compliance addresses, you are right, i was talking about something else. I'll make those point to existing hosts.
|
merging to a branch in the repo so that I can push changes (most cleaning up unnecessary changes) |
@diasdavid Let me know if this is more or less whats needed for dns multiaddress support... (it did send me down the rabbit hole 😛 )