feat!: add support for sharded pubsub topics & remove support for named pubsub topics

[danisharora099]

This PR:

• changes the create${xyz}Node() API to take in ShardInfo instead of a PubsubTopic
  • if shardInfo is not provided, it falls back to the DefaultPubsubTopic
• changes the createEncoder and createDecoder API to take in ShardInfo optionally
  • if a value is not provided, the DefaultPubsubTopic is used
• changes filter.createSubscription() to take in SingleShardInfo optionally instead of a string
  • if no shard info is passed, the DefaultPubsubTopic is used

Notes

• resolves feat: Deprecate Named Sharding and Update Lightpush Client API #1690

TODO:

• update tests
• update docs: feat: add docs for deprecated namespace pubsub topic  docs.waku.org#131

[github-actions]

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
Waku core	78.36 KB (+0.34% 🔺)	1.6 s (+0.34% 🔺)	1.4 s (+15.99% 🔺)	2.9 s
Waku Simple Light Node	240.89 KB (+0.08% 🔺)	4.9 s (+0.08% 🔺)	2.8 s (+4.7% 🔺)	7.6 s
ECIES encryption	72.85 KB (+0.14% 🔺)	1.5 s (+0.14% 🔺)	1.8 s (+43.69% 🔺)	3.3 s
Symmetric encryption	72.83 KB (+0.14% 🔺)	1.5 s (+0.14% 🔺)	1.1 s (-44.18% 🔽)	2.5 s
DNS discovery	120.85 KB (0%)	2.5 s (0%)	1.9 s (+3.27% 🔺)	4.3 s
Privacy preserving protocols	125.75 KB (+0.07% 🔺)	2.6 s (+0.07% 🔺)	2.6 s (-7.44% 🔽)	5.1 s
Light protocols	75.94 KB (+0.31% 🔺)	1.6 s (+0.31% 🔺)	1.1 s (-25.78% 🔽)	2.6 s
History retrieval protocols	74.85 KB (+0.29% 🔺)	1.5 s (+0.29% 🔺)	756 ms (-1.94% 🔽)	2.3 s
Deterministic Message Hashing	5.65 KB (0%)	113 ms (0%)	197 ms (-48.22% 🔽)	310 ms

[fbarbu15]

Fixed tests and added 2 new ones with commit
Test report

[danisharora099]

cc @LordGhostX re docs: waku-org/docs.waku.org#131

[fryorcraken]

Adding static sharding support is a feature, not a chore.

[fryorcraken]

This is a very important change and the PR also contains a pedantic renaming.

Do the renaming first in a different PR and let's have this PR focus on the functional change.

[danisharora099]

This is a very important change and the PR also contains a pedantic renaming.

Do the renaming first in a different PR and let's have this PR focus on the functional change.

Fair point. Moving to draft and will rebase once #1703 is merged.

[weboko]

lgtm

[fryorcraken]

Approved but the APIs could be improved and more thought made on the boundaries between the various APIs.

[fryorcraken]

Suggested change

	export interface SingleTopicShardInfo extends Omit<ShardInfo, "indexList"> {
	export interface SingleTopicShardInfo extends Omit<ShardInfo, "indexList"> {

Topic and Shard are redundant. It's a single shard info, which is also a single topic info.

The redundancy of information makes the interface name more complicated for no added value.

Also, you break the interface with ShardInfo here as you removed the index list.

Considering you only bring one property (cluster) over from ShardInfo, I am not sure to understand the point of extending from ShardInfo. Just create a new type.

Re-using the type is only useful if you want a function that accept ShardInfo to also accept SingleTopicShardInfo. But because of the Omit, you are not getting that.

If that's what you want, then try to override the indexList with a tuple type to specify that it's an array with only one number.

If you don't need interface compatibility between ShardInfo and SingleTopicShardInfo, then don't make the latter extend the former, just redefine a new interface.

[danisharora099]

I see your point that since the two interfaces don't share a lot of properties (have just one in common of the two), thus it might make more sense to define a new interface.

Omit was used because I wanted to showcase the link between the two types, while keeping one property. When is a case you think Omit would make sense for a usecase like this here? Perhaps when there are more number of properties?

--

Addressed redefining the interface here: 5dd508c

[fryorcraken]

IMO Omit makes sense when you want common API to be used. e.g.

interface A {
foo: number
bar: string
}

interface B extends <A, "foo"> {}

function func (alpha: Partial<A>) {
...
}

// this works
func(B)

I see your point re code documentation and duplication but with so very few properties, I think it makes it less readable.

[fryorcraken]

why would the index not be defined? It's not optional on SingleTopicShardInfo.

[danisharora099]

maybe this is being extra cautious & is easily swappable with a simple check of pubsubTopicShardInfo but this was just added as an extra check to ensure that if somebody force passes a string, or a structure that doesn't have index, we fallback to using the DefaultPubsubTopic
also something @weboko wanted here: #1697 (comment)

happy to follow up with a change if you prefer

[danisharora099]

actually updated this part of #1742

[fryorcraken]

Hopefully this goes away and the encoder does the automated conversion from content topic to pubsub topic cc @adklempner.

I think the encoder should do the conversion from shard info to pubsub topic too. The API should be reviewed.

[danisharora099]

I think the encoder should do the conversion from shard info to pubsub topic too. The API should be reviewed.

can you elaborate?

[fryorcraken]

my bad, API (createEncoder) is looking ok actually. did not realized it was an internal API (new Encoder).