-
Notifications
You must be signed in to change notification settings - Fork 53
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
chore(api): validate rln message before sending (rest + rpc) #1968
Changes from all commits
96a55cb
6a4b28b
aa3c96a
0bcc43e
c964916
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,9 +191,8 @@ proc validator(pubsubTopic: string, message: messages.Message): Future[Validatio | |
proc isSubscribed*(w: WakuRelay, topic: PubsubTopic): bool = | ||
GossipSub(w).topics.hasKey(topic) | ||
|
||
iterator subscribedTopics*(w: WakuRelay): lent PubsubTopic = | ||
for topic in GossipSub(w).topics.keys(): | ||
yield topic | ||
proc subscribedTopics*(w: WakuRelay): seq[PubsubTopic] = | ||
return toSeq(GossipSub(w).topics.keys()) | ||
Comment on lines
+194
to
+195
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Morning! Is this directly related to the purpose of the PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope, but imho there is no need to use generators for this. the amount of topics is not enough to justify it. also using poorly documented features such as lent |
||
|
||
proc subscribe*(w: WakuRelay, pubsubTopic: PubsubTopic, handler: WakuRelayHandler) = | ||
debug "subscribe", pubsubTopic=pubsubTopic | ||
|
@@ -202,7 +201,7 @@ proc subscribe*(w: WakuRelay, pubsubTopic: PubsubTopic, handler: WakuRelayHandle | |
let wrappedHandler = proc(pubsubTopic: string, data: seq[byte]): Future[void] {.gcsafe, raises: [].} = | ||
let decMsg = WakuMessage.decode(data) | ||
if decMsg.isErr(): | ||
# fine if triggerSelf enabled, since validators are bypassed | ||
# fine if triggerSelf enabled, since validators are bypassed | ||
error "failed to decode WakuMessage, validator passed a wrong message", error = decMsg.error | ||
let fut = newFuture[void]() | ||
fut.complete() | ||
|
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.
unsure if its ok to use
validateMessage
here, since inside it updates a bunch of metrics and callupdateLog
. Any opinion @rymncThere 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.
yeah I don't think we should use validateMessage, because if you try publishing the same message it would get stuck since the proofMetadata would exist in the nullifierLog afaik. should probably split the function into the check, and then the state update
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.
Something like this 74d38f5 ?
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.
yes!