Conversation
6e40863 to
93bd790
Compare
Ivansete-status
left a comment
There was a problem hiding this comment.
This is really superb so far! Congrats for it 🙌
I'm just suggesting minor tweaks
NagyZoltanPeter
left a comment
There was a problem hiding this comment.
Added some recomendation.
standards/application/waku-api.md
Outdated
| - No `default` means that the value is mandatory | ||
| - Primitive types are `string`, `int`, `bool` and `uint` | ||
| - Complex pre-define types are `struct`, `option` and `array` | ||
| - Primitive types are preferred to describe the API for simplicity, the implementator may prefer a native type (e.g. `string` vs `Multiaddr` object/struct) |
There was a problem hiding this comment.
I would argue with it, as such API has reduntant use of complex types (structs like WakuMessage or even very specific named types like Multiaddr). I think IDL shall support references to such types to allow reuse them over native types.- much like OpenAPI syntax.
This is even beneficial if we think it is harder to read as it leads more comprehensive api definition.
There was a problem hiding this comment.
I am not convinced Waku Message needs to leak out of the API.
In terms of Multiaddr, I don't want to prescribe the API to have to only accept an object Multiaddr because in some language like JS, a string is often used and expected.
Hence not entirely sure the best way to express that. I gave it a go in adaba54 (#65)
There was a problem hiding this comment.
IMHO , Multiaddr type doesn't seem as generic as option, array, struct. I wouldn't add it adaba54
There was a problem hiding this comment.
I think we cna expect it to be a string in most languages. so will do that.
standards/application/waku-api.md
Outdated
| active_relay_shards: | ||
| type: array<uint> | ||
| constraints: mode == "relay" | ||
| default: [] | ||
| description: "The shards for relay to subscribe to and participate in." |
There was a problem hiding this comment.
I am actually not convinced this should be available when initializing a node.
The action here is that "subscribe" will be called on those shards. "subscribe" is a live action to be done on a running node.
While it is fine for wakunode2 to have a way to specify shards to subscribe to on the CLI, it should be seen separately for node instantiation.
Hence I think I would recommend removing it.
standards/application/waku-api.md
Outdated
| @@ -0,0 +1,224 @@ | |||
| --- | |||
| title: MESSAGING-API | |||
There was a problem hiding this comment.
It seems we aligned on new naming.
| title: MESSAGING-API | |
| title: WAKU-API |
standards/application/waku-api.md
Outdated
| - `option`: a value that can be set or left null | ||
| - `array`: iterable object containing values of all the same type | ||
| - `multiaddr`: a libp2p multiaddress; may be an object or a string, most idiomatic approach depending on the language | ||
| - `result`: an enum type that either contain a return value (success), or an error (failure). The error is left to the implementor |
There was a problem hiding this comment.
we can specify somewhere that result is recommended way of communicating errors rather than throw but it can be left to implementations
standards/application/waku-api.md
Outdated
| The accessibility of Waku protocols is capped by the accessibility of their implementations, and hence API. | ||
| This RFC enables a concerted effort to draft an API that is simple and accessible, and provides an opinion on sane defaults. | ||
|
|
||
| This effort is best done in an RFC, allowing all current implementors to review and agree on it. |
There was a problem hiding this comment.
nit: we can omit this line, seems it doesn't bring much value
standards/application/waku-api.md
Outdated
| - `multiaddr`: a libp2p multiaddress; may be an object or a string, most idiomatic approach depending on the language | ||
| - `result`: an enum type that either contain a return value (success), or an error (failure). The error is left to the implementor | ||
| - `error`: Left to the implementor on whether `error` types are `string` or `object` in the given language. | ||
| - The first parameter of a function MAY be considered as the object on which a method is called. It is left to the implementor on whether an objective programming approach is idiomatic to the language. |
There was a problem hiding this comment.
this line seems to be too specific even though it is true for nim and rust
There was a problem hiding this comment.
we can re-introduce it when we define function that needs a reference to some exiting Waku node instance.
standards/application/waku-api.md
Outdated
| ## The Waku API | ||
|
|
||
| ```yaml | ||
| api_version: "0.0.1" |
There was a problem hiding this comment.
just general question: why not start with 1.0.0?
There was a problem hiding this comment.
As usual, let's mature things before we decide to crystallize and make it a 1.0.0
Being able to set what shard to subscribe too when initialising a Waku node is a biased towards service nodes. Indeed, subscribing to shard should be done with the `subscribe` API (yet to be defined). Which can be used with a content topic, and pubsub topic (only if static sharding). A service node such as the `wakunode2` binary can still accept "shards to subscribe to" via CLI, but this is unrelated to the Waku node conf, and the `wakunode2` software should simply call `subscribe` when ready.
|
Only comment node configuration and initialization part of this PR. The work here is highly controversial, so let's focus on one bit at a time. Discussions related to other functions such as |
Reducing the scope, we can review Rust, Golang and C in a separate PR.
standards/application/waku-api.md
Outdated
| fields: | ||
| mode: | ||
| type: string | ||
| constraints: ["edge", "relay"] |
There was a problem hiding this comment.
core or standard are good alternatives to relay.
There was a problem hiding this comment.
I wouldn't change relay. It sounds correct to me.
There was a problem hiding this comment.
tough to get right, convo ongoing here: https://discord.com/channels/1110799176264056863/1389431080012087306/1421097560113942549
|
|
||
| If the `mode` set is `edge`, the initialised `WakuNode` MUST mount: | ||
|
|
||
| - [LIGHTPUSH](https://github.com/vacp2p/rfc-index/blob/main/waku/standards/core/19/lightpush.md) as client |
There was a problem hiding this comment.
Metadata should be loaded too.
After feedback from @osmaczko
|
@jimstir I think it's going to be hard to maintain a spell check dictionary. |
Ivansete-status
left a comment
There was a problem hiding this comment.
LGTM! Thanks so much for it! 🥳
Just added another nitpick one ;P
standards/application/waku-api.md
Outdated
| constraints: [ "store", "filter" ] | ||
| # Until further dogfooding, assuming default false, usage of SDS should be preferred | ||
| default: [ "none" ] |
There was a problem hiding this comment.
EDITED:
I think we don't need that message_confirmation parameter. That can be considered internally as per the given operation mode (edge, relay.)
EDITED: disregard the following comment. I was confused.
Maybe :)?
| constraints: [ "store", "filter" ] | |
| # Until further dogfooding, assuming default false, usage of SDS should be preferred | |
| default: [ "none" ] | |
| constraints: [ "none", "store", "sds" ] | |
| # Until further dogfooding, assuming default none, usage of SDS should be preferred | |
| default: [ "none" ] |
Besides, I can't quite get why an array<string> is needed
There was a problem hiding this comment.
Besides, I can't quite get why an array is needed
@Ivansete-status as I understand it is because we can have multiple confirmation mechanisms working at the same time filter + store
There was a problem hiding this comment.
What @weboko said: the p2p reliability strategies are not mutually exclusive.
standards/application/waku-api.md
Outdated
| The accessibility of Waku protocols is capped by the accessibility of their implementations, and hence API. | ||
| This RFC enables a concerted effort to draft an API that is simple and accessible, and provides an opinion on sane defaults. | ||
|
|
||
| The API defined in this document is an opinionated-by-purpose method to use the more agnostic [WAKU2]() protocols. |
There was a problem hiding this comment.
| The API defined in this document is an opinionated-by-purpose method to use the more agnostic [WAKU2]() protocols. | |
| The API defined in this document is an opinionated-by-purpose method to use the more agnostic [WAKU2](https://github.com/vacp2p/rfc-index/blob/7b443c1aab627894e3f22f5adfbb93f4c4eac4f6/waku/standards/core/10/waku2.md) protocols. |
standards/application/waku-api.md
Outdated
| - No `default` means that the value is mandatory, meaning a `default` value implies an optional parameter. | ||
| - Primitive types are `string`, `int`, `bool`, `enum` and `uint` | ||
| - Complex pre-defined types are: | ||
| - `struct`: object and other nested types. |
There was a problem hiding this comment.
nit: by being biased by high level languages I would prefer object instead of struct
standards/application/waku-api.md
Outdated
| message_confirmation: | ||
| type: array<string> | ||
| constraints: [ "store", "filter" ] | ||
| # Until further dogfooding, assuming default false, usage of SDS should be preferred |
There was a problem hiding this comment.
I don't understand why we mention SDS here as it is in dogfooding / experimental stage too
There was a problem hiding this comment.
I removed, what I mean is that I expect we change the default in the future.
standards/application/waku-api.md
Outdated
| type: array<string> | ||
| # Default means the node does not bootstrap, e.g. for local development | ||
| default: [] | ||
| description: "Nodes to connect to; used for discovery bootstrapping and quick connectivity. entree and multiaddr formats are accepted." |
There was a problem hiding this comment.
nit: and we can remove the comment above
| description: "Nodes to connect to; used for discovery bootstrapping and quick connectivity. entree and multiaddr formats are accepted." | |
| description: "Nodes to connect to; used for discovery bootstrapping and quick connectivity. entree and multiaddr formats are accepted. If not provided, node does not bootstrap (e.g for local development)" |
There was a problem hiding this comment.
if we ever add mDNS the comment will be false. But will add in the mean time.
standards/application/waku-api.md
Outdated
| static_store_nodes: | ||
| type: array<string> | ||
| default: [] | ||
| description: "Only the passed nodes are used for store queries, discovered store nodes are discarded." |
There was a problem hiding this comment.
sorry if you mentioned it somewhere and I didn't see
question: why discarded? previously we were discussing it in the way that discovered nodes will be de-prioritized and used only if static store nodes are not available
There was a problem hiding this comment.
I think we can re-adjust at implementation time. Will correct and add todo.
standards/application/waku-api.md
Outdated
| description: "The auto-sharding config, if sharding mode is `auto`" | ||
| message_validation: | ||
| type: MessageValidation | ||
| # If the default config for TWN is not used, then we still provide a message validation default |
There was a problem hiding this comment.
| # If the default config for TWN is not used, then we still provide a message validation default | |
| description: If the default config for TWN is not used, then we still provide a message validation default |
standards/application/waku-api.md
Outdated
| type: string | ||
| fields: | ||
| listen_ipv4: | ||
| # Is not applicable in some environments such as browser. |
There was a problem hiding this comment.
nit: should we include comments into description? I think this gives needed context and doesn't over bloat them
standards/application/waku-api.md
Outdated
| TheWakuNetworkPreset: | ||
| type: WakuConfig | ||
| fields: | ||
| remote_nodes: [ "enrtree://AIRVQ5DDA4FFWLRBCHJWUWOO6X6S4ZTZ5B667LQ6AJU6PEYDLRD5O@sandbox.waku.nodes.status.im" ] |
There was a problem hiding this comment.
I am not sure it worth mentioning remote nodes and contract address as they can get outdated rather fast: unpredicted contract changes like we did recently etc.
There was a problem hiding this comment.
I see your point. I think it's fine to have it and forces us to update it as the intent is to ensure that all implementations are interoperable by default.
weboko
left a comment
There was a problem hiding this comment.
left some comments, lgtm otherwise
amazing to see progress on Waku API init
Yea, the nature of specifications, new terms will always be introduced which will require constant updates to the dictionary. |
This PR intends to introduce the customer Interface Definition Language, by proposing an initialization function and accompanying config for the Waku API.
The "Waku API" is a proposed renamed of "Waku Messaging API" because the term "messaging" is very generic and is going to also be used by Status to qualify the Waku Chat SDK API.
Also best to start generic for us with "Waku API", as this is the API we want developers to use. And we can refine at a later stage if there is a more specialized API we want to offer (aka "RLN API").
This PR is a suggestion to replace #19. I have already made comment in #19, but in short:
#68