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

fix: adding rln validator as default #2367

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

gabrielmer
Copy link
Contributor

@gabrielmer gabrielmer commented Jan 24, 2024

Description

It was noticed that RLN validator wasn't configured for dynamically subscribed pubsub topics.
Therefore, we decided to implement the same approach to validators as in go-waku: we're adding now support for default validators, which will be run for all pubsub topics. We're adding RLN validator to the default validators whenever RLN is enabled.

Changes

  • added default validators, which run for all pubsub topics
  • included RLN validator to the default validators when RLN is mounted

How to test

Simulated the scenario in which we send an invalid message on a dynamically added pubsub topics with and without this fix.

With this fix:
Screenshot 2024-01-26 at 17 42 07

Without it:
Screenshot 2024-01-26 at 17 42 46

Thanks @AlejandroCabeza for it!
Not adding the test in this PR as it is added in Alex's PR of the RLN test suite

Also, thanks @chaitanyaprem for helping from go-waku side to maintain consistency between both :)

Issue

closes #2365

Copy link

github-actions bot commented Jan 24, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2367

Built from 6d28045

@gabrielmer gabrielmer self-assigned this Jan 25, 2024
@gabrielmer gabrielmer force-pushed the fix-adding-rln-validator-as-default branch from 0deb882 to cc4c5b2 Compare January 26, 2024 16:49
@gabrielmer gabrielmer changed the title fix: (DEBUG) adding rln validator as default fix: adding rln validator as default Jan 26, 2024
@gabrielmer gabrielmer marked this pull request as ready for review January 26, 2024 17:05
Copy link
Contributor

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM

@gabrielmer gabrielmer merged commit bb58a63 into master Jan 29, 2024
13 of 14 checks passed
@gabrielmer gabrielmer deleted the fix-adding-rln-validator-as-default branch January 29, 2024 15:11
@@ -130,6 +130,8 @@ type
# a map that stores whether the ordered validator has been inserted
# for a given PubsubTopic
validatorInserted: Table[PubsubTopic, bool]
# seq of validators that are called for every pubsub topic
wakuDefaultValidators: seq[WakuValidatorHandler]
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure i understand this.
imho if rln is enabled, it should be enabled in all topics. same applies for all validators.

so not sure i understand the difference between validators and default validators. Having addValidator and addDefaultValidator creates some cognitive load for a use case that IMHO we don't have?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: RLN validator is only added for statically configured pubsub topics
4 participants