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!: @waku/discovery #1876

Merged
merged 13 commits into from
Mar 12, 2024
Merged

feat!: @waku/discovery #1876

merged 13 commits into from
Mar 12, 2024

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Feb 29, 2024

Problem

We currently have 3 discovery options available within Waku:

For this, we have 3 separate packages, which might only increase with time.

Solution

Create a generalised package @waku/discovery to encapsulate all the available discovery mechanisms.

Notes

@danisharora099 danisharora099 force-pushed the feat/waku-discovery branch 7 times, most recently from e8f43b4 to a02e6d9 Compare March 4, 2024 19:07
Copy link

github-actions bot commented Mar 4, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 185.23 KB (0%) 3.8 s (0%) 25.8 s (+1.53% 🔺) 29.5 s
Waku Simple Light Node 185.28 KB (0%) 3.8 s (0%) 14 s (-0.72% 🔽) 17.7 s
ECIES encryption 22.89 KB (0%) 458 ms (0%) 7.5 s (+41.31% 🔺) 8 s
Symmetric encryption 22.34 KB (0%) 447 ms (0%) 2.5 s (-30.18% 🔽) 2.9 s
DNS discovery 73.78 KB (+5.48% 🔺) 1.5 s (+5.48% 🔺) 17.6 s (+74.18% 🔺) 19 s
Privacy preserving protocols 39.96 KB (0%) 800 ms (0%) 11.3 s (-8.52% 🔽) 12.1 s
Waku Filter 20.11 KB (0%) 403 ms (0%) 5.8 s (+14.14% 🔺) 6.2 s
Waku LightPush 115.49 KB (0%) 2.4 s (0%) 25.4 s (+51.84% 🔺) 27.7 s
History retrieval protocols 19.34 KB (0%) 387 ms (0%) 3.5 s (-8.08% 🔽) 3.9 s
Deterministic Message Hashing 4.96 KB (0%) 100 ms (0%) 331 ms (-62.83% 🔽) 431 ms
Peer Exchange discovery 75.27 KB (+100% 🔺) 1.6 s (+100% 🔺) 21.5 s (+100% 🔺) 23 s
Local Peer Cache Discovery 69.07 KB (+100% 🔺) 1.4 s (+100% 🔺) 23.2 s (+100% 🔺) 24.6 s

@danisharora099 danisharora099 marked this pull request as ready for review March 4, 2024 19:15
@danisharora099 danisharora099 requested a review from a team as a code owner March 4, 2024 19:15

// Peer Exchange Discovery
export {
wakuPeerExchange,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if we need these waku*. To me it seems we can avoid and just be straight - peerExchangeDiscovery, peerCacheDiscovery, etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed two exports, but we use reft of them somewhere or the other (mostly for different types of tests)

Please feel free to take a look and suggest if there's an unnecessary export remaining

// dynamically importing the local storage polyfill for node
if (typeof window === "undefined") {
try {
const { LocalStorage } = await import("node-localstorage");
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's only needed for tests so another check can be used too, for example

if (!options?.hideWebSocketInfo && process.env.NODE_ENV !== "test") {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

typeof window check feels more robust for karma compilation purposes: karma can't compile node packges, thus this acts as a good safe guard.
Is this a nit? can also try with the NODE_ENV flag

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.

left some comments

@danisharora099
Copy link
Collaborator Author

left some comments

Thanks for the comments! Addressed :)

@danisharora099 danisharora099 requested a review from weboko March 11, 2024 10:55
@@ -6,5 +6,5 @@
"tsBuildInfoFile": "dist/.tsbuildinfo"
},
"include": ["src"],
"exclude": ["src/**/*.spec.ts", "src/test_utils"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

src/test_utils isn't actually any directory in this package. seems to have come in during a copy. spec.ts files are still excluded

@danisharora099 danisharora099 merged commit 1e86c3d into master Mar 12, 2024
9 of 10 checks passed
@danisharora099 danisharora099 deleted the feat/waku-discovery branch March 12, 2024 10:26
@weboko weboko mentioned this pull request Mar 12, 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.

feat: create a new package @waku/discovery to encapsulate all discovery mechanisms available within Waku
3 participants