refactor: mark {In, Out}boundOpenInfo as deprecated

[drHuangMHT]

Description

Mark {In, Out}boundOpenInfo as deprecated.
May close #3268.

Notes & open questions

I'm putting off formatting until the change has been accepted for clearer diff.
I have changed all occurrence of OpenInfo with concrete type wherever possible to avoid excessive use of #[allow(deprecated)]. Is this optimal ?

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[drHuangMHT]

@jxs Got a moment to review this? It seems that I cannot assign a reviewer because I don't have the permission, so I have to do an @ mention.

[elenaf9]

Thank you @drHuangMHT, for this PR, and all of your recent contributions in general!

I think we might be able to avoid some of the #[allow(deprecated)] if we add defaults to the types that currently include {Inbound, Outbound}Info generics (if they are trailing), e.g. to ConnectionEvent. That way the deprecated types won't need to be mentioned by the handlers anymore. Wdyt?

[drHuangMHT]

I think we might be able to avoid some of the #[allow(deprecated)] if we add defaults to the types that currently include {Inbound, Outbound}Info generics (if they are trailing)

What about those that are not(whose OpenInfo generic parameter in the middle)? I gave it a shot and it came out quite terrible because I had to reorder the parameters in every implementation. I think the impact will basically be the same as we outright removed {In,Out}boundOpenInfo without a deprecation warning, or even worse because the error messages are rather confusing.

[elenaf9]

What about those that are not(whose OpenInfo generic parameter in the middle)? I gave it a shot and it came out quite terrible because I had to reorder the parameters in every implementation. I think the impact will basically be the same as we outright removed {In,Out}boundOpenInfo without a deprecation warning, or even worse because the error messages are rather confusing.

Yeah, I'd leave those as they are (=> your current implementation).

[elenaf9]

One more comment about using expect instead of allow in all places, rest lgtm!

[drHuangMHT]

Waiting for #5730 for better handling of deprecation warning bypass?

[elenaf9]

Clippy failing on beta is unrelated, will be fixed with #5802.

Only thing missing is a CHANGELOG entry. It would be great if the entry could be a bit more verbose, so that users know why it {In, Out}boundOpenInfo was removed, and how to refactor their implementation. Maybe something along the lines of (feel to rephrase):

Deprecate ConnectionHandler::{InboundOpenInfo, OutboundOpenInfo}.
Previously, this allowed to pass along data for a pending inbound or outbound substream. The info was then returned to the ConnectionHandler together with the upgraded substream output.
However, substreams in libp2p are completely interchangeably and thus there is no reason to link data to a specific substream. Instead, if data should be added to a substream after its upgrade, the ConnectionHandler should track it (e.g. in a FIFO queue) and match in to ConnectionEvents.

[jxs]

LGTM, thanks for this @drHuangMHT! And @elenaf9 for the review!

[mergify]

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Approvals have been dismissed because the PR was updated after the send-it label was applied.

[elenaf9]

Wasn't the CHANGELOG entry still missing? Or did I just not see it?

[dariusc93]

Wasn't the CHANGELOG entry still missing? Or did I just not see it?

Looks like it is missing (thought the CI would have catched that?)

[elenaf9]

Wasn't the CHANGELOG entry still missing? Or did I just not see it?

Looks like it is missing (thought the CI would have catched that?)

Our CI only checks for a CHANGELOG entry if it's not chore / refactor / deps / docs... This PR probably should have been a feat instead of refactor, since refactor usually describes internal changes that are not visible to the outside and change no logic.
MB, I didn't notice when reviewing.

@drHuangMHT would you mind creating a follow-up PR to amend the CHANGELOGs? I think we might even need a CHANGELOG entry in each crate that was touched in this PR.

[drHuangMHT]

changelog is still missing