-
Notifications
You must be signed in to change notification settings - Fork 3
Add "Messaging API" spec #19
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
base: master
Are you sure you want to change the base?
Conversation
7d3248b
to
fe20d79
Compare
Thumb up if you want me to walk you through the document on a short call. :) |
|
||
```typescript | ||
{ | ||
mode: "edge" | "service"; |
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.
General question to anyone from me:
- I don't believe we actually need this property AS
Messaging API
can be smart enough to determine and prioritize underlying protocols.
I am in favor of removing it altogether giving a seamless way for mounting Messaging API
on any node.
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.
Automatic resolution of mode would be the best, but at the same time, I would kinda expect to have 3 modes - auto
(default), edge
, service
- ideally I never have to switch manually, but then I also know my shitty internet connection at home and being able to explicitly switch (as long as an app exposes that option, obviously) to edge
mode would make sense to me.
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.
On a first RAW spec I would not include an auto
mode. We can plan and add this later. Probably around the time we build a full stack JS.
I'd suggest
- edge
- relay: relay + service for filter, light push and peer exchange - like "relay mode" in status desktop
- service (or store): relay + provide service for store.
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.
Then I keep edge
and introduce relay
.
As for the service
, correct me if I am wrong, but I think it is out of the scope of Messaging API
.
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.
Are we suggesting that for now, if one wants to run a store node or full blow service node, they would use a binary such a wakunode2
and not the messaging API?
Sounds fair (for now).
and [FILTER](https://github.com/vacp2p/rfc-index/blob/7b443c1aab627894e3f22f5adfbb93f4c4eac4f6/waku/standards/core/12/filter.md) for receiving messages. | ||
|
||
If `service` selected the node MUST implement [RELAY](https://github.com/vacp2p/rfc-index/blob/0277fd0c4dbd907dfb2f0c28b6cde94a335e1fae/waku/standards/core/11/relay.md), | ||
[STORE](../standards/core/store.md) as well as host endpoint for [LIGHTPUSH](../standards/core/lightpush.md) and [FILTER](https://github.com/vacp2p/rfc-index/blob/7b443c1aab627894e3f22f5adfbb93f4c4eac4f6/waku/standards/core/12/filter.md). |
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.
Requiring store does not feel right - Store nodes have different expectations than Lightpush and Filter nodes - those can come and go and still be useful (unless the fluctuation is in order of seconds or minutes), but Store (even with Store sync) should be more stable and available + enabling the protocol puts requirement on hosting the DB.
I'd personally see store managed separately
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.
To add to it, I might be fine if my phone is a service
node when on Wifi and plugged in the charger, but I would not want it to store GBs of random and "useless" data:)
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 agree here. see my response in https://github.com/waku-org/specs/pull/19/files#r1998346439. Modes should be edge
, relay
, store
. with relay
providing services for light push, filter and px
interface ISend { | ||
public send(encoder: Encoder, message: Message): RequestID; | ||
public cancelRequest(requestId: RequestID): void; | ||
public getPendingRequests(): RequestID[]; |
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.
Should there also be isPending(requestId: RequestID)
method, or is iterating over getPendingRequests()
result enough?
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 think I prefer this idea of getting info from a particular RequestID
Maybe, another option could be:
public getRequestState(requestId: RequestID): ReqState
Where ReqState
can be:
- Delivered
- Pending
- NotDelivered
- Cancelled
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.
Ok very interesting here.
Due to the nature of Waku's domain (async network request), it seems much more appropriate to have an event based approach.
having getRequestState
implies someone is going to loop on that - not a great design.
I'd remove in favor of event listening/callbacks
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 agree with @fryorcraken here. It would be better to have an event based approach to indicate success/failure/timeout etc.
##### `send` | ||
An `Encoder` and a `Message` MUST be provided as parameters. | ||
When invoked, the method schedules the message to be sent via [LIGHTPUSH](../standards/core/lightpush.md) or [RELAY](https://github.com/vacp2p/rfc-index/blob/0277fd0c4dbd907dfb2f0c28b6cde94a335e1fae/waku/standards/core/11/relay.md) based on the node’s configuration. | ||
The method MUST throw an error if the `pubsubTopic` provided by the `Encoder` is not supported by the underlying node. |
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.
Can we avoid using pubsubTopic
and say shard
? Since that is what is configured on node's creation?
type SubscriptionListener = (message: MessageRecord) => void | Promise<void>; | ||
|
||
interface ISubscribe { | ||
public subscribe(decoder: Decoder, fn: SubscriptionListener): SubscriptionID; |
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.
One thing that I was never really sure about is why do we splic Decoder
and SubsciptionListener
- given that Decoder has all the info about what to listen for (and how to prep the message e.g. with decryption) and then SubsciptionListener has info about what to do with the message - why not to make SubscriptionListener a property of decoder? And then Subscribe would just receive the decoder. I am curious what am I mising:D
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 would remove decoder.
|
||
##### `subscribe` | ||
Registers a new subscription to listen for messages processed by a given `Decoder`. | ||
The subscription is based on the `pubsubTopic` and `contentTopic` provided by the `Decoder`, |
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.
As I mentioned above - would it make sense to move away from pubsubTopic
in dev/user facing parts and only refer to shard
?
The method MUST throw an error if the `pubsubTopic` provided by the `Decoder` is not supported by the underlying node. | ||
No specific order is guaranteed for callback invocation; | ||
subscription callbacks MUST be executed as messages are received from the protocol in use. | ||
If `confirmStored` is set to `true`, a recurring [STORE](../standards/core/store.md) query MUST be started. |
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 understood confirmStored
as targetting sent messages (i.e. to confirm a message I published reached the network).
What if I don't necessarily need confirmStore
for sending (e..g I am just publishing an "I am online" heartbeat, so I don't really care if they are missed sometime), but I do care for regular store queries as I'd really like to make sure I get all the messages?
Basically, are we conflating 2 separate features under confirmStored
?
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 agree and think that we should have 2 configs as it is currently implemented in status. We have separate flag to indicate sendConfirmations
vs fetchMissingMessages
Content-Type: application/json | ||
|
||
{ | ||
"error": "Cannot create subscription. clusterId 0 is not supported." |
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 is no clusterId
in /subscribe
- this message would utterly confuse me as a developer:) Probably related to my question(s) about replacing pubsubTopic
with shard
(and then also adding clusterid
to /subscribe
for completeness)
|
||
Response: | ||
```http | ||
HTTP/1.1 200 OK |
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.
Should there also be 404
or if the subscription does not exist, or would it return 200
since it is ok
, kinda
##### By `contentTopic` | ||
Request: | ||
```http | ||
GET /messages?contentTopic=/messaging/2/api/utf8&skip=0&take=10 HTTP/1.1 |
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.
Should this be URL encoded?
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.
Thanks so much for it! 🙌
Some comments so far
confirmStored?: boolean; | ||
confirmReceived?: boolean; | ||
confirmContentTopics?: string[]; |
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 would start without those and always ensure any message is delivered by default, i.e., for now I wouldn't add additional constraints until we get the implementations more mature
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.
It might be better to have this config e.g in case of status there might be many content-topics which are used for broadcasting repetetive status's and other info even if missed once would get updated in the next broadcast interval. Such topics might not require store confirmations.
From an implementation pov this doesn't seem complicated.
If `edge` selected the node MUST use [LIGHTPUSH](../standards/core/lightpush.md) for sending messages, | ||
and [FILTER](https://github.com/vacp2p/rfc-index/blob/7b443c1aab627894e3f22f5adfbb93f4c4eac4f6/waku/standards/core/12/filter.md) for receiving messages. |
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 think is interesting to mention store-client
, filter-client
, and lightpush-client
. The "no-client" component is used by service
nodes.
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.
@Ivansete-status I am not sure I understand this comment, can you elaborate, please?
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.
@Ivansete-status I am not sure I understand this comment, can you elaborate, please?
Sure! The edge
nodes must act as both lightpush-client
and filter-client
. In both cases, the edge
node is not directly mounting LIGHTPUSH
nor FILTER
protocols. Instead, the edge node just dial peers that mount those protocols. Therefore, I think is more precise to mention that.
Another option would be something like:
MUST use [LIGHTPUSH](../standards/core/lightpush.md) as a client and connect to peers that mount LIGHTPUSH
and
[FILTER](https://github.com/vacp2p/rfc-index/blob/7b443c1aab627894e3f22f5adfbb93f4c4eac4f6/waku/standards/core/12/filter.md) as a client and connect to peers that mount FILTER
}; | ||
``` | ||
|
||
##### `Encoder` |
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 wouldn't define that Encoder
type
type SubscriptionListener = (message: MessageRecord) => void | Promise<void>; | ||
|
||
interface ISubscribe { | ||
public subscribe(decoder: Decoder, fn: SubscriptionListener): SubscriptionID; |
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.
As suggested in the send
section , I think we need to bring more feedback to the caller.
This ID MUST be used to manage active subscriptions, | ||
allowing them to be terminated when they are no longer needed. | ||
|
||
##### `SubscriptionListener` |
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 think the "subscription" term doesn't reflect completely what it does
##### `SubscriptionListener` | |
##### `OnMsgReceived` |
Each identifier corresponds to a subscription that is in effect, | ||
and no duplicate values MUST be present in the returned array. | ||
|
||
##### `Decoder` |
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 think this is not needed.
The onMsgReceived(contentTopic: string, payload: array[byte])
, or similar , should suffice.
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.
This depends on how smart the Messaging API should be - e.g. in Waku Dispatcher my "onMsgReceived` function signature looks like this: https://github.com/vpavlin/waku-dispatcher/blob/main/src/dispatcher.ts#L135 - I kind of like the flexibility of using Encoder/Decoder gives us and potentially gives all the devs (I could implement my own Decoder which would do some smart validation or processing)
Another thing is you cannot just pass payload and contentTopic - the dev also needs the rest of the fields - like meta
, timestamp
or rln proof
:)
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 kind of like the flexibility of using Encoder/Decoder gives us and potentially gives all the devs
ah okay I see. I have the impression that the Encoder/Decoder concept fits very well from typescript PoV but I wonder how it would fit from a C API PoV
Another thing is you cannot just pass payload and contentTopic - the dev also needs the rest of the fields - like meta, timestamp or rln proof:)
Yes good point! I agree that meta
and timestamp
is needed but maybe rln proof
not needed
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.
Another thing is you cannot just pass payload and contentTopic - the dev also needs the rest of the fields - like
meta
,timestamp
orrln proof
:)
Why not pass WakuMessage itself which has all these fields?
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 would move away from the Decoder in the context of the spec.
But js-waku is welcome to have an abstraction specific to JS/TS over the messaging API is decoder/encoder is a better dev ex.
] | ||
``` | ||
|
||
### Message Storage |
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 think the users shouldn't be aware to any Storage. We need to handle the store queries internally and the users shouldn't worry about any kind of history API. We already have the "expert" API for that purpose :)
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 think we need some Message Storage API:-) The naming might need to be tweaked, but as a dev, I want to be able to sift through the messages I have sent and received, to filter them, or verify things - and it would be bad if everyone had to implement this on their own - Messaging API is a great place for it:) (speaking again based on what I needed in Waku Dispatcher - https://github.com/vpavlin/waku-dispatcher/blob/main/src/dispatcher.ts#L482)
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.
How about we have this API only to do time-range based retrieval queries where a node has come back from offline state and wants history.
The hash based queries can be done behind the scenes as part of the Messaging API for reliability checks.
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 think we are talking about local storage. Hence, i would rename to Local Message Storage API
.
I would NOT include this at this point in time, and would instead aim for the scope to be narrower and spec leaner.
However, I believe that the opposite is missing. A Waku implementation needs access to a local DB or cache to function properly, and remember message ids. etc.
This should be defined here. See how go-waku interfaces with status-go DB to mark messages as sent etc.
So when you start a node, there is a couple of components you should pass to your node, and we need to define the APIs those component must implement.
Another one is a time source
, and DNS source
(or maybe just DNS Servers ips or DoH addresses).
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 have reviewed half. I think it's enough comment for now.
Let me know if you prefer a full review.
Hopefully, lot of opportunity to thin it out.
}; | ||
|
||
interface ISend { | ||
public send(encoder: Encoder, message: Message): RequestID; |
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.
Not convinced encoder concept works here. Let's see how it looks on C API.
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 do we need to have an encoder? Shouldn't encoder be more like a utility from waku/sdk app devs can use to encode their payload and then supply WakuMessage to be sent? This gives flexibility for devs to decide whether to use Waku encoder or some other way to encode their messages.
This looks like better separate of concerns in case apps want to use their own way to encode different message types e.g: in status we have private messages which are encrypted/encoded differently and public messages which are done differently. Ultimately status backend handsover only WakuMessage to the send API which seems more cleaner to me.
|
||
interface ISend { | ||
public send(encoder: Encoder, message: Message): RequestID; | ||
public cancelRequest(requestId: RequestID): void; |
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.
Do we need/use that? sounds too low level
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.
Agreed it is too low level, rather a more simpler approach would be for the user to indicate a timeout for the send API. The user can also pass a context(this might be too Golang specific but is a very good concept for an API in general) which can be cancelled or timeout.
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 thought of having it as a backup for consumers to have granular control
but after all discussions with you I think maybe some start
and stop
methods will be enought
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 do simple first. https://www.notion.so/Waku-Mission-and-Methodology-1a58f96fb65c80b9b789c1bbb9e99915?pvs=4#1d78f96fb65c802c928af2576c55bc20
So yes, start/stop sounds fair
interface ISend { | ||
public send(encoder: Encoder, message: Message): RequestID; | ||
public cancelRequest(requestId: RequestID): void; | ||
public getPendingRequests(): RequestID[]; |
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.
Ok very interesting here.
Due to the nature of Waku's domain (async network request), it seems much more appropriate to have an event based approach.
having getRequestState
implies someone is going to loop on that - not a great design.
I'd remove in favor of event listening/callbacks
##### `Encoder` | ||
The `Encoder` object encapsulates the logic for message handling. | ||
It MUST ensure that messages are encoded into the appropriate format for transmission. | ||
Additionally, it MUST provide information regarding the `pubsubTopic` and `contentTopic` to which messages SHOULD be sent. |
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.
No mention of pubsubtopic
in the API, we only deal with shards and clusters.
#### REST API | ||
|
||
##### `send` | ||
Request: | ||
```http | ||
POST /send HTTP/1.1 | ||
Accept: application/json | ||
Content-Type: application/json | ||
|
||
{ | ||
"timestamp": 0, | ||
"ephemeral": false, | ||
"meta": "ZXhhbXBsZQ==", | ||
"pubsubTopic": "string", | ||
"contentTopic": "string", | ||
"payload": "ZXhhbXBsZQ==" | ||
} | ||
``` | ||
|
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 would not define things twice. Only once in C
and provide a recommendation on how this should translate to a REST API
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.
@weboko thoughts on this?
IMO
define a C API:
char* waku_send(char* message, int shard, int timeoutMs, WakuCallBack onOkCb, WakuCallBack onErrorCb){}
and define a way to deduce the REST endpoint:
- name function because route,
_
becomes/
- data arguments are properties of the JSON payload
- bytes payload are encoded in base64
- callbacks are properties on the response JSON/websocket
Which would be:
``
POST /waku/send
{
message: WakuMessageJSON,
shard: number,
}
etc
Discussion around store abstraction: https://forum.vac.dev/t/no-store-in-messaging-api/455/4 |
I would also reserve a space for RLN. It's not in scope but I overlook to specify it https://www.notion.so/Waku-FURPS-1498f96fb65c803faedef2a591c22c00?d=1c48f96fb65c80369e8f001ca5c7e353&pvs=4#1578f96fb65c80778215ef012c6f6136 |
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.
Reviewed till Send API, will continue filter review little later.
confirmStored?: boolean; | ||
confirmReceived?: boolean; | ||
confirmContentTopics?: string[]; |
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.
It might be better to have this config e.g in case of status there might be many content-topics which are used for broadcasting repetetive status's and other info even if missed once would get updated in the next broadcast interval. Such topics might not require store confirmations.
From an implementation pov this doesn't seem complicated.
|
||
##### `shards` | ||
This property MUST be provided. | ||
An array of shard under a specified cluster that node MUST operate at as per [RELAY-SHARDING](https://github.com/waku-org/specs/blob/186ce335667bdfdb6b2ce69ad7b2a3a3791b1ba6/standards/core/relay-sharding.md). |
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.
Just to confirm that all the shards that are supported within the cluster are to be specified even if a node is only interested in a few of them?
wondering how to address this scenario though or is it something for later stage?
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.
By this description I implied that this is a list of shards that node is interested in, i.e MUST operate at
is present description misleading, @chaitanyaprem ?
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.
for autosharding logic to work properly there must be a config somewhere to be specified how many shards are there in the cluster.
wanted to know which config is this one. if it is only the shards the node is interested in, then we may need to specify another config to indicate total shards in the network.
e.g cluster-id 1 has 8 shards, but node is only interested in shards 0,1 then this config will list [0,1].
But somewhere shardCount 8 also has to be specified so that autosharding logic can use that to determine shard based on contentTopic.
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.
@chaitanyaprem , actually you are right, I missed it
for some reason I thought all auto shard clusters expected to have 8 shards
adding an option for it with default value being 8 shards
let me know if you agree with that - cb446bd
on a side note - it seems numbers of shards
is only needed for deriving shard from content topic
in that sense, shouldn't it be included into content topic format itself?
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.
on a side note - it seems
numbers of shards
is only needed for deriving shard from content topic
in that sense, shouldn't it be included into content topic format itself?
well, the idea was to make users agnostic to shards altogether and only use content-topic. but as each cluster can define its own number of shards, it is requiring to be configured locally. Note that the ideal way to get this info would be through some network configuration made maybe inside RLN contract rather than local configuration. As we don't have network configuration specified anywhere as of now, it is required to be configured locally.
The idea of encoding shard-count in content-topic would create a dependency of content-topic on shard count. In a cluster ideally the number of shards can change over time based on the network's requirements e.g today cluster 1 has 8 shards based on some napkin math of network requirements and bandwidth usage, but this may get increased in future. If we make content-topic format dependent on shard-count this would create problem to all existing content-topics when shard count changes in the network and would require migration etc.
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 me know if you agree with that - cb446bd
maybe the order and naming of config could be something like this as clusterId and shardsInCluster are network level config which are not specific to node being initialized whereas shardsOfInterest
is something specific to the local node.
clusterId:
shardsInCluster:
shardsOfInterest:
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 started to look into this, but I haven;t yet done a PR on nwaku side to propose a solution. I somewhat agree with the above but wanted to summarize a bit more.
1) Autosharding:
The number of shards in cluster is needed shardsInCluster
is a good name. I would not use "under".
2) Static sharding
In this instance, the shards to subscribe to need to be specified when one wants to receive messages from it.
shards
is not qualifying enough, so I'd be more explicit with shardsToSubscribe
Note that those configuration are mutually exclusive and there's a question on whether we want to represent that in the config.
type shardingMode = "auto" | "static"
autoShardingConf: {
shardsInCluster: number;
},
staticShardingConf: {
shardsToSubscribe: number[]
}
}
Now there is the question on whether shards subscriptions should be part of the init config, or done when sending/receiving messages.
I think it is fine to aim for simple first, and make it part of the init config, which is restrictive. Which leads to:
type shardingMode = "auto" | "static"
autoShardingConf: {
shardsInCluster: number;
subscribeTo: string[]
},
staticShardingConf: {
shardsToSubscribe: number[]
}
}
Knowing that subscribeTo
would only be used in relay
mode. The name can be better. But contentTopics
is not really appropriate as we don't use the full content topic, just part of it. More thoughts for food.
How do we call the part of the content topic we use for autosharding?
|
||
interface ISend { | ||
public send(encoder: Encoder, message: Message): RequestID; | ||
public cancelRequest(requestId: RequestID): void; |
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.
Agreed it is too low level, rather a more simpler approach would be for the user to indicate a timeout for the send API. The user can also pass a context(this might be too Golang specific but is a very good concept for an API in general) which can be cancelled or timeout.
interface ISend { | ||
public send(encoder: Encoder, message: Message): RequestID; | ||
public cancelRequest(requestId: RequestID): void; | ||
public getPendingRequests(): RequestID[]; |
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 agree with @fryorcraken here. It would be better to have an event based approach to indicate success/failure/timeout etc.
}; | ||
|
||
interface ISend { | ||
public send(encoder: Encoder, message: Message): RequestID; |
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 do we need to have an encoder? Shouldn't encoder be more like a utility from waku/sdk app devs can use to encode their payload and then supply WakuMessage to be sent? This gives flexibility for devs to decide whether to use Waku encoder or some other way to encode their messages.
This looks like better separate of concerns in case apps want to use their own way to encode different message types e.g: in status we have private messages which are encrypted/encoded differently and public messages which are done differently. Ultimately status backend handsover only WakuMessage to the send API which seems more cleaner to me.
|
||
##### `RequestID` | ||
A `RequestID` is a `GUID` string that uniquely identifies a message send operation. | ||
This `GUID` MAY be used to cancel a scheduled request or to retrieve information about the associated message from the `Message Storage API`. |
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 not use the term messageHash
as it is used in Store API and in all other existing specs?
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.
since message hash takes into account timestamp
- due to retries it can change over time
hence we need a better way to identify requests TO Messaging API
FOR sending a message
then we have a way to match RequestID
with end message hash
This was my reasoning here, let me know what you think!
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.
ok, i see what you mean. I am wondering this may pose challenge in quering message from store as user won't have messageHash to work with or use SDS (as i think it relies on messageHashes).
But then again if the idea is to abstract away both of these from user, then it is fine i guess. But this is more of a transactionID between user and local waku messaging API and nothing beyond that right.
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 think it's fine for the request id to be purely implementation specific, and mainly used to track events.
|
||
##### `subscribe` | ||
Registers a new subscription to listen for messages processed by a given `Decoder`. | ||
The subscription is based on the `pubsubTopic` and `contentTopic` provided by the `Decoder`, |
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.
In case of relay contentTopic would be optional, whereas in case of Filter it is mandatory. Would be good to document his in the API so that users are aware based on the nodeType how the params are to be passed.
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.
In case of relay contentTopic would be optional, whereas in case of Filter it is mandatory.
Well, ideally one uses autosharding, making it the same interface for both relay and filter :)
The method MUST throw an error if the `pubsubTopic` provided by the `Decoder` is not supported by the underlying node. | ||
No specific order is guaranteed for callback invocation; | ||
subscription callbacks MUST be executed as messages are received from the protocol in use. | ||
If `confirmStored` is set to `true`, a recurring [STORE](../standards/core/store.md) query MUST be started. |
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 agree and think that we should have 2 configs as it is currently implemented in status. We have separate flag to indicate sendConfirmations
vs fetchMissingMessages
The subscription is uniquely identified by a `SubscriptionID`, | ||
which MUST be used for managing the subscription (e.g., for later unsubscribing). | ||
The method MUST throw an error if the `pubsubTopic` provided by the `Decoder` is not supported by the underlying node. | ||
No specific order is guaranteed for callback invocation; |
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 better to add a note that users would need to have a way to deduplicate messages (returned by 2 filter subscriptions or store nodes etc). Or is that implemented underneath this API? If so, better to mention that as well for implementers.
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.
yes I agree here, we may need to define a deduplication logic on payload within a timeframe.
Removes a previously created subscription identified by its unique `SubscriptionID`. | ||
Once unsubscribed, the associated `SubscriptionListener` MUST NOT receive any further messages. | ||
If the provided `SubscriptionID` does not correspond to an active subscription, | ||
the call MUST be ignored. |
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.
wouldn't it be better to return an error indicating the same?
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 think ignore is fine, because you may to unsubscribe for a clean shutdown but if the remote node already unsubscribe you because you were disconnected or something, you dont' want to be collecting errors.
Each identifier corresponds to a subscription that is in effect, | ||
and no duplicate values MUST be present in the returned array. | ||
|
||
##### `Decoder` |
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.
Another thing is you cannot just pass payload and contentTopic - the dev also needs the rest of the fields - like
meta
,timestamp
orrln proof
:)
Why not pass WakuMessage itself which has all these fields?
A significant challenge has been the lack of a unified abstraction for message exchange. | ||
Developers require a straightforward mechanism to send, | ||
receive, and track messages—whether originating from a node or propagated throughout its connected network. | ||
|
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.
Generic Comments:
- Might be good to have a section listing all API's and referring to subsections for each of them. otherwise for a dev/user it would be hard to navigate the spec to read so much detail.
- Also it might be a good idea to provide an API for the user to get localNodeInfo i.e (MultiAddr node is listening on, ENR of the node if applicable). This would be useful in relay node+service node scenarios. So maybe this API is applicable only if node is initialized with relay/serviceNode.
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.
Added table of content f1d100f
As for (2) - I think it is out of scope of Messaging API
as this is something that, I think, must be provided by any Waku client.
We can thought use an opportunity here to specify it. Let me know if you are in favor of it and I will push that change, @chaitanyaprem .
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.
As for (2) - I think it is out of scope of
Messaging API
as this is something that, I think, must be provided by any Waku client.
ok, since we are defining the API i thought we can provide this as well as it might be useful for users to know their nodeinfo. We can add it as it is a small API.
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.
sure, it's only couple of lines
which part of API do you think it fits best, @chaitanyaprem ?
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 am guessing where we have health indicator.
##### `SubscriptionListener` | ||
A callback function that is invoked whenever a new `MessageRecord` is received. | ||
It processes the incoming message and MAY return a `Promise` to support asynchronous handling. | ||
The `MessageRecord` is defined in the `Message Storage API` section. |
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.
Would be good to add a note that the callback is expected to not do any long processing so that underlying threads don't get blocked and drops messages.
We have noticed this issue in status once where message's were getting dropped as the app thread was blocked due to some sync logic which was causing reliability issues.
925d73c
to
b6e6e62
Compare
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.
good job. You did not address the question of language. I suggest to try C with a programmatic way of deriving the REST API indicators.
I have commented and there are a number of things that can be left to implementation detail. The spec is being too detailed right now.
I also expressed some opinion around the (local) message storage. I think it should be the opposite where we should define an API here of what a storage solution MUST implement (ie, provided by application developer), for the waku node to use.
|
||
```typescript | ||
{ | ||
mode: "edge" | "relay"; |
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.
If anyone has a better name than "relay" to express the "relay mode" in Status Desktop (service for PX+LP+Filter, usage of discv5+px+rdv for discovery, relay for sending and receiving.), I am all ears.
|
||
##### `shards` | ||
This property MUST be provided. | ||
An array of shard under a specified cluster that node MUST operate at as per [RELAY-SHARDING](https://github.com/waku-org/specs/blob/186ce335667bdfdb6b2ce69ad7b2a3a3791b1ba6/standards/core/relay-sharding.md). |
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 started to look into this, but I haven;t yet done a PR on nwaku side to propose a solution. I somewhat agree with the above but wanted to summarize a bit more.
1) Autosharding:
The number of shards in cluster is needed shardsInCluster
is a good name. I would not use "under".
2) Static sharding
In this instance, the shards to subscribe to need to be specified when one wants to receive messages from it.
shards
is not qualifying enough, so I'd be more explicit with shardsToSubscribe
Note that those configuration are mutually exclusive and there's a question on whether we want to represent that in the config.
type shardingMode = "auto" | "static"
autoShardingConf: {
shardsInCluster: number;
},
staticShardingConf: {
shardsToSubscribe: number[]
}
}
Now there is the question on whether shards subscriptions should be part of the init config, or done when sending/receiving messages.
I think it is fine to aim for simple first, and make it part of the init config, which is restrictive. Which leads to:
type shardingMode = "auto" | "static"
autoShardingConf: {
shardsInCluster: number;
subscribeTo: string[]
},
staticShardingConf: {
shardsToSubscribe: number[]
}
}
Knowing that subscribeTo
would only be used in relay
mode. The name can be better. But contentTopics
is not really appropriate as we don't use the full content topic, just part of it. More thoughts for food.
How do we call the part of the content topic we use for autosharding?
This property MUST be provided. | ||
An array of shard under a specified cluster that node MUST operate at as per [RELAY-SHARDING](https://github.com/waku-org/specs/blob/186ce335667bdfdb6b2ce69ad7b2a3a3791b1ba6/standards/core/relay-sharding.md). | ||
|
||
##### `shardsUnderCluster` |
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.
##### `shardsUnderCluster` | |
##### `shardsInCluster` |
An array of shard under a specified cluster that node MUST operate at as per [RELAY-SHARDING](https://github.com/waku-org/specs/blob/186ce335667bdfdb6b2ce69ad7b2a3a3791b1ba6/standards/core/relay-sharding.md). | ||
|
||
##### `shardsUnderCluster` | ||
This is an optional property that MUST be a positive integer. |
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.
in a C
interface you can specify must be uint
|
||
##### `shardsUnderCluster` | ||
This is an optional property that MUST be a positive integer. | ||
If not provided, it defaults to 8. |
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.
Drop any default in this document. Let's reduce the scope for now to avoid nitpicking and leave it to implementation detail.
I will create a set of "cluster presets" as well so we can solve some of the interop issues we discussed.
##### `stored` | ||
Defaults to `false`. | ||
Indicates whether a message has been confirmed to be present in the underlying [STORE](../standards/core/store.md) protocol. | ||
This property is populated based on a recurring [STORE](../standards/core/store.md) query described in `Acknowledgements` section or via the `History API`. | ||
Once a message is confirmed as stored, the `stored` property MUST be updated to `true` as a final state. | ||
If an error occurs during reception, the `error` field MUST be populated with implementation-specific details. | ||
|
||
##### `received` | ||
Defaults to `false`. | ||
Indicates whether a message was successfully received from the network via `Subscribe API`. | ||
Once a message is received, the `received` property MUST be updated to `true` as a final state. | ||
If an error occurs during reception, the `error` field MUST be populated with implementation-specific details. |
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.
as discussed earlier, merged those to a confirmed
status.
##### `getByRequestId` | ||
Retrieves the `MessageRecord` corresponding to the specified `RequestID` associated with a send request for a particular message. | ||
If the provided `RequestID` is invalid or no associated `MessageRecord` exists, | ||
the method MUST return nothing. | ||
|
||
##### `getByHash` | ||
Retrieves the `MessageRecord` corresponding to the provided `message hash`. | ||
The hash is computed deterministically based on the message content, | ||
as described in the [MESSAGE](https://github.com/vacp2p/rfc-index/blob/8ee2a6d6b232838d83374c35e2413f84436ecf64/waku/standards/core/14/message.md#deterministic-message-hashing) specification. | ||
If the provided hash is invalid or no associated `MessageRecord` exists, | ||
the method MUST return nothing. | ||
|
||
##### `getByContentTopic` | ||
Retrieves an array of `MessageRecord` objects corresponding to messages that were circulated under the specified `contentTopic`. | ||
The method accepts two optional parameters for pagination: | ||
- `skip`: A non-negative integer specifying the number of initial matching records to bypass. If `skip` exceeds the total number of matching records, the method MUST return an empty array. | ||
- `take`: A non-negative integer specifying the maximum number of records to return. If `take` is 0, the method MUST return an empty array. If `take` is greater than the number of available records, all remaining matching records are returned. | ||
|
||
If both `skip` `and` take are omitted, the method MUST return all matching records. | ||
If no records match the specified `contentTopic`, an empty array MUST be returned. | ||
|
||
##### `getBySubscriptionId` | ||
Retrieves an array of `MessageRecord` objects corresponding to messages received under the specified `SubscriptionID` (as defined in the `Subscribe API` section). | ||
The method accepts two optional parameters for pagination: | ||
- `skip`: A non-negative integer specifying the number of initial matching records to bypass. If `skip` exceeds the total number of matching records, the method MUST return an empty array. | ||
- `take`: A non-negative integer specifying the maximum number of records to return. If `take` is 0, the method MUST return an empty array. If `take` is greater than the number of available records, all remaining matching records are returned. | ||
|
||
If both `skip` and `take` are omitted, the method MUST return all matching records. | ||
If no records match the specified `SubscriptionID`, an empty array MUST be returned. | ||
|
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.
Too many APIs, especially with getPendingRequests
already there.
I would say pick ONE way to check on message status at first, and we can then extend as we learn from dogfooding.
For the purposes of this specification, three health states are defined: | ||
- `unhealthy`: Indicates that the node has lost connectivity for message reception, sending, or both, and as a result, it cannot reliably process or transmit messages. | ||
- `minimally healthy`: Indicates that the node meets the minimum operational requirements, although performance or reliability may be impacted. | ||
- `healthy`: Indicates that the node is operating optimally, with full support for message processing and transmission. |
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.
delete that and just referred to the verbose definition in eponymous sections below
This PR attempts to define
Messaging API
as per all previous conversations in various threads or offline.This doc describes following parts of said umbrella API:
initial configuration
- just configuration;Send API
- send for message with retry and ack checks;Subscribe API
- initiate, track and manage ongoing source of messages;Messaging Storage API
- entry point for access for messages;History API
- allows to fetch past messages;Event Source API
- provides reactive way for app devs;These sections cover programmatic API as well as
REST
API.For code sections
typescript
was used AND I am not sure it is the best option for our case so I propose either to:Unfortunately the document turned out rather big, with some degree of repetitions in explanations and references between sections. Please, let me know right after first read if it is too much and I will rewrite it for further review.