Skip to content
This repository was archived by the owner on Jun 26, 2023. It is now read-only.

fix!: add topicValidators to pubsub interface#298

Merged
mpetrunic merged 4 commits intomasterfrom
fix/missing-pubsub-topic-validator-fn
Oct 11, 2022
Merged

fix!: add topicValidators to pubsub interface#298
mpetrunic merged 4 commits intomasterfrom
fix/missing-pubsub-topic-validator-fn

Conversation

@mpetrunic
Copy link
Copy Markdown
Member

@mpetrunic mpetrunic commented Oct 6, 2022 •
edited
Loading

resolves #297

Still need to:

@mpetrunic mpetrunic mentioned this pull request Oct 6, 2022
Closed
@mpetrunic
Copy link
Copy Markdown
Member Author

mpetrunic commented Oct 6, 2022

cc @tuyen @dapplion I've dropped topic from params in topic validator function as you could do something like:
topicValidators.set(topic, createTopicValidator(topic))

@mpetrunic mpetrunic changed the title feat!: add topicValidators to pubsub interface fix!: add topicValidators to pubsub interface Oct 6, 2022
@mpetrunic mpetrunic marked this pull request as ready for review October 6, 2022 12:02
@achingbrain
Copy link
Copy Markdown
Member

achingbrain commented Oct 6, 2022

What's the reasoning behind this being a field on the public API and not an implementation-specific field?

Given that it's part of the public API and Maps can be mutated in place are we ok with validators being added and removed at runtime?

@achingbrain
Copy link
Copy Markdown
Member

achingbrain commented Oct 6, 2022

I guess it is in the spec so maybe we are fine with it?

@mpetrunic
Copy link
Copy Markdown
Member Author

mpetrunic commented Oct 6, 2022

What's the reasoning behind this being a field on the public API and not an implementation-specific field?

Given that it's part of the public API and Maps can be mutated in place are we ok with validators being added and removed at runtime?

Spec says it's mandatory for every implementation so it makes sense to have it in public API and shared between implementations. From what I see, none of the implementations are caching it and support changing it at runtime (which spec doesn't say it must be supported):

@mpetrunic
Copy link
Copy Markdown
Member Author

mpetrunic commented Oct 6, 2022

If we aren't gonna adopt this in interfaces, we should remove/change example which says this is possible: https://github.com/libp2p/js-libp2p/blob/master/examples/pubsub/message-filtering/README.md

@achingbrain
Copy link
Copy Markdown
Member

achingbrain commented Oct 6, 2022

No, I guess this just got missed during some of the refactorings.

If this works out of the box with floodsub and gossipsub it's probably safe to release it as a minor, right?

@mpetrunic
Copy link
Copy Markdown
Member Author

mpetrunic commented Oct 6, 2022

No, I guess this just got missed during some of the refactorings.

If this works out of the box with floodsub and gossipsub it's probably safe to release it as a minor, right?

Both floodsub/pubsub and gossipsup has a different implementation. I took a bit of both.
floodsub - expects validator function to throw error and doesn't pass peerId as param
gossipsup - returns accept/ignore/reject, but passes in topic param unnessesary

achingbrain
achingbrain approved these changes Oct 6, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM, just a small nit about the comments

@mpetrunic
Copy link
Copy Markdown
Member Author

mpetrunic commented Oct 7, 2022 •
edited
Loading

We should probably allow @wemeetagain @dapplion and @tuyennhv to review as this impacts gossipsub

@achingbrain
Copy link
Copy Markdown
Member

achingbrain commented Oct 7, 2022

@mpetrunic could you PR floodsub and gossipsub with the updates necessary to support this interface change?

@mpetrunic
Copy link
Copy Markdown
Member Author

mpetrunic commented Oct 7, 2022

@mpetrunic could you PR floodsub and gossipsub with the updates necessary to support this interface change?

already have changes locally for pubsub/floodsub, will do gossipsub

@mpetrunic mpetrunic mentioned this pull request Oct 7, 2022
Merged
dapplion
dapplion approved these changes Oct 7, 2022
View reviewed changes
Copy link
Copy Markdown

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Requiring functions to throw is a bad pattern I've been regreting in Lodestar for a while. Exceptions should be exceptional, and a message getting a REJECT, IGNORE is expected behaviour.

topicValidators have to be mutable at runtime too.

So these changes LGTM

@mpetrunic mpetrunic merged commit e5ff819 into master Oct 11, 2022
@mpetrunic mpetrunic deleted the fix/missing-pubsub-topic-validator-fn branch October 11, 2022 13:19
github-actions bot pushed a commit that referenced this pull request Oct 11, 2022
@semantic-release-bot
chore(release): 3.0.0 [skip ci]
e074f10 
## [@libp2p/interface-pubsub-v3.0.0](https://github.com/libp2p/js-libp2p-interfaces/compare/@libp2p/interface-pubsub-v2.1.0...@libp2p/interface-pubsub-v3.0.0) (2022-10-11)

### ⚠ BREAKING CHANGES

* add topicValidators to pubsub interface (#298)

### Bug Fixes

* add topicValidators to pubsub interface ([#298](#298)) ([e5ff819](e5ff819))
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 11, 2022

🎉 This PR is included in version @libp2p/interface-pubsub-v3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request Oct 11, 2022
@semantic-release-bot
chore(release): 3.0.0 [skip ci]
5f68c1b 
## [@libp2p/interface-pubsub-compliance-tests-v3.0.0](https://github.com/libp2p/js-libp2p-interfaces/compare/@libp2p/interface-pubsub-compliance-tests-v2.0.5...@libp2p/interface-pubsub-compliance-tests-v3.0.0) (2022-10-11)

### ⚠ BREAKING CHANGES

* add topicValidators to pubsub interface (#298)

### Bug Fixes

* add topicValidators to pubsub interface ([#298](#298)) ([e5ff819](e5ff819))

### Dependencies

* update sibling dependencies ([8f3680e](8f3680e))
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 11, 2022

🎉 This PR is included in version @libp2p/interface-pubsub-compliance-tests-v3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wemeetagain wemeetagain Awaiting requested review from wemeetagain

3 more reviewers

@achingbrain achingbrain achingbrain approved these changes

@twoeths twoeths twoeths approved these changes

@dapplion dapplion dapplion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Broken pubsub type when using external pubsub library

4 participants

@mpetrunic @achingbrain @twoeths @dapplion