-
Notifications
You must be signed in to change notification settings - Fork 186
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
add support for custom protocol matching function #440
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this looks useful.
A couple of small changes are needed:
- It should probably be combined with the appropriate feature test function in gossipsub.
Mind adding some about this in the documentation? - No need to use a pointer! functions can be tested against nil with
!=
Also, can you add a test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, this is looking good. Just a couple of minor things to fix and we can merge.
pubsub.go
Outdated
@@ -235,6 +240,7 @@ func NewPubSub(ctx context.Context, h host.Host, rt PubSubRouter, opts ...Option | |||
peerOutboundQueueSize: 32, | |||
signID: h.ID(), | |||
signKey: nil, | |||
protoMatchFunc: nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to initialize this, the default is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we do it for signKey
to be explicit about it, as these are grouped together.
pubsub.go
Outdated
// be used by the protocol handler on the Host's Mux. Can be combined with | ||
// WithGossipSubProtocols feature function for checking if a protocol is supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say here "Should be combined ... for checking if certain protocol features are supported"
I finished the required changes. Thank you for the code review! |
thank you! |
This allows setting a custom protocol matching function.
Could potentially be used to allow semver matching