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(static-sharding)!: allow multiple pubSubTopics #1586

Merged
merged 34 commits into from
Sep 27, 2023

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Sep 20, 2023

This PR, part of the #1310 milestone:

  • allows consumers to set multiple PubSubTopic during node creation
    • keeps our DefaultPubSubTopic still as default
  • introduced PubSubTopic as an argument for the encoder/decoder creation API
  • updates all protocols (lightpush, filter, store and relay) to confirm that the request can only be sent to one of the PubSubTopic that were used during initialisation
  • updates existing tests for the new API

Notes

Next

  • ConnectionManager
    • only connect with nodes that support relevant shards
  • Discovery
    • update rs/rsv values for peers

@danisharora099 danisharora099 requested a review from a team as a code owner September 20, 2023 13:29
@danisharora099 danisharora099 mentioned this pull request Sep 20, 2023
9 tasks
@github-actions
Copy link

github-actions bot commented Sep 20, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 27.81 KB (+0.46% 🔺) 557 ms (+0.46% 🔺) 1.4 s (-0.39% 🔽) 2 s
Waku Simple Light Node 262.97 KB (+0.12% 🔺) 5.3 s (+0.12% 🔺) 3.8 s (-8.73% 🔽) 9.1 s
ECIES encryption 29.56 KB (+3% 🔺) 592 ms (+3% 🔺) 1.6 s (+25.04% 🔺) 2.2 s
Symmetric encryption 29.56 KB (+3% 🔺) 592 ms (+3% 🔺) 1.7 s (-7.59% 🔽) 2.3 s
DNS discovery 110.58 KB (+0.03% 🔺) 2.3 s (+0.03% 🔺) 2.4 s (-25.86% 🔽) 4.6 s
Privacy preserving protocols 117.31 KB (+0.05% 🔺) 2.4 s (+0.05% 🔺) 3.4 s (+0.92% 🔺) 5.7 s
Light protocols 25.96 KB (+0.5% 🔺) 520 ms (+0.5% 🔺) 1.4 s (+16.18% 🔺) 1.9 s
History retrieval protocols 25.13 KB (+0.91% 🔺) 503 ms (+0.91% 🔺) 1.1 s (+16.28% 🔺) 1.6 s
Deterministic Message Hashing 5.65 KB (0%) 113 ms (0%) 583 ms (+22.66% 🔺) 696 ms

Copy link
Collaborator

@fryorcraken fryorcraken left a comment

Choose a reason for hiding this comment

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

Preliminary review. blocking on the multiple store requests

Copy link
Collaborator

@fryorcraken fryorcraken left a comment

Choose a reason for hiding this comment

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

Missing various tests:

  • initiate with multiple pubsub topic
  • try to use any protocol with a pubsub topic that is not configured.

Copy link
Collaborator

@fryorcraken fryorcraken left a comment

Choose a reason for hiding this comment

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

I think relay could have been done with a 2nd PR to keep reviewing easy.

@@ -41,12 +42,12 @@ type PreparePushMessageResult =
* Implements the [Waku v2 Light Push protocol](https://rfc.vac.dev/spec/19/).
*/
class LightPush extends BaseProtocol implements ILightPush {
options: ProtocolCreateOptions;
private readonly pubsubTopics: PubSubTopic[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
private readonly pubsubTopics: PubSubTopic[];
private readonly pubSubTopics: PubSubTopic[];

Copy link
Collaborator Author

@danisharora099 danisharora099 Sep 26, 2023

Choose a reason for hiding this comment

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

refactored this, thanks for the spot.

perhaps this change should be part of a larger code-wide refactor? tracking here: https://github.com/waku-org/js-waku/compare/chore/pubSubTopic-typo?expand=1

danisharora099 and others added 6 commits September 26, 2023 13:49
Co-authored-by: fryorcraken <110212804+fryorcraken@users.noreply.github.com>
* refactor: simplify handling of observers

* refactor: Remove redundant PubSubTopic from Observer
@danisharora099
Copy link
Collaborator Author

I think relay could have been done with a 2nd PR to keep reviewing easy.

looking back in hindsight, would definitely make things simpler! 😅

Copy link
Collaborator

@fryorcraken fryorcraken left a comment

Choose a reason for hiding this comment

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

Nice.

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.

bug: pubSubTopic not respected on createLightNode
3 participants