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.

Sign up for GitHub

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

Jump to bottom

feat!: use ShardingParams on subscriptions, make Decoder/Encoder auto sharding friendly by default #1958

Merged
merged 45 commits into from
Apr 28, 2024
Merged

feat!: use ShardingParams on subscriptions, make Decoder/Encoder auto sharding friendly by default #1958

merged 45 commits into from
Apr 28, 2024

Conversation

weboko
Copy link
Collaborator

@weboko weboko commented Apr 15, 2024 •
edited
Loading

Problem

Seems latest fleet update is not allowing to subscribe to DefaultPubsubTopic anymore and we should be using shards.

Solution

Use pubsub topic for subscription creation from those determined from shards / contentTopics

Though this PR is huge it includes only following changes:

  • change to the logic of Decoders/Encoders that makes use of default cluster ID and maps content topic to pubsubTopic;
  • createSubscription on Filter accepts ShardingParams;
  • updates to the tests to check for the above;
  • subscribe checks pubsub topics from Decoders;
  • some improvements to the tests to remove duplication;

Notes

Addresses - #1961
Next PR - #1959

@weboko weboko requested a review from a team as a code owner April 15, 2024 23:12
weboko
weboko commented Apr 15, 2024
View reviewed changes
packages/interfaces/src/waku.ts
@@ -18,7 +18,7 @@ export interface Waku {

connectionManager: IConnectionManager;

dial(peer: PeerId | Multiaddr, protocols?: Protocols[]): Promise<Stream>;
dial(peer: PeerId | MultiaddrInput, protocols?: Protocols[]): Promise<Stream>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix for discrepancy in types - this prevents us from doing .dial(multiAddrStr) in TS

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 15, 2024 •
edited
Loading

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 180.93 KB (+0.11% 🔺) 3.7 s (+0.11% 🔺) 13.8 s (-43.29% 🔽) 17.5 s
Waku Simple Light Node 180.85 KB (-0.01% 🔽) 3.7 s (-0.01% 🔽) 22.2 s (+30.04% 🔺) 25.8 s
ECIES encryption 23.08 KB (-0.14% 🔽) 462 ms (-0.14% 🔽) 6.5 s (+11.66% 🔺) 6.9 s
Symmetric encryption 22.55 KB (-0.15% 🔽) 452 ms (-0.15% 🔽) 5.1 s (-20.88% 🔽) 5.6 s
DNS discovery 72.42 KB (0%) 1.5 s (0%) 12.3 s (+28.64% 🔺) 13.7 s
Peer Exchange discovery 74.1 KB (+0.18% 🔺) 1.5 s (+0.18% 🔺) 15.6 s (+64.29% 🔺) 17.1 s
Local Peer Cache Discovery 67.64 KB (0%) 1.4 s (0%) 16.5 s (+14.35% 🔺) 17.9 s
Privacy preserving protocols 38.87 KB (0%) 778 ms (0%) 11.2 s (+2.99% 🔺) 12 s
Waku Filter 111.42 KB (+0.16% 🔺) 2.3 s (+0.16% 🔺) 18.2 s (+93.72% 🔺) 20.5 s
Waku LightPush 110.11 KB (-0.12% 🔽) 2.3 s (-0.12% 🔽) 12.1 s (+9.49% 🔺) 14.3 s
History retrieval protocols 110.72 KB (+0.01% 🔺) 2.3 s (+0.01% 🔺) 15 s (-29.78% 🔽) 17.2 s
Deterministic Message Hashing 4.83 KB (0%) 97 ms (0%) 701 ms (+64.37% 🔺) 798 ms

@weboko weboko marked this pull request as draft April 15, 2024 23:32
@weboko weboko changed the title fix: use pubsubTopic from current on filter subscribe fix!: use pubsubTopic from current on filter subscribe Apr 18, 2024
@weboko weboko marked this pull request as ready for review April 22, 2024 11:39
@danisharora099
Copy link
Collaborator

I might be missing something but I don't fully understand what the PR is solving for. Can you please elaborate, or point to related issues/references?

@weboko
Copy link
Collaborator Author

weboko commented Apr 22, 2024

I might be missing something but I don't fully understand what the PR is solving for. Can you please elaborate, or point to related issues/references?

This problem is addressed - #1961
Happens due to https://discord.com/channels/1110799176264056863/1111539372618166294/1231967635722342540

@weboko weboko mentioned this pull request Apr 22, 2024
Merged
@weboko weboko changed the title fix!: use pubsubTopic from current on filter subscribe feat!: use pubsubTopic from current on filter subscribe Apr 22, 2024
@weboko weboko mentioned this pull request Apr 22, 2024
Closed
danisharora099
danisharora099 reviewed Apr 23, 2024
View reviewed changes
packages/sdk/src/protocols/filter.ts Outdated Show resolved Hide resolved
packages/sdk/src/protocols/filter.ts Outdated Show resolved Hide resolved
@weboko weboko mentioned this pull request Apr 26, 2024
Merged
@weboko weboko changed the title feat!: use pubsubTopic from current on filter subscribe feat!: use ShardingParams on subscriptions, make Decoder/Encoder auto sharding friendly by default Apr 26, 2024
weboko
weboko commented Apr 26, 2024
View reviewed changes
packages/interfaces/src/protocols.ts
@@ -29,7 +29,7 @@ export type IBaseProtocolSDK = {
};

export type ContentTopicInfo = {
clusterId: number;
clusterId?: number;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Important change - assumption is that default choice is TWN cluster ID.

weboko
weboko commented Apr 26, 2024
View reviewed changes
packages/sdk/src/protocols/filter.ts
): Promise<SubscriptionManager> {
const pubsubTopic =
typeof pubsubTopicShardInfo == "string"
? pubsubTopicShardInfo
: singleShardInfoToPubsubTopic(pubsubTopicShardInfo);
: shardInfoToPubsubTopics(pubsubTopicShardInfo)?.[0];
Copy link
Collaborator Author

@weboko weboko Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ShardingParams can be mapped to many pubsubTopics - we provide subscription to one.

Any idea how to handle it better? @waku-org/js-waku-developers

Copy link
Member

Choose a reason for hiding this comment

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

@weboko we could forbid passing shard info where shards length is greater than 1

fbarbu15
fbarbu15 approved these changes Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@fbarbu15 fbarbu15 left a comment

Choose a reason for hiding this comment

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

LGTM, Awesome refactoring of the tests and utils!

packages/tests/tests/filter/single_node/subscribe.node.spec.ts Show resolved Hide resolved
@weboko weboko merged commit f3627c4 into master Apr 28, 2024
9 of 10 checks passed
@weboko weboko deleted the weboko/protocol-fixes branch April 28, 2024 09:15
@weboko weboko mentioned this pull request Apr 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adklempner adklempner adklempner approved these changes

@fbarbu15 fbarbu15 fbarbu15 approved these changes

@danisharora099 danisharora099 Awaiting requested review from danisharora099

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@weboko @danisharora099 @adklempner @fbarbu15