Skip to content
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

feat: lift contentTopics and make shardInfo mandatory for createLight… #1959

Merged
merged 13 commits into from
Apr 29, 2024

Conversation

weboko
Copy link
Collaborator

@weboko weboko commented Apr 15, 2024

Problem

End consumers might not want to understand the concept of sharding and wouldn't want to read the types.

Solution

Lift contentTopics option so that consumers just care about supplying a bunch of strings.

Notes

Additionally resolved #1980

@weboko weboko requested a review from a team as a code owner April 15, 2024 23:26
Copy link

github-actions bot commented Apr 15, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 181.17 KB (+0.05% 🔺) 3.7 s (+0.05% 🔺) 23.5 s (+10.44% 🔺) 27.1 s
Waku Simple Light Node 181.16 KB (+0.02% 🔺) 3.7 s (+0.02% 🔺) 21.5 s (+21.4% 🔺) 25.1 s
ECIES encryption 23.08 KB (0%) 462 ms (0%) 2.9 s (-32.11% 🔽) 3.3 s
Symmetric encryption 22.55 KB (0%) 452 ms (0%) 3.6 s (-39.94% 🔽) 4 s
DNS discovery 72.42 KB (0%) 1.5 s (0%) 11.7 s (+50% 🔺) 13.2 s
Peer Exchange discovery 74.1 KB (0%) 1.5 s (0%) 15.7 s (+47.46% 🔺) 17.2 s
Local Peer Cache Discovery 67.64 KB (0%) 1.4 s (0%) 14.1 s (+17.21% 🔺) 15.5 s
Privacy preserving protocols 38.87 KB (0%) 778 ms (0%) 6.5 s (-25.48% 🔽) 7.3 s
Waku Filter 111.58 KB (-0.17% 🔽) 2.3 s (-0.17% 🔽) 17.9 s (+11.11% 🔺) 20.1 s
Waku LightPush 110.15 KB (+0.05% 🔺) 2.3 s (+0.05% 🔺) 21.7 s (+117.7% 🔺) 23.9 s
History retrieval protocols 110.74 KB (+0.02% 🔺) 2.3 s (+0.02% 🔺) 15.9 s (+15.65% 🔺) 18.1 s
Deterministic Message Hashing 7.29 KB (0%) 146 ms (0%) 978 ms (-27.93% 🔽) 1.2 s

* If not provided pubsubTopics will be determined based on shardInfo
* See [Waku v2 Topic Usage Recommendations](https://github.com/vacp2p/rfc-index/blob/main/waku/informational/23/topics.md) for details.
*/
contentTopics?: string[];
Copy link
Collaborator Author

@weboko weboko Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A follow up to this PR will be:

  • deprecate or find better way of handling pubsubTopics parameter;
  • refine public API of it;
  • set default values to TWN fleet;
  • drop custom pubsubTopic support (also remove DefaultPubsubTopic from codebase and use DefaultShardInfo);

Will decouple into an issue.

@weboko weboko requested a review from danisharora099 April 29, 2024 09:25
@@ -41,6 +16,10 @@ export async function createNode(
export async function createLightNode(
options: CreateWakuNodeOptions = {}
): Promise<LightNode> {
if (!options.shardInfo || !options.contentTopics) {
throw new Error("Shard info must be set");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I am running on non-TWN network/pubsub? Does it mean that I need to provide contentTopics on the node creation time?

What I do in Qaku is I accept lightNode as a parameter for waku-dispatcher and only re-create the dispatcher instance (which needs to know the contentTopic to create encoder/decoder), but keep the node to speed up switching between Q&As

The need to provide contentTopic on node creation completely breaks this and I'd have to either provide some dummy contentTopic or re-create node on each Q&A switch, which would slow down the loading.

I understand this is necessary to be able to discover the right nodes supporting the right shards, but not sure if it will be practical for the apps.

Copy link
Member

@adklempner adklempner Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vpavlin are all the content topics for qwaku known beforehand or are they created dynamically? Either way, if you use the same application and version for all of them, then you shouldn't need to recreate the the node each time so long as you provide a dummy content topic at node creation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I am running on non-TWN network/pubsub

js-waku is moving towards dropping custom pubsub topics
if non-TWN can be described with ShardInfo - then it can passed just as is and should work, otherwise it won't be supported

note that not in this PR - I returned ability to run node with array of pubsubTopics but it will be removed next release

let's discuss if you think that we should handle it differently


as @adklempner mentioned you can pass different content topics that will resolve to the same shard - this can be ensured as per spec

@weboko weboko changed the title feat!: lift contentTopics and make shardInfo mandatory for createLight… feat: lift contentTopics and make shardInfo mandatory for createLight… Apr 29, 2024
@weboko weboko requested a review from vpavlin April 29, 2024 22:12
@weboko weboko merged commit 5b03709 into master Apr 29, 2024
9 of 10 checks passed
@weboko weboko deleted the weboko/content-topic-node branch April 29, 2024 23:47
@weboko weboko mentioned this pull request Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: fail when both pubsubTopics and shardInfo are specified
3 participants