feat_: use a single content-topic for all community chats #5864
Conversation
chaitanyaprem
requested review from
richard-ramos,
kaichaosun and
plopezlpz
Jenkins BuildsClick to see older builds (69)
|
chaitanyaprem
force-pushed
the
feat/comm-content-topic-poc
branch
2 times, most recently
from
48283d7
to
4f10919
Compare
chaitanyaprem
force-pushed
the
feat/comm-content-topic-poc
branch
3 times, most recently
from
b6aaaf3
to
29b3c44
Compare
chaitanyaprem
changed the title
chaitanyaprem
force-pushed
the
feat/comm-content-topic-poc
branch
from
29b3c44
to
8cb3323
Compare
|
Let's not merge it until the release branch for 2.31 is cut in status-go, seems PR might be very risky |
sure, and agreed that this PR needs more testing and dogfooding before merging. |
chaitanyaprem
force-pushed
the
feat/comm-content-topic-poc
branch
2 times, most recently
from
cfec7d4
to
d8fa686
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5864 +/- ##
===========================================
- Coverage 60.57% 60.57% -0.01%
===========================================
Files 812 812
Lines 109328 109359 +31
===========================================
+ Hits 66230 66246 +16
- Misses 35337 35344 +7
- Partials 7761 7769 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
chaitanyaprem
force-pushed
the
feat/comm-content-topic-poc
branch
from
d8fa686
to
0eb01c3
Compare
protocol/transport/transport.go
Outdated
| @@ -258,6 +258,8 @@ func (t *Transport) RetrieveRawAll() (map[Filter][]*types.Message, error) { | |||
| } | |||
|
|
|||
| for i := range msgs { | |||
| // TODO: find the filter for the msg based on chatID in the message and map it properly. or better this is done in filter layer itself? | |||
| // something like t.FilterByChatID() | |||
There was a problem hiding this comment.
Better have an issue to list the todos and link it here if still needed.
There was a problem hiding this comment.
Ah, i am not sure if this is needed.
Will wait for review from status team to understand the impact and then either remove this or track it separately.
There was a problem hiding this comment.
Not sure I understand why we should find the filter for given message here? The filter is already there 🤔
There was a problem hiding this comment.
I was little doubtful as to how this will have an impact on any other flows.
I had noticed that messages received by a filter can be fetched using chatID. But the actual chatID of the message will be different than the one stored in filter (i.e <communityID>-memberUpdatesChannelID).
I was wondering if message retrieval flow will get affecting in some form if i don't use the actual chatID of the message. The caller of RetrieveAll has to retrieve messages using <communityID>-memberUpdatesChannelID as chatID and will not have flexibility to filter messages based on actual chatID.
Because the result of this function will now index all messages with single filter rather than chatID specific filters.
chaitanyaprem
force-pushed
the
feat/comm-content-topic-poc
branch
from
12451a8
to
d61d200
Compare
There was a problem hiding this comment.
Thank you, @chaitanyaprem!
Although I appreciate the minimum code changes for this, I think it made the code more complex.
-
ContentTopicOverrideand the custom argument toFiltersManager.LoadPublclook quite specific. I think we shouldn't do it this way and introduce a proper function that will just subscribe to a content topic and pubsub topic, isn't that possible? -
It is also very unobvious why we use
MemberUpdateChannelIDeverywhere now.
Maybe it would be better to create something like this? At least a wrapper with explanation:func (o *Community) UniversalContentTopic() string { // TODO: Explain here why we use MemberUpdateChannelID as the universal content topic return c.MemberUpdateChannelID() }
| TargetPeer string `json:"targetPeer"` | ||
| Ephemeral bool `json:"ephemeral"` | ||
| Priority *int `json:"priority"` | ||
| ContentTopicOverride string `json:"contentTopicOverride"` |
There was a problem hiding this comment.
This looks quite counter-intuitive. Why do we want some special ContentTopicOverride field, and not just use Topic?
There was a problem hiding this comment.
good point, let me think of why i did not do that. maybe it is possible without breaking anything.
but for some reference as to why i had to use a separate override field
There was a problem hiding this comment.
I had to do this as
status-go/protocol/messenger.go
Line 2272 in d61d200
status-go/protocol/common/message_sender.go
Line 668 in d61d200
Happy to follow any suggestions you have to get around this.
There was a problem hiding this comment.
Thanks, @chaitanyaprem. This looks good overall. I’m just wondering about the possibility and value of making it feel a bit less hacky. I’ve left some comments for you to check out.
protocol/messenger.go
Outdated
| @@ -2268,7 +2268,8 @@ func (m *Messenger) dispatchMessage(ctx context.Context, rawMessage common.RawMe | |||
| ) | |||
| return rawMessage, fmt.Errorf("can't post message type '%d' on chat '%s'", rawMessage.MessageType, chat.ID) | |||
| } | |||
|
|
|||
| //setting content-topic over-ride for community messages to use memberUpdatesChannelID | |||
| rawMessage.ContentTopicOverride = community.MemberUpdateChannelID() | |||
There was a problem hiding this comment.
Would it be possible to override rawMessage.LocalChatID directly here rather than using ContentTopicOverride?
There was a problem hiding this comment.
The reason chatID was not over-ridden is because the same method LoadPublic is used to create a filter that is not present and also load an existing filter.
In phase-1 we want to send using only universal-content-topic but we want to receive using both universal and chatID based content-topics to have compatibility.
If i override chatID then i was not sure if in some flow filter gets created via LoadPublic may get messed up. e.g in case of encrypted channels hashratched-groupID is derived from chatID
status-go/protocol/messenger.go
Line 2295 in d61d200
Not sure where else in the code downward chatID is used for some other purposes.
If you think it is safe to override, i will go ahead and change it.
protocol/transport/transport.go
Outdated
| @@ -258,6 +258,8 @@ func (t *Transport) RetrieveRawAll() (map[Filter][]*types.Message, error) { | |||
| } | |||
|
|
|||
| for i := range msgs { | |||
| // TODO: find the filter for the msg based on chatID in the message and map it properly. or better this is done in filter layer itself? | |||
| // something like t.FilterByChatID() | |||
There was a problem hiding this comment.
Not sure I understand why we should find the filter for given message here? The filter is already there 🤔
The problem is there are too many layers to navigate in the code and there is a separate function that will subscribe to content-topic and pubsubTopic already here. The filters are tied to many things in the code such as If you can provide any specific suggestions as to how this change can be done more cleanly, i can implement the same.
Thanks! will make this change. |
chaitanyaprem
force-pushed
the
feat/comm-content-topic-poc
branch
from
d61d200
to
b5315d5
Compare
|
@igor-sirotin , @osmaczko there was a suggestion from @fryorcraken that for any new communities that are created we can start using approach of single conent-topic straight on whereas for existing communities, we can do it in a 2 phase manner. In order to do that shall we create some sort of attribute fields in the community-description to indicate such feature changes for communities so that we can apply new features to new communities quicker. Explained more here WDYT? can we add an attributes field in the community description which for now will have just a single flag |
chaitanyaprem
force-pushed
the
feat/comm-content-topic-poc
branch
from
b5315d5
to
103b415
Compare
@chaitanyaprem I'm worried that this PR makes it even more confusing 😄
I think we could take the opportunity of the new requirement and refactor this code, so that this change is not very harmful. |
@chaitanyaprem @fryorcraken Sorry, I don't think this is a good idea.
|
Let me think and see if there is any other way to make this change without refactor. I want to try and avoid it if possible. |
From reading the comment here, I think there needs to be a clearer path on how to untangle status-go at each PR. I agree with some of the comments trying to making it less hacky, while at the same time I get Prem's input that he may not have knowledge and confidence here to do further refactor. If we compare it to the more usual nwaku development flow. Prem is a researcher implementing a PoC for a protocol change (content topic community management). Usually, the expectation is for codebase owners/engineers to help the researcher harden this PoC and proceed with neeeded refactors.
Let's move this conversation to https://forum.vac.dev/t/breaking-changes-and-roll-out-strategies/338/5 I am not convinced that Status Communities is in a state where the cost of "ensuring that newly created communities are compatible with older software version" outweigh the benefits. This specific change helps with Community scaling, by reducing the resources used by a single community user on store. Moreover, we could always have a toggle for an owner creating a new community. "experimental creation: includes changes that improves efficiency but it means your members need to have the same version or new than you to join". This is probably to discussion to have more generally within Status team here. Speed of development and improvement vs letting your users keep an old app version. As stated in https://forum.vac.dev/t/breaking-changes-and-roll-out-strategies/338 there is also a clean way to handle it:
This sounds paradoxal. you are suggesting that we should not break things for current users (joining communities) because at the end of the day there is not enough users (creating communities). It is one or the other? Do you have not enough activity or too much activity to make this change viable? |
|
Final point: by creating a new code path for this, which is "cleaner", then you have the opportunity to deleted the "messy" code path in the future. Yes, it would mean that people "have to upgrade", but what is the alternative? |
|
@fryorcraken sorry if I was unclear, I think the order of my arguments was wrong. My main argument is this: So I thought why do more work, if we keep current solution for supporting both old and new communities, and remove this code in 3-6 months. I have nothing against making a new code path, but it doesn't seem to be easy in this case. @chaitanyaprem we can try to help you to find a better solution with the filters to avoid the |
So iiuc for now we will stick with the 2 phase plan of implementing this rather than doing it for new vs existing communities.
Sure, will ping one of you. Thanks! |
This PR is first step i moving towards using a single content-topic for all community chats and most of the control messages.
Once this is released , in a subsequent release we can disable installing filters using chatIDs for communities altogether.
Detailed proposal https://forum.vac.dev/t/status-communities-review-and-proposed-usage-of-waku-content-topics/335
Refer to waku-org/pm#268 ->
Implementation Detailsfor the plan of this feature.All channels shall use content-topic used for MemberUpdates .
Important changes: