feat: support creating node using application info #1809
Conversation
size-limit report 📦
|
packages/sdk/src/create.ts
Outdated
| return _createNode(shardInfo.shardInfo, options); | ||
| } | ||
|
|
||
| export async function createNodeFromContentTopics( |
There was a problem hiding this comment.
I think combining it with createNode is better.
To me it seems reasonable to keep amount of create* function as low as possible. Ideally 2: light node, relay node (for later).
There was a problem hiding this comment.
adklempner
force-pushed
the
feat/application-version-create-node
branch
from
3cff754
to
e22cdd0
Compare
adklempner
force-pushed
the
feat/application-version-create-node
branch
2 times, most recently
from
93e93ef
to
5181b38
Compare
adklempner
changed the title
adklempner
force-pushed
the
feat/application-version-create-node
branch
4 times, most recently
from
a4f3ac0
to
a64c7d2
Compare
adklempner
force-pushed
the
feat/application-version-create-node
branch
from
a64c7d2
to
f8b8df8
Compare
| it("given an application and version, creates a waku node with the correct pubsub topic", async function () { | ||
| const contentTopic = ensureValidContentTopic(ContentTopic); | ||
| const waku = await createApplicationNode( | ||
| contentTopic.application, |
There was a problem hiding this comment.
At this point I am thinking if we need a class for it:
class ContentTopic {
static public fromString(value: string): ContentTopic {
return new ContentTopic({ value });
}
constructor({ value, application, version }) {}
get application() {}
get version() {}
get encoding() {}
}before it seemed as overhead but more we go to autosharding realm I think it would a useful entity to:
- encapsulate
contentTopicrelated utils & types; - mechanism to re-usability and common interface;
@adklempner @danisharora099 would like to hear your thoughts on it
There was a problem hiding this comment.
yeah, i like your idea!
i think we can refactor sdk/utils/content_topic into class-based 👌
There was a problem hiding this comment.
@adklempner if you agree with approach we can follow up with this in a separate issue. Can be linked to https://github.com/waku-org/pm/issues/143
| @@ -6,6 +6,15 @@ import { CreateWakuNodeOptions, WakuNode, WakuOptions } from "./waku.js"; | |||
|
|
|||
| export { Libp2pComponents }; | |||
|
|
|||
| export async function createApplicationNode( | |||
There was a problem hiding this comment.
at this point we have way to many create* functions
for now let's have only createNode and extend with additional parameter in options
There was a problem hiding this comment.
@adklempner do you need support with this one? we can go ahead with this PR and follow up later as we discussed at prev PM
| application: string, | ||
| version: string, |
There was a problem hiding this comment.
you can simplify the API with two args into just one by taking contentTopic: ContentTopic as the argument instead
| it("given an application and version, creates a waku node with the correct pubsub topic", async function () { | ||
| const contentTopic = ensureValidContentTopic(ContentTopic); | ||
| const waku = await createApplicationNode( | ||
| contentTopic.application, |
There was a problem hiding this comment.
yeah, i like your idea!
i think we can refactor sdk/utils/content_topic into class-based 👌
|
@adklempner do you think we should proceed with this PR or take a step back and reconsider? |
|
Let's close this PR as a lot of things has changed and we'd need to re-evaluate this approach. |
Problem
All functions to create a node allow it to be setup for either autosharding or static sharding depending on which options are passed. However, we want to encourage users to run nodes with autosharding. Requiring specific configuration options in order to set up a node to run autosharding makes it more difficult to do so.
Solution
This commit adds SDK functions for creating a node configured to use autosharding by providing an application/version or a list of content topics.
Notes