chore(lightpush)!: move protocol implementation opinions to `@waku/sdk` #1887

danisharora099 · 2024-03-05T12:30:59Z

Problem

#1886

Solution

Moves away the abstraction of using multiple peers for lightpush by:

creating a new class LightPushSDK (and renaming protocol implementation to LightPushCore)
LightPushCore remains accessible from within LightPushSDK (it is an abstraction)
similarly introduces BaseProtocolSDK that houses information such as number of peers for the protocol
- common between all SDK-type protocol classes (as BaseProtocol is common between all protocol implementation classes)

Notes

also changes SendResult types by renaming recipients to successes, and introducing failures of the type error: SendError, peerId
- wanted to do this part of a followup PR initially, but became essential part of the same PR to implement LightPushSDK using Promise.allSettled()
Related to chore: protocol implementations in @waku/core should be as unopinionated as possible #1886

github-actions · 2024-03-05T13:09:25Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
Waku node	185.23 KB (-0.01% 🔽)	3.8 s (-0.01% 🔽)	1.1 s (+37.38% 🔺)	4.8 s
Waku Simple Light Node	185.28 KB (+0.07% 🔺)	3.8 s (+0.07% 🔺)	1.1 s (-26.32% 🔽)	4.8 s
ECIES encryption	22.89 KB (0%)	458 ms (0%)	480 ms (+91.22% 🔺)	938 ms
Symmetric encryption	22.34 KB (0%)	447 ms (0%)	173 ms (-59.75% 🔽)	619 ms
DNS discovery	69.95 KB (0%)	1.4 s (0%)	716 ms (-0.35% 🔽)	2.2 s
Privacy preserving protocols	39.96 KB (+0.29% 🔺)	800 ms (+0.29% 🔺)	612 ms (+62.16% 🔺)	1.5 s
Light protocols	0 B (-100% 🔽)	0 ms (-100% 🔽)	0 ms (-100% 🔽)	0 ms
History retrieval protocols	19.34 KB (-0.04% 🔽)	387 ms (-0.04% 🔽)	353 ms (+46.42% 🔺)	740 ms
Deterministic Message Hashing	4.96 KB (0%)	100 ms (0%)	31 ms (-48.69% 🔽)	130 ms
Waku Filter	20.11 KB (+100% 🔺)	403 ms (+100% 🔺)	298 ms (+100% 🔺)	700 ms
Waku LightPush	115.49 KB (+100% 🔺)	2.4 s (+100% 🔺)	937 ms (+100% 🔺)	3.3 s

packages/core/src/lib/light_push/index.ts

weboko · 2024-03-07T22:12:05Z

packages/core/src/lib/base_protocol.ts

 /**
 * A class with predefined helpers, to be used as a base to implement Waku
 * Protocols.
 */
