Is this $ObjMap basically just $Shape? If not, what's different?

It seems like there's a risk of two of these running simultaneously.

To address this, I think we'll likely need to move this to a context, and set a ref before entering the async block that would prevent the second call from entering its async block.

Separately, we should ask if the same risk occurs in native, and address it if so

This affect does nothing. You're just declaring a function that is never executed

*effect

It is not. Applying $Shape to CompleteCryptoStore type results in the following type:

export type CompleteCryptoStore = {
  +primaryAccount?: PickledOLMAccount,
  +primaryIdentityKeys?: OLMIdentityKeys,
  +notificationAccount?: PickledOLMAccount,
  +notificationIdentityKeys?: OLMIdentityKeys,
};

Which is different to the original CryptoStore definition. In other words it makes fields optional but what we want it to keep fields required but make them of optional type. Applying $Shape results in many flow errors in some files (redux-setup and crypto-store-reducer).

However I understand the concern since this construct does not look obvious at the first glance. We might consider creating a follow-up diff that introduces some utility that can apply the operation above to any type we want but has more meaningful name.

It seems like there's a risk of two of these running simultaneously.

To address this, I think we'll likely need to move this to a context, and set a ref before entering the async block that would prevent the second call from entering its async block.

I will try to introduce this approach.

Separately, we should ask if the same risk occurs in native, and address it if so

We don't have any similar hook on native since this data are not kept in redux there. We have a JSI call implemented in C++ that idempotently creates olm data and persists them in SQLite. Then there is another JSI call to fetch those data in SQLite.

You are right - I need to add (..)();. The fact that test plan worked is because this differential also introduces healing the state using socket requests.

I recently was looking around at the equivalent code in native while I was investigating while the notif issue in ENG-5354 was also crashing the app on start.

Regarding the idempotent JSI call that @marcin mentioned, I believe this is initializeCryptoModule. I observed it getting called a lot when the iOS app started. I wonder if adding a caching layer to skip the SQL query would be wise. We would just need to make sure we invalidate this cache appropriately when eg. the user logs out or deletes their account.

We should have a single action to set entire CryptoStore at once.

On the other hand not unsetting this promise will result in not creating new CryptoStore when the user logs-out and CryptoStore is removed but returning old CryptoStore that was created with cached promise. That is since when the user logs out we will see that CryptoStore became nullish, we will not do early return and check for already existing promise. We will see that the promise is there so we will just return it and never create new cryptoStore.

That said I am starting to doubt the approach where we have a hook that returns CryptoStore or creates and returns it if it wasn't already there (hence working somehow like RAII). I am wondering if we could adopt much simpler approach where the context returns CompleteCryptoStore instead of () => Promise<CompleteCryptoStore> and the provider creates necessary data in useEffect`.

Actually this is what already happens on web. LoginForm performs exactly same action. The disadvantage of the code currently on master is that we don't handel the case of the user that hasa never logged-out and (most importantly) we are forced to tread data in CryptoStore as optional types which I found inconvenient in initial stages of this work. That said changing what the context returns will keep as on pair with current functionality.

1. Is this line necessary?

I think so - we can't guarantee that getOrCreateCryptoStore will call await initOlm.

2. If it's necessary, why are you calling it "serially"? There is no need to wait on getOrCreateCryptoStore before calling this. I think I've given you this feedback before... I'd appreciate it if you could integrate it into your understanding, and make sure I never have to bring it up again.

I will change it to use Promise.all()

I have to guarantee that GetOrCreateCryptoStoreContext has set value before Socket is created since Socket requires this context. Putting it anywhere in web/app.react.js will result in an error.

I'm open to the idea of initializing in an effect, but keep in mind that the context will not always be able to return CompleteCryptoStore if the promise has not been resolved yet. As such, we will still need an interface on the context that returns Promise<CompleteCryptoStore>

I think so - we can't guarantee that getOrCreateCryptoStore will call await initOlm.

Really? It seems guaranteed to me...

Thanks for explaining

Without async keyword this function caused flow error since it considered its type to be CryptoStore | Promise<CryptoStore>. This is why I put async keyword in front of this function.

It is not guaranteed. If getOrCreateCryptoStore returned early it wouldn't call await initOlm();.

First I tried to set this value using useEffect. However I found that useEffect doesn't get executed before other components try to access the context which results in an error. That is why I decided to opt for useMemo since it is guaranteed to be executed before this component returns. However there is a major drawback pointed by @ashoat here: https://phab.comm.dev/D9532?id=32316#inline-60292. I don't think there is a good way to handle exception in useMemo since function in useMemo shouldn't have side effects. That said this approach is not perfectly textbook. Theoretically we could try{} .. catch{} and return something falsy. However this undermines the entire purpose of this differential which is to enable other components to acquire CryptStore data without considering it optional.

If this solution is not acceptable we might consider abandoning this differential. It is not obligatory for e2e web notifications to work. Without this differential we would have to treat CryptoStore optional and instantiate session with the keyserver only if it is there.

I think this can be simplified