It isn't ideal but I don't think there is a better place for this migration.

This is defined in CommCoreModule: https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/NativeModules/CommCoreModule.h#L27. The CommCoreModule uses this constant to store pickling key for notifs and content account. Ideally this constant should be removed from CommCoreModule and we should keep picling keys for accounts in SQLite in the same table where we keep olm accounts as we do on the keyserver. The fact that we keep picling keys for both accounts in Secure Store is an artifact since code that creates and persists crypto account in SQLite was implemented before we introduced SQLite encryption so it made sense. But now we need to migrate this pickling key to SQLite. I will create follow up task for this.

This implementation addresses the issue.

When we call clearSensitiveData then NOTIFS.PICKLING_KEY is removed from MMKV. When we call flushState we call getPicklingKey(false). Therefore if clearSensitiveData is called after statefulDecrypt and before flushState then flushState will fail since there is no pickling key in MMKV. This solves the race condition described in the issue linked above.

I think @inka is currently working on making this configurable on dev envs, so we probably can't just hardcode it.

I had a discussion with @tomek and was told that a good practice with key-value stores is to have a schema for keys in which the key begins with most general and ends with the most specific information. Therefore we begin with KEYSERVER. meaning that this key holds a value for some keyserver, then we have the keyserver id and finally we have information what kind of data we store. Similar scheme is adopted for unread count: KEYSERVER. + <id> + .UNREAD_COUNT.

We can't just hardcode this. It needs to be configurable, as @michal mentioned

(In particular, it should configurable via the same mechanisms that @inka is using on native... we should avoid the need for additional configuration)

In cases of other errors, we're returning null, e.g. when Malformed notification JSON payload.. Why do we handle this case differently?

I agree - throwing isn't particularly meaningful here. On the other hand logging and silently returning (as we do in case of Malformed notification JSON payload ) also isn't our best option since notification without keyserverID is more likely to occur than JSONException since it is new functionality that we introduce. I will display error message notification for staff users (we have done this already multiple times) and log + silent return for public users. @tomek tell me what you think.

This constant is used only when someone currently logged in updates/re-builds the app. Then session from old storage is transferred to MMKV and the key is created from value 256. That said it is correct to hardcode for production. It is also correct to hardcode for dev env if some one is logged to "old" keyserver. It will only fail if we are currently logged to "new" keyserver (launched according to @inka doc) and rebuild the app after landing my changes. That said I am wondering how bad is it to ask developers to log out before building the app with my changes and then log in after rebuilding the app? In such a case new session will be created with key created from actual keyserver id and this constant will never be used. The only alternative that I can think of is XCode build phase/build.gradle task that will read the content of @inka JSON and replace this constant with the value from the JSON. I am not sure if it is worth the hassle.
cc @ashoat @michal @inka what do you think?

I will display error message notification for staff users (we have done this already multiple times) and log + silent return for public users.

I think this is a good approach

My opinion is that since this is a one time migration that works for production and for master, we shouldn't spend time on using the id from my config files (since I'm told it would take more than one day, so quite a lot of time).
If someone is using the real keyserver id already, they have to have a patched diff (D11141). This is basically trying to avoid breaking someones local changes, which I don't think we tend to do.
Especially that all they will need to do is log out, which probably isn't a big problem.

Thanks for explaining. Only one concern:

This constant is used only when someone currently logged in updates/re-builds the app.

How do we know that will be the case in the future? Introducing a constant like this is risky. Given how important it appears that this constant is not used elsewhere, it seems like you should rename it something like ashoatKeyserverIDUsedOnlyForMigrationFromLegacyNotifStorage, and add a lengthy comment above it warning against its use.