-
Notifications
You must be signed in to change notification settings - Fork 57
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(bindings): base64 payload and key for content topic #2435
Conversation
You can find the image built from this PR at
Built from 3fed8ac |
3b78991
to
0c34551
Compare
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 thanks!
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! Thanks for it!
Just added a couple of nitpick comments xD
proc toWakuMessage*(self: JsonMessage): WakuMessage = | ||
let payloadRes = base64.decode(self.payload) | ||
if payloadRes.isErr(): | ||
raise newException(ValueError, "invalid payload format: " & payloadRes.error) |
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.
Nitpick, if possible I'd avoid using exceptions:)
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.
Replaced by a Result in a4f7b88
version: uint32(self.version), | ||
timestamp: self.timestamp, | ||
ephemeral: self.ephemeral | ||
) |
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.
At first sight I missed having the rln proof but I believe this is something we will add in the future when we expose some kind of publish_rln
function
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.
Ended up adding the proof in a4f7b88 so it's now 'future-ready' for when we include rln functionality to bindings
library/libwaku.nim
Outdated
@@ -43,14 +45,14 @@ const RET_MISSING_CALLBACK: cint = 2 | |||
proc relayEventCallback(ctx: ptr Context): WakuRelayHandler = | |||
return proc (pubsubTopic: PubsubTopic, msg: WakuMessage): Future[system.void]{.async.} = | |||
# Callback that hadles the Waku Relay events. i.e. messages or errors. | |||
if not isNil(ctx[].eventCallback): | |||
if not isNil(ctx[].eventCallback) and not isNil(ctx[].eventUserData): |
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.
Given that we now have two conditions that should be met, I wonder if we could split the validation in two, and apply some kind of "return early pattern". The following article explains the idea much better:
https://medium.com/swlh/return-early-pattern-3d18a41bba8
lmk what do you think :)
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.
Sounds good! I removed the validation from the if
and executed those following the 'return early' pattern as described in the article you shared.
a4f7b88
2e18057
to
a4f7b88
Compare
Description
Various fixes to the bindings to issues identified while attempting to integrate nwaku with waku-rust-bindings
Changes
waku_set_event_callback
is invokedcontentTopic
instead ofcontent_topic
in the message JSON