Jenkins Builds

Let's not merge it until the release branch for 2.31 is cut in status-go, seems PR might be very risky

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.

Codecov Report

Attention: Patch coverage is 83.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 60.57%. Comparing base (768cda8) to head (103b415).
Report is 6 commits behind head on develop.

@@             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     

1. ContentTopicOverride and the custom argument to FiltersManager.LoadPublc look 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?

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 chatID, sym/asym encryption along with deriving content-topic and subscribing to it.
If we need a clean way to do it, i think it might lead to a massive refactor of the code (which i am not confident enough to do) and it would require significant effort and testing to ensure nothing else is broken.
It took a couple of weeks for me to understand all dependencies and various layers and abstractions to think of a simple way to make this change without impacting lot of code. I still don't understand a large part of it and its impact :)

If you can provide any specific suggestions as to how this change can be done more cleanly, i can implement the same.
I may be missing some simpler approach as i am not familiar with many parts of the code.

2. It is also very unobvious why we use MemberUpdateChannelID everywhere now.
Maybe it would be better to create something like this? At least a wrapper with explanation:

Thanks! will make this change.

@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 single-content-topic-usage and it will be set to no by default for all existing communities and yes to communities that are created after this change.
Note that the side-effect is users using old code will not be able to join/interact with these new communities.

The problem is there are too many layers to navigate in the code

@chaitanyaprem I'm worried that this PR makes it even more confusing 😄

... i think it might lead to a massive refactor of the code

I think we could take the opportunity of the new requirement and refactor this code, so that this change is not very harmful.

there was a suggestion that for any new communities that are created we can start using approach of single conent-topic straight

@chaitanyaprem @fryorcraken Sorry, I don't think this is a good idea.

1. It will confuse the code even more
2. So new communities won't work for old apps
3. Considering that there're not many new communities created everyday (how many do we have? ~5?), the effort is just not worth the benefit.

@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.

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.

1. It will confuse the code even more

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.
@kaichaosun @plopezlpz, is that something you can help with please?

2. So new communities won't work for old apps

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:

Waku chat and Status teams to consider UX around incompatible communities, a generic solution is likely to be enough regardless of the migration. e.g. “This community has been created with a new version for the app, please update to join”.

3. Considering that there're not many new communities created everyday (how many do we have? ~5?), the effort is just not worth the benefit.

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?