Toy chat fix #105

Ivansete-status · 2024-12-19T22:07:47Z

This PR aims for recovering toy chat powered by libwaku/nwaku.

This PR was motivated by Atoma, who wanted to start using it and we needed to prepare the waku-rust-bindings for them.

Additionally:

Bump nwaku to the current master branch ( commit: 625c8ee5 )
Suggests to make the waku crate to behave tokio-asynchronously
Use of store
Use of lightpush and filter
add waku-bindings/src/general/messagehash.rs
add waku-bindings/src/general/time.rs
add waku-bindings/src/general/waku_decode.rs
add WakuEvent management (WakuMessage, ConnectionChange, TopicHealthChange.)
add waku-bindings/src/macros.rs

Part of

Maintenance Y2025Q1 #106

danielSanchezQ

I think its nice, I would check on my comment to see if it can be improved. But nice job overall.

danielSanchezQ · 2024-12-20T09:46:18Z

waku-bindings/src/node/filter.rs

+    let mut result = LibwakuResponse::default();
+    let notify = Arc::new(Notify::new());
+    let notify_clone = notify.clone();
+    let result_cb = |r: LibwakuResponse| {
+        result = r;
+        notify_clone.notify_one(); // Notify that the value has been updated
+    };


I think this pattern its ok but should be simpler if you use a channel to return the result. You can probably use kanal a mixed flavored channel (sync/async). To simplify it.

danielSanchezQ · 2024-12-20T09:51:12Z

@Ivansete-status Are the nwaku calls on the other side of the FFI are async?

Ivansete-status · 2024-12-20T10:26:04Z

@Ivansete-status Are the nwaku calls on the other side of the FFI are async?

Yes. Everything in nwaku is async and the interaction with the library is async too. To elaborate a bit more, libwaku runs a separate thread, a.k.a. The Waku Thread, which is in charge of running the Waku node and also, attending the app's requests (waku_new, waku_start, etc.)

richard-ramos · 2024-12-20T13:31:02Z

waku-sys/build.rs

