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.

Sign up for GitHub

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

Jump to bottom

feat: replace waitForRemotePeers() with waku.waitForPeer() method #2161

Merged
merged 30 commits into from
Oct 8, 2024
Merged

feat: replace waitForRemotePeers() with waku.waitForPeer() method #2161

merged 30 commits into from
Oct 8, 2024

Conversation

weboko
Copy link
Collaborator

@weboko weboko commented Oct 4, 2024 •
edited
Loading

Problem

We have a common pattern when users absolutely need to:

import { createLigthNode, waitForRemotePeers } from "@waku/sdk";

const waku = createLigthNode();

await waku.start();
await waitForRemotePeers(waku);

This PR makes IWaku self sufficient:

import { createLigthNode } from "@waku/sdk";

const waku = createLigthNode();

await waku.start();
await waku.waitForPeer();

Other stuff

  • Removed export of waitForRemotePeers;
  • Renamed Waku interface to IWaku;
  • Added nice docs:
    image

Notes

@weboko
fix comment of default number of peers
4dbb1b6
@weboko
export default number of peers from base protocol sdk
315d82f
@weboko
rename to light_push, move class to separate file
23ded77
@weboko
move waitForRemotePeer to sdk package
240b1b6
@weboko
add todo to move waitForGossipSubPeerInMesh into @waku/relay
ad56eae
@weboko
clean up waitForRemotePeer, split metadata await from event and optim…
7580eeb 
…ise, decouple from protocol implementations
@weboko
simplify and rename ILightPush interface
800eae2
@weboko
use only connected peers in light push based on connections instead o…
6bcf283 
…f peer renewal mechanism
@weboko
improve readability of result processing in light push
9742d68
@weboko
fix check & update tests
0f09905
@weboko
address tests, add new test cases, fix racing condition in StreamManager
3dda8ad
@weboko
use libp2p.getPeers
06498f8
@weboko
feat: confirm metadata and protocols needed in waitForRemotePeer
eaee635
@weboko
merge with master
3928603
@weboko
rely on passed protocols and fallback to mounted
21056fd
@weboko
add I prefix to Waku interface
ac212c1
@weboko
implement waku.connect method
4b94686
@weboko
add docs to IWaku interface
690e630
@weboko weboko requested a review from a team as a code owner October 4, 2024 14:31
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 4, 2024 •
edited
Loading

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 84.23 KB (+0.73% 🔺) 1.7 s (+0.73% 🔺) 4 s (-1.66% 🔽) 5.7 s
Waku Simple Light Node 135 KB (-0.34% 🔽) 2.8 s (-0.34% 🔽) 5.1 s (+53.43% 🔺) 7.8 s
ECIES encryption 22.94 KB (0%) 459 ms (0%) 1.3 s (+51.41% 🔺) 1.8 s
Symmetric encryption 22.39 KB (0%) 448 ms (0%) 1.3 s (+13.09% 🔺) 1.7 s
DNS discovery 72.28 KB (0%) 1.5 s (0%) 2.8 s (-29.18% 🔽) 4.2 s
Peer Exchange discovery 73.88 KB (0%) 1.5 s (0%) 3.6 s (+31.76% 🔺) 5.1 s
Local Peer Cache Discovery 67.63 KB (0%) 1.4 s (0%) 3.3 s (+21.97% 🔺) 4.7 s
Privacy preserving protocols 75.61 KB (+1.06% 🔺) 1.6 s (+1.06% 🔺) 3.3 s (-28.36% 🔽) 4.8 s
Waku Filter 78.58 KB (-0.12% 🔽) 1.6 s (-0.12% 🔽) 4.2 s (+16.59% 🔺) 5.8 s
Waku LightPush 76.83 KB (-0.08% 🔽) 1.6 s (-0.08% 🔽) 3.8 s (+17.9% 🔺) 5.4 s
History retrieval protocols 76.09 KB (+0.17% 🔺) 1.6 s (+0.17% 🔺) 2.8 s (-27.62% 🔽) 4.3 s
Deterministic Message Hashing 7.38 KB (0%) 148 ms (0%) 501 ms (-30.84% 🔽) 649 ms

weboko
weboko commented Oct 4, 2024
View reviewed changes
packages/interfaces/src/waku.ts Outdated
* waku.isConnected() === true;
* ```
*/
connect(protocols?: Protocols[], timeoutMs?: number): Promise<void>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@waku-org/js-waku looking for your feedback on naming

named it connect as when it is used it looks like await waku.connect(); which seems convenient

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agreed to use .waitForPeer instead

@danisharora099 ping for your input

Copy link
Collaborator

Choose a reason for hiding this comment

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

waitForConnectivity perhaps would be more verbose. Indifferent from now, we can improve this with external feedback

@weboko weboko changed the title feat!: replace waitForRemotePeers() with waku.connect() method feat!: replace waitForRemotePeers() with waku.waitForPeer() method Oct 4, 2024
Base automatically changed from weboko/improve-wait-for-remote to master October 4, 2024 23:49
@weboko weboko changed the title feat!: replace waitForRemotePeers() with waku.waitForPeer() method feat: replace waitForRemotePeers() with waku.waitForPeer() method Oct 5, 2024
danisharora099
danisharora099 reviewed Oct 8, 2024
View reviewed changes
Copy link
Collaborator

@danisharora099 danisharora099 left a comment

Choose a reason for hiding this comment

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

commented nits, lgtm

packages/interfaces/src/relay.ts Outdated Show resolved Hide resolved
packages/interfaces/src/waku.ts Show resolved Hide resolved
weboko and others added 4 commits October 8, 2024 13:13
@weboko
feat: add mocha tests to @waku/sdk and cover waitForRemotePeer (#2163)
8937a56 
* feat: add mocha tests to @waku/sdk and cover waitForRemotePeer

* add waitForRemote UTs

* remove junk
@weboko
feat: expose peerId and protocols from WakuNode (#2166)
027a1d1 
* chore: expose peerId and protocols from WakuNode

* remove unused method

* move to private method
@weboko
rename to waitForPeers
448e659
@weboko
up test
ec8ccb4
@weboko weboko merged commit 75fcca4 into master Oct 8, 2024
10 of 11 checks passed
@weboko weboko deleted the weboko/implement-connect branch October 8, 2024 22:43
This was referenced Oct 8, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danisharora099 danisharora099 danisharora099 approved these changes

@adklempner adklempner adklempner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@weboko @adklempner @danisharora099