That sounds reasonable to me. Thanks @dmitry-markin for the detailed report.

I am not going to work on this any time soon. I am happy to help you implement it though. Given your "maybe", are you still interested?

Yes, I think I can look into it in a week or so. The one thing I worry about is that these changes will definitely break the API, and I don't know how to handle it. I.e, I can update substrate, but we need to somehow notify other clients.

Given that this is not a subtle change, but a change at the type system, I think users will (a) notice early and (b) can consult the changelog to get more advice. In other words, I am not worried about the breaking API change @dmitry-markin.

It seems the cause for DialError::WrongPeerId to contain ConnectedPoint, and not Dialer or just Multiaddr is that PendingOutboundConnectionError is based on PendingConnectionError also used for PendingInboundConnectionError. So they both share ConnectedPoint internally, even though ConnectedPoint::Listener is not relevant for PendingOutboundConnectionError/DialError.

To fix the API issue we can drop the Listener branch when converting PendingOutboundConnectionError into DialError here.

I see two ways of doing this. First is to extract ConnectedDialer, PendingDialer, and Listener (listeners are the same in both enums) from ConnectedPoint & PendingPoint. Then replace ConnectedPoint with ConnectedDialer in DialError. This requires a lot of modifications throughout the codebase and significantly changes the API. EDIT: I don't think this makes sense :-)

Another option is to keep ConnectedPoint and PendingPoint enums the same, and just modify DialError to include something instead ConnectedPoint. This can be either independent new ConnectedDialer struct, or even a plain Multiaddr.

I would go with the second option, but I don't understand whether we need to report the role_override when reporting the DialError (i.e., use the ConnectedDialer struct), or reporting a Multiaddr will be enough.

@mxinden what do you think?

I've implemented the simplest solution in #2793.

It seems the cause for DialError::WrongPeerId to contain ConnectedPoint, and not Dialer or just Multiaddr is that PendingOutboundConnectionError is based on PendingConnectionError also used for PendingInboundConnectionError. So they both share ConnectedPoint internally, even though ConnectedPoint::Listener is not relevant for PendingOutboundConnectionError/DialError.

I would be in favor of doing as much as possible at compile time. Would splitting PendingConnectionError be an option?

I would be in favor of doing as much as possible at compile time. Would splitting PendingConnectionError be an option?

Yes, this seems reasonable, I'll check it.

Hey @jxs

Can I pick this issue?

Hi @akaladarshi yes! that would be great, Thanks! 🚀