-
Notifications
You must be signed in to change notification settings - Fork 42
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!: add support for sharded pubsub topics & remove support for named pubsub topics #1697
Changes from 14 commits
5d59a0b
6ae6fd6
a7d7673
9d3e34b
9c88102
b6ea40a
85a786a
a0de535
973ab4f
ab4831c
fe81a26
7fd07b2
85500d3
0579376
f42d848
5dd508c
d10eb95
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,11 @@ import type { | |
IMetaSetter, | ||
IProtoMessage, | ||
IRateLimitProof, | ||
PubsubTopic | ||
PubsubTopic, | ||
SingleTopicShardInfo | ||
} from "@waku/interfaces"; | ||
import { proto_message as proto } from "@waku/proto"; | ||
import { Logger } from "@waku/utils"; | ||
import { Logger, singleTopicShardInfoToPubsubTopic } from "@waku/utils"; | ||
|
||
import { DefaultPubsubTopic } from "../constants.js"; | ||
|
||
|
@@ -119,12 +120,19 @@ export class Encoder implements IEncoder { | |
* messages. | ||
*/ | ||
export function createEncoder({ | ||
pubsubTopic = DefaultPubsubTopic, | ||
pubsubTopicShardInfo, | ||
contentTopic, | ||
ephemeral, | ||
metaSetter | ||
}: EncoderOptions): Encoder { | ||
return new Encoder(contentTopic, ephemeral, pubsubTopic, metaSetter); | ||
return new Encoder( | ||
contentTopic, | ||
ephemeral, | ||
pubsubTopicShardInfo?.index | ||
? singleTopicShardInfoToPubsubTopic(pubsubTopicShardInfo) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
can you elaborate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my bad, API ( |
||
: DefaultPubsubTopic, | ||
metaSetter | ||
); | ||
} | ||
|
||
export class Decoder implements IDecoder<DecodedMessage> { | ||
|
@@ -182,7 +190,12 @@ export class Decoder implements IDecoder<DecodedMessage> { | |
*/ | ||
export function createDecoder( | ||
contentTopic: string, | ||
pubsubTopic: PubsubTopic = DefaultPubsubTopic | ||
pubsubTopicShardInfo?: SingleTopicShardInfo | ||
): Decoder { | ||
return new Decoder(pubsubTopic, contentTopic); | ||
return new Decoder( | ||
pubsubTopicShardInfo?.index | ||
? singleTopicShardInfoToPubsubTopic(pubsubTopicShardInfo) | ||
: DefaultPubsubTopic, | ||
contentTopic | ||
); | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,5 +1,10 @@ | ||||||
import { ShardInfo } from "./enr.js"; | ||||||
import type { PubsubTopic } from "./misc.js"; | ||||||
|
||||||
export interface SingleTopicShardInfo extends Omit<ShardInfo, "indexList"> { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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 Considering you only bring one property ( Re-using the type is only useful if you want a function that accept If that's what you want, then try to override the If you don't need interface compatibility between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
-- Addressed redefining the interface here: 5dd508c There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO
I see your point re code documentation and duplication but with so very few properties, I think it makes it less readable. |
||||||
index: number; | ||||||
} | ||||||
|
||||||
export interface IRateLimitProof { | ||||||
proof: Uint8Array; | ||||||
merkleRoot: Uint8Array; | ||||||
|
@@ -38,7 +43,7 @@ export interface IMetaSetter { | |||||
} | ||||||
|
||||||
export interface EncoderOptions { | ||||||
pubsubTopic?: PubsubTopic; | ||||||
pubsubTopicShardInfo?: SingleTopicShardInfo; | ||||||
/** The content topic to set on outgoing messages. */ | ||||||
contentTopic: string; | ||||||
/** | ||||||
|
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 would the index not be defined? It's not optional on
SingleTopicShardInfo
.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.
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 haveindex
, we fallback to using theDefaultPubsubTopic
also something @weboko wanted here: #1697 (comment)
happy to follow up with a change if you prefer
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.
actually updated this part of #1742