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

feature/filter #63

Merged
merged 16 commits into from
Jul 22, 2020
Merged

feature/filter #63

merged 16 commits into from
Jul 22, 2020

Conversation

decanus
Copy link
Contributor

@decanus decanus commented Jul 21, 2020

No description provided.

@decanus decanus requested a review from kdeme July 21, 2020 14:41
@@ -81,6 +83,7 @@ method subscribeTopic*(w: WakuSub,
peerId: string) {.async, gcsafe.} =
proc handler(topic: string, data: seq[byte]) {.async, gcsafe.} =
info "Hit NOOP handler", topic
w.filters.notify(topic, data)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are mixing the GossipSub/FloodSub topic(s) here with the ones of Waku. The current idea/plan is that these are not the same.

So the way I see to do this is as follows: a Waku subscribe should subscribe a WakuSub handler on the "general" chosen topic (=GossipSub/FloodSub topic) for WakuSub communication, and use that WakuSub handler, to filter messages according the provided Waku topics, and then trigger the Waku handler.
I realize this probably sounds very confusing :), the naming isn't great.

And that would be in the case of full nodes. Light nodes would require another system probably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so then I have another problem. I need to decode the message. But from my understanding we do not yet have a message format do we? Hence my asking in nim-waku. Maybe @oskarth could also elaborate here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea what this PR is even trying to do

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have a message format. It is documented here: https://specs.vac.dev/specs/waku/waku-v2.html#message

What isn't fully specced out there is: (a) how to deal with waku/content topics (not pubsub topics) and (b) how to deal with encryption of payload. Neither is necessary at this stage for the issues discussed on e.g. Monday.

For (a) there is vacp2p/rfc#156 and vacp2p/rfc#160 For (b) there is vacp2p/rfc#158

You don't need either to get a generic Topic Handler going.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you make a good point that this extra filter that doesn't do anything currently (as it uses the same topic) is not needed to make this work. I thought it was fine as in my envision of the API, the filter would eventually filter messages on Waku topics which are provided through the subscribe call.

Copy link
Contributor

@kdeme kdeme left a comment

Choose a reason for hiding this comment

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

OK for now considering the added comment & created issue.

@decanus decanus merged commit 0799ef9 into waku-org:master Jul 22, 2020
@oskarth
Copy link
Contributor

oskarth commented Jul 23, 2020

What is this and where does it come from? There is no PR description and no issue.

Skimming briefly it seems like it is using a filter mechanism as exists in v1, but this isn't something we have discussed explicitly for v2

@decanus
Copy link
Contributor Author

decanus commented Jul 23, 2020

Hey sorry for the unclarity @oskarth, so this looks very close to the filter system from v1, the difference is we do not validate messages. The point of it right now is very simple pub/sub in the architecture. This was needed in a basic form so I can skim all receieved messages and forward them to the historic message part of the application to then archive messasges etc.

staheri14 pushed a commit that referenced this pull request Oct 29, 2020
* simplified filter

* add filter

* eol

* Update waku_protocol2.nim

* trigger GitHub actions

* comment

* fix import

* oops

* and

* init filters

* import tables
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.

3 participants