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

feat(swarm): add ConnectionDenied::downcast_ref #4020

Merged
merged 1 commit into from
Jun 12, 2023

Conversation

nathanielc
Copy link
Contributor

@nathanielc nathanielc commented Jun 2, 2023

Description

Prior to this change it was only possible to downcast a ConnectionDenied error when you had ownership of it which isn't the case inside NetworkBehaviours. This change allows downcasting by reference.

See #4018.

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

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Add a changelog entry please.

@@ -466,6 +466,57 @@ mod tests {
quickcheck(prop as fn(_));
}

#[test]
fn max_established_incoming_downcast_ref() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this test. We don't actually control any logic here other than forwarding calls so the typesystem should be good enough :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove the test or just leave it be?

@thomaseizinger thomaseizinger changed the title feat: add downcast_ref support to ConnectionDenied type feat(swarm): add ConnectionDenied::downcast_ref Jun 3, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 12, 2023

This pull request has merge conflicts. Could you please resolve them @nathanielc? 🙏

Prior to this change it was possible to only have a reference to a ConnectionDenied error and not be
able to downcast it to check if the connection-limits produced the error.
@nathanielc
Copy link
Contributor Author

@thomaseizinger Thanks for the review, I have fixed the conflicts, added a changelog entry and removed the unnecessary test.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mergify mergify bot merged commit 4a1703f into libp2p:master Jun 12, 2023
@nathanielc nathanielc deleted the downcast_ref branch June 12, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants