-
Notifications
You must be signed in to change notification settings - Fork 48
feat: introduce Send API #2583
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?
feat: introduce Send API #2583
Conversation
size-limit report 📦
|
69052e1 to
998c2a1
Compare
|
I still need to add some E2E Tests and better dogfooding example. |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
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 introduces a unified Send API for Waku nodes, simplifying message sending by providing a lean interface that handles routing, acknowledgments, and retry logic internally.
- Adds
sendmethod toIWakuinterface that accepts simplified message data structure - Implements messaging backend with automatic acknowledgment tracking via filter and store protocols
- Integrates retry logic and subscription management for reliable message delivery
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/interfaces/src/waku.ts | Adds send method to IWaku interface and deprecation warnings |
| packages/interfaces/src/message.ts | Defines ISendMessage and RequestId types for new API |
| packages/sdk/src/waku/waku.ts | Integrates Messaging class into WakuNode implementation |
| packages/sdk/src/messaging/* | Complete messaging system with sender, ack manager, and message store |
| packages/core/src/lib/light_push/* | Extends light push to return proto messages for acknowledgment tracking |
| packages/interfaces/src/sender.ts | Adds numPeersToUse option to ISendOptions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| this.filterAckManager.subscribe(decoder), | ||
| this.storeAckManager.subscribe(decoder) | ||
| ]) | ||
| ).some((success) => success); |
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 is only one success sufficient?
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.
we rely on background process of Filter to automatically recover subscriptions or Store to continue querying
| if (this.toSubscribeContentTopics.size > 0) { |
that's why enough to get confirmation for at least one successful in-place subscription
because later it will recover in async way
| } | ||
| } | ||
|
|
||
| class StoreAckManager { |
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.
nit: splitting StoreAckManager and FilterAckManager into separate classes from AckManager seems overkill, unless there's a good reason for someone to use them outside of AckManager
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 disagree, it seems logical to keep them separate and might be useful once we want to provide other ack mechanics such as SDS.
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.
might be useful
I would recommend avoiding introducing complexity for a might and just proceed with a refactoring once this complexity becomes necessary.
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.
once we want to provide other ack mechanics such as SDS.
We already provide those via ReliableChannel and I am not convinced we want to merge routing (Waku API) and application (Reliable Channel) level concepts in the same objects.
|
Will wait for agreement on the spec before review of this PR. |
| contentTopic: string; | ||
| payload: Uint8Array; | ||
| ephemeral?: boolean; | ||
| rateLimitProof?: boolean; |
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 think the API consumer should be exposed to the rate limit proof, this sit behind the Waku API. see createNode spec for details.
| * @param {ISendMessage} message - The message to send. | ||
| * @returns {Promise<RequestId>} A promise that resolves to the request ID | ||
| */ | ||
| send(message: ISendMessage): Promise<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.
the Waku message Id must be exposed in the API to enable SDS's retrieval hints.
I strongly recommend for it to be returned by send to avoid issues with timestamp and pubsubtopic (both being used to calculate message id)
| /** | ||
| * Send message data structure used in {@link IWaku.send}. | ||
| */ | ||
| export interface ISendMessage { |
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 convinced we need a type here, it just makes it harder to understand the API.
Just do
send: (
contentTopic: string;
payload: Uint8Array;
ephemeral?: boolean;
) => Promise<Uint8Array | undefined>;
Problem / Description
Creation of unified
Sendmethod is needed.Solution
Make sending a message over the network completely lean.
This PR adds:
IWaku- new.sendmethod that accepts lean data structure to route the messagenetworkConfigonIWakunodeSubscribefunctionality by creatingAckManagerfacadeNotes
Checklist