@@ -68,12 +68,6 @@ fn generate_bindgen_code(project_dir: &Path) {

    println!("cargo:rustc-link-lib=stdc++");

-    println!(


Negentropy is no longer needed to build libwaku?! that's very cool!

richard-ramos · 2024-12-20T18:12:22Z

examples/tic-tac-toe-gui/src/main.rs

+        // let ctopic = WakuContentTopic::new("waku", "2", "tictactoegame", Encoding::Proto);
+        // let content_topics = vec![ctopic];
+        // waku.filter_subscribe(&self.game_topic, content_topics).await.expect("waku should subscribe");


Maybe a comment can be added above these lines to explain what is their purpose (i.e. to indicate that filter subscriptions are done with it)

richard-ramos · 2024-12-20T18:12:44Z

examples/tic-tac-toe-gui/src/main.rs

+            dbg!(format!("message hash published: {}", msg_hash));
+        }
+
+        // self.waku.lightpush_publish_message(&message, &self.game_topic);


Same comment as before. Add a comment to indicate what is this about

richard-ramos · 2024-12-20T18:15:57Z

waku-bindings/src/node/events.rs

-        let s = "{\"eventType\":\"message\",\"messageHash\":\"0x26ff3d7fbc950ea2158ce62fd76fd745eee0323c9eac23d0713843b0f04ea27c\",\"pubsubTopic\":\"/waku/2/default-waku/proto\",\"wakuMessage\":{\"payload\":\"SGkgZnJvbSDwn6aAIQ==\",\"contentTopic\":\"/toychat/2/huilong/proto\",\"timestamp\":1665580926660}}";
-        let evt: Event = serde_json::from_str(s).unwrap();
-        assert!(matches!(evt, Event::WakuMessage(_)));
+        let s = "{\"eventType\":\"message\",\"messageHash\":[91, 70, 26, 8, 141, 232, 150, 200, 26, 206, 224, 175, 249, 74, 61, 140, 231, 126, 224, 160, 91, 80, 162, 65, 250, 171, 84, 149, 133, 110, 214, 101],\"pubsubTopic\":\"/waku/2/default-waku/proto\",\"wakuMessage\":{\"payload\":\"SGkgZnJvbSDwn6aAIQ==\",\"contentTopic\":\"/toychat/2/huilong/proto\",\"timestamp\":1665580926660}}";


Why use a numeric array instead of a byte array?
reason why I ask is that one of the advantages of json is that it's human readable (not much tbf), Then, comparing the byte array with the hash, it seems to me that a hash is much more friendly.
(Although tbf, we do use a MessageHash type that abstracts the dev from the array)

@richard-ramos - yes good point and I completely agree the need of having readable hex.
I found this is the most convenient approach of handling message hash from store and relay. Also, in the messagehash.rs module we have the following, which is interesting to "print" the msg_hash content:

// Implement the Display trait impl fmt::Display for MessageHash { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.to_hex_string()) } }

richard-ramos · 2024-12-20T18:17:36Z

waku-bindings/src/node/filter.rs

+    let mut result = LibwakuResponse::default();
+    let notify = Arc::new(Notify::new());
+    let notify_clone = notify.clone();
+    let result_cb = |r: LibwakuResponse| {
+        result = r;
+        notify_clone.notify_one(); // Notify that the value has been updated
+    };


This patterns tends to repeat consistently. I wonder if we can maybe extract this into a function?

richard-ramos · 2024-12-20T18:20:20Z

waku-bindings/src/node/mod.rs

+        let one_day_in_secs = 60 * 60 * 24;
+        let time_start = (Duration::from_secs(Utc::now().timestamp() as u64)
+            - Duration::from_secs(one_day_in_secs))
+        .as_nanos() as usize;


Shoudn't the time-start and time-end be optional duration's passed by the developer?

richard-ramos · 2024-12-20T18:21:47Z

waku-bindings/src/node/mod.rs

+                true,             // pagination_forward
+                Some(25),         // pagination_limit,


Should probably be parameters as well.

Perhaps for all of the parameters of the query it makes sense to define a query object that can be built with an options pattern (https://medium.com/@omid.jn/options-pattern-in-rust-6425520b2b23)

richard-ramos · 2024-12-20T18:22:18Z

waku-bindings/src/node/mod.rs

+                true,             // pagination_forward
+                Some(25),         // pagination_limit,
+                peer_addr,
+                None, // timeout_millis


Timeout should probably be a parameter given by the developer

richard-ramos · 2024-12-20T18:25:45Z

waku-bindings/src/node/store.rs

+#[serde(rename_all = "camelCase")]
+pub struct StoreWakuMessageResponse {
+    pub message_hash: MessageHash,
+    pub message: WakuStoreRespMessage,


IIRC, include_data will determine whether a message will be returned or not, so probably makes sense for message to be an option

Yes correct :) We are using the include_data concept in the requests. I think is fine unless I'm missing something

But wouldn't that mean that if you send include_data: false, your message will contain a (invalid) struct with default values? having it as an option perhaps would make it clearer for the dev that there's no message available

Addressed in f1e4e02

richard-ramos · 2024-12-20T18:26:09Z

waku-bindings/src/general/mod.rs

+    pub timestamp: usize,
+    #[serde(default)]
+    pub ephemeral: bool,
+    // pub proof: Vec<u8>,


Should this be uncommented?

richard-ramos · 2025-01-09T13:20:31Z

waku-bindings/src/node/mod.rs

+        include_data: bool, // is true, resp contains payload, etc. Only msg_hashes otherwise
+        time_start: Option<u64>, // unix time nanoseconds
+        time_end: Option<u64>, // unix time nanoseconds
+        timeout_millis: Option<i32>,


Probably should be an Option<Duration>

Addressed in 3b0015d

danielSanchezQ

Rusty stuff 🦀
Just left a few comments/nitpicks. Nothing critic.

danielSanchezQ · 2025-01-09T13:57:07Z

waku-bindings/src/general/messagehash.rs

+    }
+}
+
+impl Hash for MessageHash {


Couldn't this be derived directly?

Addressed in 566fae9
Thanks so much indeed for the lesson :)

danielSanchezQ · 2025-01-09T14:00:46Z

waku-bindings/src/general/messagehash.rs

+impl<'de> Deserialize<'de> for MessageHash {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        // Deserialize the input as a vector of u8
+        let vec: Vec<u8> = Deserialize::deserialize(deserializer)?;
+
+        // Ensure the vector has exactly 32 elements
+        let array: [u8; 32] = vec
+            .try_into()
+            .map_err(|_| serde::de::Error::custom("Expected an array of length 32"))?;
+
+        Ok(MessageHash(array))
+    }
+}


Also couldn't use derive?
This would be useful to implement manually if you want to discern "human" readable (the hex_bytes you have from the parsing method above for example) form a simple array of bytes.

Addressed in 6541dc3
Super happy about it, thanks!

danielSanchezQ · 2025-01-09T14:02:32Z

waku-bindings/src/general/waku_decode.rs

+use crate::general::Result;
+use multiaddr::Multiaddr;
+// Define the WakuDecode trait
+pub trait WakuDecode: Sized {
+    fn decode(input: &str) -> Result<Self>;
+}
+
+impl WakuDecode for String {
+    fn decode(input: &str) -> Result<Self> {
+        Ok(input.to_string())
+    }
+}
+
+pub fn decode<T: WakuDecode>(input: String) -> Result<T> {
+    T::decode(input.as_str())
+}
+
+impl WakuDecode for Vec<Multiaddr> {
+    fn decode(input: &str) -> Result<Self> {
+        input
+            .split(',')
+            .map(|s| s.trim().parse::<Multiaddr>().map_err(|err| err.to_string()))
+            .collect::<Result<Vec<Multiaddr>>>() // Collect results into a Vec
+            .map_err(|err| format!("could not parse Multiaddr: {}", err))
+    }
+}


Why not use FromStr directly? Otherwise, you could probably implement WakuDecode for T where T: FromStr.

I've tried to apply that suggestion. At some point I came across an issue when trying to implement the FromStr trait for Vec<Multiaddr> and the problem is that Multiaddr is defined in an external crate.

I tried with the following but it requires Vec<Multiaddr> to implement FromStr , which I'm afraid isn't doable:

impl<T> WakuDecode for T where T: FromStr, T::Err: ToString, { fn decode(input: &str) -> Result<Self> { T::from_str(input).map_err(|e| e.to_string()) } }

~~Multiaddr do not impl FromStr?. Well, you could implement FromStr for a wrapper type MultiaddrVec(Vec<Multiaddr>) and the implementation would go through.~~ Ah, yeah, Vec doesn't impl FromStr ofc. This are just nitpicks. But you could then impl WakuDecode for Vec<T> where T: WakuDecode. Sadly you cannot provide customized implementations yet for different types. It would be nice to implement both.
TLDR: it is ok as it is 😉

danielSanchezQ · 2025-01-09T14:05:36Z

waku-bindings/src/node/mod.rs

+        loop {
+            let query = StoreQueryRequest::new()
+                .with_pubsub_topic(pubsub_topic.clone())
+                .with_content_topics(content_topics.clone())
+                .with_include_data(include_data)
+                .with_time_start(time_start)
+                .with_time_end(time_end)
+                .with_pagination_cursor(cursor)
+                .with_pagination_forward(true);
+
+            let response =
+                store::waku_store_query(&self.ctx, query, peer_addr, timeout_millis).await?;
+
+            messages.extend(response.messages);
+
+            if response.pagination_cursor.is_none() {
+                break;
+            }
+            cursor = response.pagination_cursor;
+        }
+
+        messages.reverse();
+
+        Ok(messages)


It makes no sense too much for this method to be asynchronous as it stands. I would return a stream of values (requested asynchronously) instead so it can be consumed later by the caller if needed. Otherwise, you keep the asynchronous API without allocation into a Vec.

…ug msgs

Ivansete-status added 9 commits November 29, 2024 16:19

update waku-sys to latest nwaku master

ad0eab2

some changes to add toy-chat example. not completed yet

0d37037

update waku-sys/vendor to libwaku-store-enhancements branch

be987f8

progress in toy-chat

3f72fc7

proper use of messagehash and store

345dcbe

add nimcache folder to gitignore

51ea3be

compilation fixes and update wakusys/vendor

a5338bf

waku-sys update

9d73660

wide adaptations to make the waku crate behave tokio-based async

e937e05

github-actions bot assigned Ivansete-status Dec 19, 2024

danielSanchezQ reviewed Dec 20, 2024

View reviewed changes

Ivansete-status mentioned this pull request Dec 20, 2024

chore: enhance libwaku store protocol and more waku-org/nwaku#3223

Merged

Ivansete-status added 2 commits December 20, 2024 18:05

make sure the tests run in serial and the waku_destroy is called

666bc73

