Modify NetAddress Enum naming

[yanganto]

Hi there,

I have a proposal.

IPv4 socket addresses consist of an IPv4 address and a 16-bit port number, as stated in IETF RFC 793

It may be good to use SocketV4 and SocketV6 here, and also the same naming in Rust std SocketAddr

rust-lightning/lightning/src/ln/msgs.rs

Lines 745 to 759 in 74a9ed9

	pub enum NetAddress {
	/// An IPv4 address/port on which the peer is listening.
	IPv4 {
	/// The 4-byte IPv4 address
	addr: [u8; 4],
	/// The port on which the node is listening
	port: u16,
	},
	/// An IPv6 address/port on which the peer is listening.
	IPv6 {
	/// The 16-byte IPv6 address
	addr: [u8; 16],
	/// The port on which the node is listening
	port: u16,
	},

If this proposal is good, I am happy to do this and also #2183, #2056 at the same time. Some these are implemented in our project.
https://github.com/kuutamolabs/lightning-knd/blob/e5f05755be1c702c56b469964c7d9586bce15e5d/api/src/lib.rs

[TheBlueMatt]

Sorry I missed this before the long weekend. I'm not sure that SocketAddress is right here - a socket address is the address of your socket, and it wouldn't be unreasonable to call a named pipe or even an onion address (with the right API) a "socket address". If we want to be overly precise I believe the proper naming would be "TCPv4" and "TCPv6" as those are the direction protocols in use here (and the type of socket address).

[yanganto]

Yeh, if they are not real sockets and are protocols, I think TcpV4 and TcpV6 are good here.
Would I open a PR on this?

[TheBlueMatt]

Sure if you want, I'm not super convinced its confusing to users and worth changing, but if you feel strongly about it feel free!

[arik-so]

I think it's actually less confusing to users to stick with IPv4 and IPv6.

[yanganto]

I think it's actually less confusing to users to stick with IPv4 and IPv6.

The reason is here
#2380 (comment)

[TheBlueMatt]

I wonder if some of the confusion isn't enum/variant mismatch - the enum is really a sockaddr in the traditional sense, the variants just describe the different potential types of socket addresses. Would you be happy simply renaming the enum, as a socket address if type ipv4 makes sense to me.

[TheBlueMatt]

Closing this for inactivity - I'm definitely happy to keep discussing this (any thoughts on my last suggestion above?) but it seems the consensus outside of the reporter was that its not super confusing as-is, so doesn't seem worth keeping open waiting for the OP to respond.

[yanganto]

Sorry about the delay, I just had a trip and forgot the thread, and wait for someone to own this project to decide the final result.

@TheBlueMatt Did you suggest SocketAddrV4 or TcpIPv4SockAddr or anything else?

I don't understand what you mean by what naming you accept. For me, SocketAddrV4 is better than TcpIPv4SockAddr, and both are correct. IPv4 with port does not make sense and is unprofessional to miss up ip and socket.

If you have a final result better than IPv4, I can modify it.

[TheBlueMatt]

Sorry about the delay, I just had a trip and forgot the thread, and wait for someone to own this project to decide the final result.

No problem!

@TheBlueMatt Did you suggest SocketAddrV4 or TcpIPv4SockAddr or anything else?

My suggestion was mostly to rename the enum itself, not just the variants. So eg the enum would be SocketAddr (rather than NetAddress) and the variant could then be TcpIPv4 or whatever. Certainly not "final", but folks may be more interested in that, pending you thinking it resolves the confusion.

[yanganto]

Your solution looks good to me. Would we set a timer here? if there is no other voice and I can prepare the PR for this.

SocketAddress is better than NetAddress, because each net card within the same protocol can have more than one socket address and only one net address. So the SocketAddress here is better than NetAddress.

[jkczyz]

SocketAddr is fine by me. For the variant names, I'd suggest using TcpIpV4 and TcpIpV6. Note the lowercase p in Ip and uppercase V for consistency with Tcp and typically how acronyms are cased in Rust identifiers found in the standard library. At very least it would be internally consistent within the enum.

[yanganto]

Good, I think it is good to implement now. 😃

pub enum SocketAddress {
	/// An IPv4 address/port on which the peer is listening.
	TcpIpV4 { .. },
	/// An IPv6 address/port on which the peer is listening.
	TcpIpV6 { .. },
	/// An old-style Tor onion address/port on which the peer is listening.
	///
	/// This field is deprecated and the Tor network generally no longer supports V2 Onion
	/// addresses. Thus, the details are not parsed here.
	OnionV2([u8; 12]),
	/// A new-style Tor onion address/port on which the peer is listening.
	///
	/// To create the human-readable "hostname", concatenate the ED25519 pubkey, checksum, and version,
	/// wrap as base32 and append ".onion".
	OnionV3 { .. },
	/// A hostname/port on which the peer is listening.
	Hostname { .. },
}