Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove PendingUpgrade in favor of DeniedUpgrade #2856

Closed
thomaseizinger opened this issue Aug 30, 2022 · 2 comments
Closed

Remove PendingUpgrade in favor of DeniedUpgrade #2856

thomaseizinger opened this issue Aug 30, 2022 · 2 comments

Comments

@thomaseizinger
Copy link
Contributor

Description

In #2785, we added a PendingUpgrade implementation that is essentially equivalent to DeniedUpgrade apart from also needing a protocol. @mxinden was this on purpose or an oversight? DeniedUpgrade has quite some usage throughout the codebase but I think I prefer the name PendingUpgrade as it reflects the use of futures::pending. At the same time, passing a protocol to PendingUpgrade is unnecessary IMO.

What do people think of removing

Motivation

Having two upgrades with the same functionality is unnecessary.

Current Implementation

Are you planning to do it yourself in a pull request?

Maybe.

@mxinden
Copy link
Member

mxinden commented Sep 2, 2022

In #2785, we added a PendingUpgrade implementation that is essentially equivalent to DeniedUpgrade apart from also needing a protocol. @mxinden was this on purpose or an oversight?

They serve two different purposes. PendingUpgrade provides a protocol name during multistream-select negotiation, but never proceeds once the protocol has been negotiated. DeniedUpgrade provides no protocol name during multistream-select negotiation, thereby the multistream-select negotiation fails.

I can not think of a use-case for PendingUpgrade beyond testing, though I think that is enough to justify its existence.

Does the above make sense?

@thomaseizinger
Copy link
Contributor Author

Does make sense, thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants