feat: subscribe to content topic via SDK #1823

adklempner · 2024-02-02T01:33:34Z

Problem

There is currently no simple way to create a subscription with just a content topic.

Solution

Adds SDK functions that given a content topic will create a waku node and a subscription to the provided content topic. It assumes autosharding is being used.

Notes

Related to allow user to pass content topic to createSubscription #1764

github-actions · 2024-02-02T01:37:25Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
Waku core	0 B (-100% 🔽)	0 ms (-100% 🔽)	0 ms (-100% 🔽)	0 ms
Waku Simple Light Node	241.45 KB (+0.82% 🔺)	4.9 s (+0.82% 🔺)	499 ms (+31.46% 🔺)	5.4 s
ECIES encryption	27.55 KB (0%)	552 ms (0%)	251 ms (+52.34% 🔺)	802 ms
Symmetric encryption	26.87 KB (0%)	538 ms (0%)	160 ms (+12.78% 🔺)	698 ms
DNS discovery	92.51 KB (0%)	1.9 s (0%)	217 ms (+52.72% 🔺)	2.1 s
Privacy preserving protocols	51.13 KB (0%)	1.1 s (0%)	205 ms (+24.88% 🔺)	1.3 s
Light protocols	24.71 KB (-15.6% 🔽)	495 ms (-15.6% 🔽)	143 ms (+30.21% 🔺)	638 ms
History retrieval protocols	23.17 KB (-16.18% 🔽)	464 ms (-16.18% 🔽)	141 ms (-4.06% 🔽)	604 ms
Deterministic Message Hashing	6 KB (0%)	120 ms (0%)	77 ms (+81.96% 🔺)	196 ms
Waku node	241.39 KB (+100% 🔺)	4.9 s (+100% 🔺)	350 ms (+100% 🔺)	5.2 s

danisharora099

i like the direction, but imo the API can be simplified
can also be tackled as a followup
happy to discuss

packages/sdk/src/content_topic.ts

packages/tests/tests/sdk/content_topic.spec.ts

packages/sdk/src/content_topic.ts

danisharora099 · 2024-02-12T08:40:20Z

packages/sdk/src/content_topic.ts

+    ((await createLightNode({
+      shardInfo: { contentTopics: [contentTopic] }
+    })) as WakuNode);


why are we typecasting the type LightNode as WakuNode? if there's some API we require, we can add it part of the LightNode interface if necessary

replaced with LightNode in last commit

weboko · 2024-02-12T21:30:24Z

packages/sdk/src/content_topic.ts

+  shardInfoToPubsubTopics
+} from "@waku/utils";
+
+import { createLightNode } from ".";


nit: let's not do this kind of imports and explicitly mention source file

fixed in latest commit

weboko · 2024-02-12T21:34:01Z

packages/sdk/src/content_topic.ts

+    );
+
+  // Ensures there is a peer. Without this condition, the subscription will fail to create.
+  if (peer) {


If peer is needed for this function to work - it should be mandatory parameter then

updated in latest commit

let's add a condition with throw if peer is not provided as in the current state if run in vanilla JS - peer can be missed to be provided

i don't understand -- when peer is mandated as an arg in the function, why are we doing an if(peer) check

re usage in vanilla js: i would not add if-elses to handle those scenarios

removed the if clause in latest commit since peer is now required

weboko · 2024-02-12T21:50:10Z

packages/sdk/src/content_topic.ts

@@ -0,0 +1,109 @@
+import type { Multiaddr } from "@multiformats/multiaddr";


Overall I think it is the way to go - especially considering alignment on waku-org/pm#114 (comment)

With current implementation I think the main problem is ambiguity between existing filter.subscribe and these SDK functions.

To resolve that I think we should make these utils as close as possible to the node object so that they are essentially properties - this way we make sure:

they are accessible without any additional context such as import of it or knowledge of their existence;

consumer can use filter directly as well (which can be the case if they decide to build on top of raw RFC standard);

The most straightforward way I see is to move it to core and expose from here with additionally making a tech debt task.

Another way might be to move Waku implementation to SDK as this is utility and does not follow any RFC thus does not belong to core

nb: created a debt label just for that

@weboko I added a new class to SDK that extends WakuNode and gives it a function to create a subscription from a content topic. I think it makes sense to keep new, higher-level functions like this separate for now, and then decide on which can be moved inside the WakuNode class itself.

I'm not sure if the entire WakuNode class should be moved to sdk package just yet.

I don't think we need to introduce another abstraction as in most cases it will be needed to subscribe by contentTopic

Considering this I would move WakuNode all together to sdk and use util functions in content_topic.ts as private to WakuNode that way we keep only one facade and have additional method on the WakuNode to facilitate subscription by topic.

@weboko Moved WakuNode from core to sdk and added a function to create a subscription in latest commit

weboko · 2024-02-26T21:29:24Z

packages/sdk/src/index.ts

@@ -12,6 +7,8 @@ export {

 export { utf8ToBytes, bytesToUtf8 } from "@waku/utils/bytes";

+export * from "./content_topic.js";


nit: I don't really think we need to export it

I'd rather leave it in for now just because it breaks unit tests if these functions can't be accessed from tests package

I think we should test the entry point API - that means node.subscribeToContentTopic is the only thing that should be tested externally

weboko · 2024-02-26T21:35:13Z

packages/sdk/src/waku.ts

+        waku: this as LightNode,
+        peer
+      })
+    )[0];


