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(p2p)!: remove conngater dependency #107

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

walldiss
Copy link
Member

@walldiss walldiss commented Sep 19, 2023

libp2p conngater is exact type and not an interface, which makes it impossible to modify or mock its functionality on client side. Replaces libp2p conngater dependency with callback func, to allow easy mocking for clients.

Required by celestiaorg/celestia-node#2702 to provided conngater with white list instead of basic one.

@codecov-commenter
Copy link

Codecov Report

Merging #107 (e8b49e3) into main (a6544e3) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #107   +/-   ##
=======================================
  Coverage   68.60%   68.60%           
=======================================
  Files          37       37           
  Lines        2934     2934           
=======================================
  Hits         2013     2013           
  Misses        770      770           
  Partials      151      151           
Files Changed Coverage Δ
p2p/exchange.go 82.17% <100.00%> (ø)
p2p/peer_tracker.go 78.75% <100.00%> (ø)

Comment on lines -53 to +52
gater *conngater.BasicConnectionGater,
blacklistPeer func(id peer.ID) error,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It is different one, used as dialer interceptor

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not have BlockPeer()

Copy link
Member

Choose a reason for hiding this comment

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

I see, that's confusing

@walldiss walldiss requested a review from Wondertan September 19, 2023 11:04
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Fine with me but please mark as breaking @walldiss

@renaynay renaynay changed the title feat(p2p): remove conngater dependency feat(p2p)!: remove conngater dependency Sep 19, 2023
@Wondertan
Copy link
Member

Well, actually, it would be nice to make a local interface, that has BlockPeer on it. In the future when we need more methods of PeerGater, we will then just add them, instead of breaking again.

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Comment above

@walldiss
Copy link
Member Author

walldiss commented Sep 21, 2023

If we create a interface and change it later, it would break api, because implementations satisfying old interface will not satisfy new one. So we can defer creation of extra abstraction until we need it.

@Wondertan
Copy link
Member

I mean adding more methods of PeerGater interface which BasicConnGater satisfies

@walldiss walldiss marked this pull request as draft October 23, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants