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

fix: subscription RPC & added test subscriptions #1587

Merged
merged 6 commits into from
Sep 25, 2023

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Sep 20, 2023

I totally messed up my previous PR #1572

This PR should fix the problem for real this time! Sorry about that!

@SionoiS SionoiS requested a review from a team as a code owner September 20, 2023 16:01
@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.69 KB (0%) 554 ms (0%) 2.1 s (+64.03% 🔺) 2.7 s
Waku Simple Light Node 262.6 KB (0%) 5.3 s (0%) 3.6 s (+8.58% 🔺) 8.8 s
ECIES encryption 28.7 KB (0%) 574 ms (0%) 1.9 s (+49.11% 🔺) 2.5 s
Symmetric encryption 28.7 KB (0%) 575 ms (0%) 1.2 s (-27.49% 🔽) 1.8 s
DNS discovery 110.62 KB (0%) 2.3 s (0%) 2.8 s (+27.04% 🔺) 5 s
Privacy preserving protocols 117.38 KB (0%) 2.4 s (0%) 2.3 s (+1.06% 🔺) 4.6 s
Light protocols 25.83 KB (0%) 517 ms (0%) 1.3 s (-1.5% 🔽) 1.9 s
History retrieval protocols 24.9 KB (0%) 499 ms (0%) 1.4 s (-13.84% 🔽) 1.9 s
Deterministic Message Hashing 5.65 KB (0%) 113 ms (0%) 361 ms (-29.91% 🔽) 474 ms

Copy link
Collaborator

@weboko weboko left a comment

Choose a reason for hiding this comment

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

so, essentially nothing broke but it wouldn't work as intended?

@@ -25,6 +25,8 @@ describe("Waku Light Push [node only] - custom pubsub topic", function () {
nwaku,
customPubSubTopic
);

await nwaku.ensureSubscriptions([customPubSubTopic]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have other tests using a custom pubsub topic, no?

Copy link
Contributor Author

@SionoiS SionoiS Sep 21, 2023

Choose a reason for hiding this comment

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

Yes but subscriptions is required only if you want to check the nwaku message cache.

@SionoiS
Copy link
Contributor Author

SionoiS commented Sep 21, 2023

so, essentially nothing broke but it wouldn't work as intended?

Yes exactly

@weboko weboko merged commit 6fa106a into master Sep 25, 2023
9 of 11 checks passed
@weboko weboko deleted the fix--test-subscriptions branch September 25, 2023 22:24
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.

3 participants