simple changes to make clippy happier

dd29900

richard-ramos reviewed Dec 20, 2024

View reviewed changes

Ivansete-status added 12 commits December 22, 2024 14:42

tic-tac-toe main add some more comments

f27cda7

avoid duplicated code and better separate structs in modules

08b8190

uncomment proof attribute from WakuStoreRespMessage

14aa84f

use uuid instead of hard-coded request id in store query

eb4286b

expose include_data to expert devs for store requests fn

40e908f

better time management

3cbb259

apply better option approach in store queries

80ea725

fix test compilation issue and add some more time for waku

f2a6c8e

tic-tac-toe minor fixes

5b0204f

avoid warnings

863f208

avoid clippy errors

1052dca

simple change to avoid fmt warning

a6a5988

richard-ramos approved these changes Dec 23, 2024

View reviewed changes

Ivansete-status added 9 commits December 23, 2024 15:26

waku-sys build rm dependency with libc++ avoiding macos issues

2544150

clippy changes

cc76ac0

add on_exit todo so that the waku node is stopped correctly

35bacf4

build.rs cleanup avoid linking ssl crypto and pthread

e25050e

general mod.rs remove empty line to avoid clippy picky issue

2d507c4

start handling topic health event

d758d72

Some code simplification in filter

863d3ae

few adaptations to use latest nwaku master

4cdbacd

fix test issue that didn't handle events properly

bbcf401

richard-ramos reviewed Jan 9, 2025

View reviewed changes

danielSanchezQ approved these changes Jan 9, 2025

View reviewed changes

Ivansete-status added 4 commits January 9, 2025 22:31

use of Option in StoreWakuMessageResponse for responses with or witho…

f1e4e02

…ug msgs

store use Duration type instead of i32 in waku_store_query exposed fn

3b0015d

make messagehash to derive from Hash

566fae9

messagehash directly derive from Deserialize

6541dc3

Ivansete-status linked an issue Jan 10, 2025 that may be closed by this pull request

Maintenance Y2025Q1 #106

Open

Ivansete-status removed a link to an issue Jan 10, 2025

Maintenance Y2025Q1 #106

Open

Ivansete-status merged commit 0c0b834 into nwaku Jan 10, 2025
5 of 6 checks passed

Ivansete-status deleted the toy-chat-fix branch January 10, 2025 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toy chat fix #105

Toy chat fix #105

Ivansete-status commented Dec 19, 2024 •

edited

Loading

danielSanchezQ left a comment

danielSanchezQ Dec 20, 2024

danielSanchezQ commented Dec 20, 2024

Ivansete-status commented Dec 20, 2024

richard-ramos Dec 20, 2024

richard-ramos Dec 20, 2024

richard-ramos Dec 20, 2024

richard-ramos Dec 20, 2024

Ivansete-status Dec 22, 2024

richard-ramos Dec 20, 2024

richard-ramos Dec 20, 2024

richard-ramos Dec 20, 2024

richard-ramos Dec 20, 2024

richard-ramos Dec 20, 2024

Ivansete-status Dec 22, 2024

richard-ramos Dec 23, 2024

Ivansete-status Jan 10, 2025

richard-ramos Dec 20, 2024

richard-ramos Jan 9, 2025

Ivansete-status Jan 10, 2025 •

edited

Loading

danielSanchezQ left a comment

danielSanchezQ Jan 9, 2025

Ivansete-status Jan 10, 2025

danielSanchezQ Jan 9, 2025

Ivansete-status Jan 10, 2025

danielSanchezQ Jan 9, 2025

Ivansete-status Jan 9, 2025

danielSanchezQ Jan 10, 2025 •

edited

Loading

danielSanchezQ Jan 9, 2025

		@@ -68,12 +68,6 @@ fn generate_bindgen_code(project_dir: &Path) {

		println!("cargo:rustc-link-lib=stdc++");

		println!(

Toy chat fix #105

Toy chat fix #105

Conversation

Ivansete-status commented Dec 19, 2024 • edited Loading

Part of

danielSanchezQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielSanchezQ commented Dec 20, 2024

Ivansete-status commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

danielSanchezQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielSanchezQ Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status commented Dec 19, 2024 •

edited

Loading

Ivansete-status Jan 10, 2025 •

edited

Loading

danielSanchezQ Jan 10, 2025 •

edited

Loading