Skip to content
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

feat: support creating node using application info #1809

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/sdk/src/create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@ import { CreateWakuNodeOptions, WakuNode, WakuOptions } from "./waku.js";

export { Libp2pComponents };

export async function createApplicationNode(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point we have way to many create* functions

for now let's have only createNode and extend with additional parameter in options

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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,
Comment on lines +10 to +11
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can simplify the API with two args into just one by taking contentTopic: ContentTopic as the argument instead

options: CreateWakuNodeOptions = { pubsubTopics: [] }
): Promise<LightNode> {
options = options ?? {};
options.shardInfo = { application, version };
return createNode(options);
}
/**
* Create a Waku node configured to use autosharding or static sharding.
*/
Expand Down
21 changes: 21 additions & 0 deletions packages/tests/tests/sdk/application_version.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { createApplicationNode, WakuNode } from "@waku/sdk";
import {
contentTopicToPubsubTopic,
ensureValidContentTopic
} from "@waku/utils";
import { expect } from "chai";

describe("SDK: Creating by Application and Version", function () {
const ContentTopic = "/myapp/1/latest/proto";

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,
Copy link
Collaborator

@weboko weboko Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 contentTopic related utils & types;
  • mechanism to re-usability and common interface;

@adklempner @danisharora099 would like to hear your thoughts on it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i like your idea!
i think we can refactor sdk/utils/content_topic into class-based 👌

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

contentTopic.version
);
const expectedPubsubTopic = contentTopicToPubsubTopic(ContentTopic);

expect((waku as WakuNode).pubsubTopics).to.include(expectedPubsubTopic);
});
});
Loading