-
Notifications
You must be signed in to change notification settings - Fork 3
Add Send API to Waku API #87
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
Conversation
Ivansete-status
left a comment
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.
Some niptick comments so far, thanks!
Co-authored-by: Ivan FB <[email protected]>
… weboko/waku-send-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.
Pull Request Overview
This PR adds a new Send API to the Waku API specification that enables sending messages through the Waku network. The addition provides a standardized interface for applications to publish messages with support for content topics, ephemeral messages, and rate limiting.
- Adds
byteas a new primitive type to support binary message payloads - Introduces
SendMessagetype definition with fields for content topic, payload, ephemeral flag, and rate limit proof - Defines
sendfunction that accepts a message and returns a request ID or error
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get 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.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get 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.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
fryorcraken
left a comment
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 of my comment was pending :( )
fryorcraken
left a comment
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 don't agree with some of the decisions but at the end of the day, API design must be informed by API usage.
Let's see how this holds when implement in nim and js, and used in the reliable channel layer (both in Nim and JS).
|
I am not merging till @Ivansete-status or @NagyZoltanPeter approves |
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.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
standards/application/waku-api.md
Outdated
| eventType: | ||
| type: string | ||
| default: "message:sent" | ||
| description: "Event type identifier" |
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 not sure I agree with this syntax. WDYM by default: "message:sent"? what other vale could it be?
I would suggest to opt for the syntax proposed in https://github.com/waku-org/specs/pull/86/files
The object that emit events, define the "topics" of the events (eg message:sent):
types:
EventEmitter:
type: event_emitter
description: "An event emitter for general node events."
events:
"error:subscribe":
type: string
description: "Event emitted when subscription to a content topic irremediably fails. The event contains an error message."And here, if you want you can have type: MessageSentEvent instead of type: string and defined MessageSentEvent with 2 fields: message_hash and request_id.
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.
WDYM by default: "message:sent"
I meant that eventType is a property on an object that has value of message:sent
I don't understand this syntax:
events:
"error:subscribe":
type: string
description: "..."
what is error:subscribe and how exactly it is a string and how to get access to it?
for the syntax I used message:sent is a value of eventType property that is defined on the type of MessageSent object
wdyt @fryorcraken
fryorcraken
left a comment
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.
- the yaml syntax for even types and emitting needs to be reworked,
- you need to actually define the
sendand event emitter fields onWakuNodetype definition - do as you think best re
ephemeral - We need to give the
message_hashto the consumer: I suggest to return it along side therequest_idon all events (message:sent,message:propagated,message:error). Could be optional so that we can use the same type across all event, and not setmessage_hashif not relevant.
I would recommend to have a nim PR first before merging the spec |
NagyZoltanPeter
left a comment
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.
Approving it as IMO what's discussed is something Nim implementation can follow.
Ok, draft will be tere next week. |
Ivansete-status
left a comment
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.
LGTM! Thanks for it! 💯
Just added some nitpicks that I hope you find useful
Added property definitions for events in Waku API documentation.
|
I don't think the event syntax works. here is a proposed alternative: #94 (review) |
|
@weboko |
|
@NagyZoltanPeter added description to the events @fryorcraken added your event emitter syntax with slight change (description is in the event object definition and not in event emitter) |
|
Merged with minor change:
|
Implementation - logos-messaging/js-waku#2583