-
Notifications
You must be signed in to change notification settings - Fork 49
feat: add topic validators to pubsub #75
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
Conversation
db5afb5
to
5712fd1
Compare
Codecov Report
@@ Coverage Diff @@
## master #75 +/- ##
==========================================
+ Coverage 85.51% 85.68% +0.17%
==========================================
Files 11 11
Lines 497 510 +13
==========================================
+ Hits 425 437 +12
- Misses 72 73 +1
Continue to review full report at Codecov.
|
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.
Is there any reason for not adding this to libp2p-pubsub
? I think that this is the generic implementation and I would only expect the extended topic validators logic to be in this module
I've been viewing these two open PRs (#75 and #74) as sort of clean up tasks so that this Somehow it seemed simpler to explore this with my pseudo libp2p-pubsub impl before having the broader discussion about merging breaking functionality back upstream. Happy to make the PR to libp2p-pubsub if you think thats better tho. |
@wemeetagain seems a good approach. We can start here and then iterate to |
src/pubsub.js
Outdated
const validatorFn = this.topicValidators.get(topic) | ||
if (validatorFn) { | ||
const result = validatorFn(topic, peer, message) | ||
if (!this._processTopicValidatorResult(topic, peer, message, result)) { |
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.
return the function result instead of the condition?
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.
Process all the results instead of bailing after the first falsy processed result?
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 think that this was changed according to what I was thinking :)
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.
LGTM
Clean up validation in pubsub, add topic validator functions
validate
method now has additional peer arg and triggers topic-specific validation functionsMap<string, (topic, peer, message) => unknown>
_processTopicValidatorResult
- default to truthy results passing validationcc @jacobheun