-
Notifications
You must be signed in to change notification settings - Fork 44
add ipv6 listening for API / WebGateway #84
Conversation
Thank you for submitting this PR!
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.
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. |
thumbs up for ipv6! |
addresses.go
Outdated
API Strings // address for the local API (RPC) | ||
Gateway Strings // address to listen on for IPFS HTTP object gateway | ||
API []string // addresses for the local API (RPC) | ||
Gateway []string // addresses to listen on for IPFS HTTP object gateway |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you changing these types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, Swarm, Announce and NoAnnounce are arrays, because they hold multiple addresses.
API and Gateway now need to hold multiple addresses too. So they need to be an array.
Or is there's something wrong with my assumption?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We created the Strings type to allow either a single string or an array of strings. Changing this to a simple array would be a backwards incompatible change.
What's the motivation for listening on IPv6 for localhost? IPv4 should always work for localhost regardless. |
...because [::1] is shorter to type than 127.0.0.1? ;-) now, kidding aside, ipv6 simply supersedes ipv4 and will be the the default protocol in the future. actually, it could be the default right now if people cared a bit more. ...imagine the feeling when you're dropping all this nat-traversing code and shit! ;-) i agree the localhost is the least case where ipv6 is important but i look at it from a different point of view. i feel this as a part of the "ipv6 by default everywhere" mindset that will drag the general ipv6 adoption with it... anyway, at least for this very patch, i don't see any negative (increased code complexity, security issues, ...) with it so in my opinion it should be a "go" even if you personally don't really care about ipv6 that much. |
Fair enough. I'm just always wary of changes without reasons because https://xkcd.com/1172/. |
Well, there's no reason why we shouldn't use IPv6. I dislike using legacy IP. :) |
Sorry, messed that up. I'm not sure you can change the remote branch of a PR, so I filed a new one. |
No description provided.