-
Notifications
You must be signed in to change notification settings - Fork 41
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
chore(lightpush)!: move protocol implementation opinions to @waku/sdk
#1887
Conversation
@waku/sdk
@waku/sdk
@waku/sdk
@waku/sdk
cd79658
to
8dae746
Compare
size-limit report 📦
|
/** | ||
* 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these rename? I think we can keep previous names, no need to add core
as it is already clear from the package name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -340,6 +340,8 @@ class Subscription { | |||
} | |||
} | |||
|
|||
const DEFAULT_NUM_PEERS = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it is not a part of the PR but how is it enforced? It seems we do have this as default
const DEFAULT_NODE_REQUIREMENTS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 discoveryDEFAULT_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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then it seems not to be used and fall back to DEFAULT_NODE_REQUIREMENTS
, am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -357,6 +359,9 @@ class Filter extends BaseProtocol implements IReceiver { | |||
return subscription; | |||
} | |||
|
|||
//TODO: Remove when FilterCore and FilterSDK are introduced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
FitlerRFC
andFilter
orFilterCore
andFilter
?
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not add any subtasks for now - as we don't know what we are optimizing for
c66ad58
to
299b5d9
Compare
Problem
#1886
Solution
Moves away the abstraction of using multiple peers for lightpush by:
LightPushSDK
(and renaming protocol implementation toLightPushCore
)LightPushCore
remains accessible from withinLightPushSDK
(it is an abstraction)BaseProtocolSDK
that houses information such as number of peers for the protocolSDK
-type protocol classes (asBaseProtocol
is common between all protocol implementation classes)Notes
SendResult
types by renamingrecipients
tosuccesses
, and introducingfailures
of the typeerror: SendError, peerId
LightPushSDK
usingPromise.allSettled()
@waku/core
should be as unopinionated as possible #1886