-
Notifications
You must be signed in to change notification settings - Fork 8
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
Tic tac toe example #104
Tic tac toe example #104
Conversation
Co-authored-by: richΛrd <info@richardramos.me>
Co-authored-by: richΛrd <info@richardramos.me>
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.
Regarding the removal of the state pattern for the WakuNodeHandle, did it cause trouble? @SionoiS seems to like it :) #98 (review) and it has the benefit of not allowing you to 're-initialize' a node by mistake.
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
Maybe consider using a channel instead of a closure for handling LibwakuResponse
.
Keeping the closure internal and only exposing the receiving end of a channel.
waku-bindings/src/node/filter.rs
Outdated
pub fn waku_filter_subscribe( | ||
ctx: &WakuNodeContext, | ||
pubsub_topic: &str, | ||
content_topics: &str, // comma-separated list of content topics |
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.
maybe use a vector of strings to enforce the comment?
same below
waku-bindings/src/node/lightpush.rs
Outdated
pub fn waku_lightpush_publish_message( | ||
ctx: &WakuNodeContext, | ||
message: &WakuMessage, | ||
pubsub_topic: &str, |
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.
Even if we use string for topics in nwaku I would discourage this use in rust.
string bad mkay?
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.
Even if we use string for topics in nwaku I would discourage this use in rust.
string bad mkay?
Sorry but I can't quite follow the suggestion :)
EDITED: Richard clarified it, I'll try that
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.
Create a type PubsubTopic
and use it everywhere basically.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## nwaku #104 +/- ##
========================================
Coverage ? 60.66%
========================================
Files ? 16
Lines ? 1139
Branches ? 0
========================================
Hits ? 691
Misses ? 448
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Description
This PR adds a complete ui app example (tic tac toe) that interacts with waku nodes.
Special thanks to @richard-ramos and @SionoiS for the paramount help to achieve this complex milestone!
You will notice I'm removing an instance state approach (
struct WakuNodeHandle<State: WakuNodeState>
) that might sound more Rust-idiomatic. I'm happy to get it back if you consider it appropriate.