nit: let's not do [0] access as it is not safe

updated in latest commit

weboko

left last round of comments

danisharora099 · 2024-02-27T16:35:17Z

packages/sdk/src/content_topic.ts

+    );
+
+  // Ensures there is a peer. Without this condition, the subscription will fail to create.
+  if (peer) {


i don't understand -- when peer is mandated as an arg in the function, why are we doing an if(peer) check

re usage in vanilla js: i would not add if-elses to handle those scenarios

packages/sdk/src/content_topic.ts

danisharora099 · 2024-02-28T09:08:35Z

packages/tests/tests/sdk/content_topic.spec.ts

+      })
+    ).waku;
+
+    expect((waku as WakuNode).pubsubTopics).to.include(expectedPubsubTopic);


why are we having to typecast it as WakuNode? are there any problems with implicit inference?

adklempner requested a review from a team as a code owner February 2, 2024 01:33

adklempner marked this pull request as draft February 2, 2024 01:33

adklempner force-pushed the feat/create-subscription-content-topic branch 2 times, most recently from 7b2bdba to c4a7ad1 Compare February 9, 2024 06:17

adklempner changed the title ~~feat: subscribe to content topic~~ feat: subscribe to content topic via SDK Feb 9, 2024

adklempner force-pushed the feat/create-subscription-content-topic branch from c4a7ad1 to 3550246 Compare February 9, 2024 06:20

adklempner marked this pull request as ready for review February 9, 2024 06:21

danisharora099 reviewed Feb 12, 2024

View reviewed changes

adklempner force-pushed the feat/create-subscription-content-topic branch 2 times, most recently from acb43ca to a5c278c Compare February 12, 2024 20:28

weboko reviewed Feb 12, 2024

View reviewed changes

adklempner force-pushed the feat/create-subscription-content-topic branch 9 times, most recently from 146c1f8 to ff948fc Compare February 19, 2024 20:29

adklempner force-pushed the feat/create-subscription-content-topic branch 4 times, most recently from bf05522 to 97789c8 Compare February 23, 2024 20:29

weboko reviewed Feb 26, 2024

View reviewed changes

danisharora099 reviewed Feb 27, 2024

View reviewed changes

adklempner force-pushed the feat/create-subscription-content-topic branch 2 times, most recently from fd4d4a8 to eb225aa Compare February 27, 2024 22:57

feat: create node and subscription by content topic

ee2d417

adklempner force-pushed the feat/create-subscription-content-topic branch from eb225aa to ee2d417 Compare February 27, 2024 23:47

danisharora099 approved these changes Feb 28, 2024

View reviewed changes

adklempner merged commit 78ee39a into master Feb 28, 2024
9 of 10 checks passed

adklempner deleted the feat/create-subscription-content-topic branch February 28, 2024 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: subscribe to content topic via SDK #1823

feat: subscribe to content topic via SDK #1823

adklempner commented Feb 2, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

danisharora099 left a comment

danisharora099 Feb 12, 2024

adklempner Feb 12, 2024

weboko Feb 12, 2024

adklempner Feb 13, 2024

weboko Feb 12, 2024

adklempner Feb 13, 2024

weboko Feb 26, 2024

danisharora099 Feb 27, 2024

adklempner Feb 28, 2024

weboko Feb 12, 2024 •

edited

Loading

adklempner Feb 13, 2024

weboko Feb 16, 2024

adklempner Feb 23, 2024

weboko Feb 26, 2024

adklempner Feb 27, 2024

weboko Feb 28, 2024

weboko Feb 26, 2024

adklempner Feb 27, 2024

weboko left a comment

danisharora099 Feb 27, 2024

danisharora099 Feb 28, 2024

		@@ -0,0 +1,109 @@
		import type { Multiaddr } from "@multiformats/multiaddr";

		@@ -12,6 +7,8 @@ export {

		export { utf8ToBytes, bytesToUtf8 } from "@waku/utils/bytes";

		export * from "./content_topic.js";

feat: subscribe to content topic via SDK #1823

feat: subscribe to content topic via SDK #1823

Conversation

adklempner commented Feb 2, 2024 • edited Loading

Problem

Solution

Notes

github-actions bot commented Feb 2, 2024 • edited Loading

size-limit report 📦

danisharora099 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weboko Feb 12, 2024 • edited Loading

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

weboko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adklempner commented Feb 2, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

weboko Feb 12, 2024 •

edited

Loading