Introduce the Waku API's subscribe function (+ refactor waku_api.md)#86
Introduce the Waku API's subscribe function (+ refactor waku_api.md)#86
Conversation
Ivansete-status
left a comment
Ivansete-status
left a comment
There was a problem hiding this comment.
LGTM! Thanks!
Just added some nitpicks that I hope you find useful
Cheers!
| Examples of irremediable failures are: | ||
|
|
||
| #### Type definitions | ||
| - Invalid content topic format |
There was a problem hiding this comment.
This error can be returned straight away and doesn't require an event.
In fact, the majority of errors can be given in a result. Nevertheless, I think is nice keeping the subscr-error event in case is needed.
There was a problem hiding this comment.
I think this comes from the mismatch between the C FFI API incarnation of this IDL spec and the e.g. Nim (or Rust etc.) incarnation of the API. In the C version everything returns void and responses are events, even if they can be given immediately (I think that's how the current C "kernel API" interface works?).
We can change everything here to be aware of sync results (return a proper struct type in a resut<> and minimize the use of events when they know they can have a sync response.
There was a problem hiding this comment.
Done. I deleted the subscription event source for now, we can reintroduce it if it's actually needed. But I don't think it is, since the app will already get a health status change notification for each content topic (a subscribed topic is immediately "not healthy," but once connections for it are verified, it should fire the sequence of health upgrade events).
standards/application/waku-api.md
Outdated
| parameters: | ||
| - name: waku | ||
| type: WakuNode | ||
| description: "The node to check for a binary connectivity status." |
There was a problem hiding this comment.
I can't quite fully get this api's purpose.
There was a problem hiding this comment.
This is just the retconned spec of the checkApiAvailability function that's already in waku/api/api.nim, which answers the simple question of whether it makes basic sense to even attempt to e.g. spawn a message sending task right now or whether it should be immediately bounced off until the node connectivity situation improves (also checks for nil together with the Disconnected state check).
This can be moved off of the API (public, product interface) and be used internally by the Send API implementation only, but I think it's good to expose this in the API.
The full Health API spec is not here yet, but once the Health API PR is merged, I'll open a PR to update the spec here with all the LMN API functions that will already be supported.
There was a problem hiding this comment.
Maybe we can adopt a more conservative approach and hide as much API as possible. If that can be tackled by the SendAPI I think is better to move it off for now.
There was a problem hiding this comment.
Deleted checkApiAvailability, we can figure out where to move stuff to in the code later.
Maybe we should align waku/api/* specifically to stuff that ultimately makes its way into this spec only. (But also waku/events/* and waku/requests/* will be part of the API?)
Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com>
Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com>
Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com>
…into waku-api/subscribe
* Use sync impl awareness for subscription proc sync results * Delete subscription event source (for now) * Remove checkApiAvailability
fcecin
changed the title
3 participants
Description
This PR introduces subscribe and unsubscribe APIs to the Logos Messaging API spec.
This PR also provides an initial refactor of the
waku_api.mdspec file as part of merging master into this PR.Notes:
JavaScript implementation of subscribe/unsubscribe: logos-messaging/logos-messaging-js#2667