-export class BaseProtocol implements IBaseProtocol {
+export class BaseProtocol implements IBaseProtocolCore {


Why these rename? I think we can keep previous names, no need to add core as it is already clear from the package name.

I refactor these names as we now have two types of BaseProtocol:

one on the protocol implementation level (in @waku/core)

other on the SDK level (in @waku/sdk)

I personally incline towards having more verbose & readable names.

packages/core/src/lib/wait_for_remote_peer.ts

weboko · 2024-03-07T22:51:58Z

packages/core/src/lib/filter/index.ts

@@ -340,6 +340,8 @@ class Subscription {
  }
 }

+const DEFAULT_NUM_PEERS = 3;


I know it is not a part of the PR but how is it enforced? It seems we do have this as default

js-waku/packages/sdk/src/utils/discovery.ts

Line 7 in fb41f4c

const DEFAULT_NODE_REQUIREMENTS = {

These variables are for two separate uses:

DEFAULT_NODE_REQUIREMENTS: the default configuration of number of peers for each protocol that we want to discovery using DNS discovery

DEFAULT_NUM_PEERS: the number of peers to use for the protocol request (redundancy to increase decentralization)

It is used here:

js-waku/packages/interfaces/src/protocols.ts

Lines 72 to 79 in fb41f4c

/**

* Number of peers to connect to, for the usage of the protocol.

* This is used by:

* - Light Push to send messages,

* - Filter to retrieve messages.

* Defaults to 3.

*/

numPeersToUse?: number;

but then it seems not to be used and fall back to DEFAULT_NODE_REQUIREMENTS, am I missing something?

I'm not sure what you mean: the above two constants are for two very different purposes as described.

numPeersToUse falls back to DEFAULT_NUM_PEERS if not provided part of the options argument

weboko · 2024-03-07T22:53:24Z

packages/core/src/lib/filter/index.ts

@@ -357,6 +359,9 @@ class Filter extends BaseProtocol implements IReceiver {
    return subscription;
  }

+  //TODO: Remove when FilterCore and FilterSDK are introduced


Maybe FitlerRFC and Filter or FilterCore and Filter? I think one can be left clean or renamed entirely as it is something else than Filter in the meaning of protocol.

I'd like to avoid this kind of names in general as it is kind of duplication when we have FilterX and FilterY or SendResult and CoreSendResult which just brings difficulty to understand what to use under which conditions.

Ideally we don't want anyone use core protocols except when people know what they are doing. In that case for end user I'd like to keep out API the same: LightPush/Filter or create new entities like Sender/Receiver. That way proposition of implementation is clear.

wdyt?

Maybe FitlerRFC and Filter or FilterCore and Filter?

I personally incline towards a more verbose naming convention, but I'm happy to change this to naming like Filter and FilterCore if you prefer

I think one can be left clean or renamed entirely as it is something else than Filter in the meaning of protocol.

Can you elaborate? As I see it, they are both functionalities for Filter: one provides us with an implementation to interact with the protocol (core), and the other is an implementation that offers opinions by us on how you can (better) use it (sdk)

I personally incline towards a more verbose naming convention

I think move simpler names make it easier to comprehend and comply with RFC and other implementations.
For example - FilterRFC is visible that follows RFC and Filter being exposed at the root level of node makes it go-to choice with it's API being documented.

I think one can be left clean or renamed entirely as it is something else than Filter in the meaning of protocol.

here I mean that if we have FilterRFC being implementation of RFC - then Filter with it's proxy capabilities might as well be named something else, as it is not Filter in sense of Waku terminology but convenient API we provide

RFC stands for Request For Comments - I'm not sure if the class called FilterRFC gives the right messaging.
We could, however, keep FilterCore as Filter and FilterSDK could remain as is.

Since this was merged, @weboko do we need to followup here? Can be added as a separate task that can be tackled in #1886

let's not add any subtasks for now - as we don't know what we are optimizing for

danisharora099 added 6 commits March 5, 2024 14:04

chore: restructure @waku/sdk

c8dba8e

chore: introduce BaseProtocolCore and BaseProtocolSDK

ae05525

chore: introduce LightPushCore and LightPushSDK

9164878

chore: update relay for new types

ab68514

chore(sdk): update structure

e9868b8

chore(filter): add numPeersToUse

cd62f51

danisharora099 changed the title ~~chore!(lightpush): move protocol opinions to @waku/sdk~~ chore(lightpush)!: move protocol opinions to @waku/sdk Mar 5, 2024

danisharora099 changed the title ~~chore(lightpush)!: move protocol opinions to @waku/sdk~~ chore(lightpush)!: move protocol implementation opinions to @waku/sdk Mar 5, 2024

danisharora099 added 2 commits March 5, 2024 18:32

chore: update tests

e5081d8

update: size-limit

8dae746

danisharora099 force-pushed the chore/refactor-protocol-structure branch from cd79658 to 8dae746 Compare March 5, 2024 13:05

chore: update more tests

821d734

danisharora099 marked this pull request as ready for review March 5, 2024 14:42

danisharora099 requested a review from a team as a code owner March 5, 2024 14:42

danisharora099 mentioned this pull request Mar 5, 2024

chore: protocol implementations in @waku/core should be as unopinionated as possible #1886

Closed

3 tasks

adklempner reviewed Mar 6, 2024

View reviewed changes

packages/core/src/lib/light_push/index.ts Show resolved Hide resolved

danisharora099 requested a review from a team March 7, 2024 10:52

chair28980 mentioned this pull request Mar 7, 2024

[Epic: js-waku] Reliability Protocol for Resource-Restricted Clients #2154

Closed

39 tasks

adklempner approved these changes Mar 7, 2024

View reviewed changes

weboko reviewed Mar 7, 2024

View reviewed changes

packages/core/src/lib/wait_for_remote_peer.ts Outdated Show resolved Hide resolved

weboko reviewed Mar 7, 2024

View reviewed changes

attach issue link to TODOs

299b5d9

danisharora099 force-pushed the chore/refactor-protocol-structure branch from c66ad58 to 299b5d9 Compare March 8, 2024 11:08

Merge branch 'master' into chore/refactor-protocol-structure

4aefe19

danisharora099 merged commit 8deab11 into master Mar 11, 2024
9 of 10 checks passed

danisharora099 deleted the chore/refactor-protocol-structure branch March 11, 2024 13:20

weboko mentioned this pull request Mar 11, 2024

chore: release master #1901

Closed

This was referenced Apr 7, 2024

chore: release master #1938

Closed

chore: release master #1940

Closed

chore: release master #1942

Closed

chore: release master #1945

Closed

chore: release master #1950

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(lightpush)!: move protocol implementation opinions to `@waku/sdk` #1887

chore(lightpush)!: move protocol implementation opinions to `@waku/sdk` #1887

danisharora099 commented Mar 5, 2024 •

edited

Loading

github-actions bot commented Mar 5, 2024 •

edited

Loading

weboko Mar 7, 2024

danisharora099 Mar 8, 2024

weboko Mar 7, 2024

danisharora099 Mar 8, 2024 •

edited

Loading

weboko Mar 12, 2024

danisharora099 Mar 14, 2024

weboko Mar 7, 2024

weboko Mar 7, 2024

danisharora099 Mar 11, 2024 •

edited

Loading

weboko Mar 12, 2024

danisharora099 Mar 14, 2024

danisharora099 Mar 18, 2024

weboko Apr 7, 2024

	/**
	* Number of peers to connect to, for the usage of the protocol.
	* This is used by:
	* - Light Push to send messages,
	* - Filter to retrieve messages.
	* Defaults to 3.
	*/
	numPeersToUse?: number;

chore(lightpush)!: move protocol implementation opinions to @waku/sdk #1887

chore(lightpush)!: move protocol implementation opinions to @waku/sdk #1887

Conversation

danisharora099 commented Mar 5, 2024 • edited Loading

Problem

Solution

Notes

github-actions bot commented Mar 5, 2024 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danisharora099 Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danisharora099 Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore(lightpush)!: move protocol implementation opinions to `@waku/sdk` #1887

chore(lightpush)!: move protocol implementation opinions to `@waku/sdk` #1887

danisharora099 commented Mar 5, 2024 •

edited

Loading

github-actions bot commented Mar 5, 2024 •

edited

Loading

danisharora099 Mar 8, 2024 •

edited

Loading

danisharora099 Mar 11, 2024 •

edited

Loading