Hmm, I wonder if we should wait until the server "acks" that the prekey has been published before calling this

I'm not sure exactly where this function will be called, but I suspect we should probably call unpublished_prekey (I might have that name wrong) instead. That way we are only grabbing the prekey if it has not already been published. Otherwise we will repeatedly send the same prekey to the server

I took this pattern from: https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/CryptoTools/CryptoModule.h#L51, where we already use camel case for prekey.

Otherwise we will repeatedly send the same prekey to the server

This function calls olm_account_generate_prekey at the beginning, which will generate new prekey each time it is called so each call to this method will result in new prekey being sent to keyserver.
I plan to call this method in the same place we send signed identity keys to the keyserver to pass notifications prekey to keyserver to initialize e2e session for notifications.

Ah we should fix that. In the Olm library we say eg. generate_prekey instead of generate_pre_key... so "prekey" is treated as one word

Ah okay, that makes sense if generate_prekey always generates a new prekey

From my last review:

Hmm, I wonder if we should wait until the server "acks" that the prekey has been published before calling this

I may be missing something but I am not sure about potential benefits of waiting to mark the prekey as published until keyserver send success response. We agreed on generating new notifications prekey everytime user logs in. Therefore even if the keyserver sends failure response, we will use new prekey when trying again to establish olm session for notifs.

However it may be that your concern is not about whether this method suits notifs case, but whether it suits general case. If that is the case, I would suggest that we add boolean flag as an argument that tells whether to mark the key as published immediately (set it to true in the notifs case) and introduce additional method for CryptoModule to mark the prekey as published (in case someone calls this method in a different context).

The boolean flag is an okay interim solution. I worry a little bit about how much we're focused on shipping the immediate project, versus building the long-term solution. By immediately marking the prekey as published, we are making it so it does not get returned when unpublished_prekey is called, which means it would not get published at all in the long-term solution (where we should only look at unpublished_prekey when determining which prekey to publish)

If you want to go with an "interim solution", instead of a boolean flag maybe we just rename this method to generatePrekeyAndMarkAsPublished, and create a follow-up task to address it later.

Thanks!
Now that I have got the whole picture I will go with the following approach:

1. In CryptoModule I will implement more fine-grained methods to get the full control over the prekey lifecycle that will be good long term.
2. In NotificationsCryptoModule I will keep the current solution, by having one bulk method to generate fresh prekey and mark it as published immediately.

I don't think this method should throw if there are no unpublished prekeys...

In the eventual usage, I imagine it will get called every time the client connects with the identity service, to check if there is anything to publish. If it returns "falsey" we will know not to publish anything; otherwise, if it returns "truthy" then we will have something to publish.

Can we instead return folly::Optional<std::string>?

Does ::olm_account_generate_prekey need the buffer to be filled with random bytes, or can it be zeros or unitialized?

Do we really need to "get" the prekey after generating? Does the buffer we pass to ::olm_account_generate_prekey get filled with the generated prekey?

I think folly::Optional<std::string> will be better.

It definitely needs random bytes. Two reasons:

1. Each place in CryptoModule that generates some type of key takes a buffer that was randomly initialized in the same way. So I believe original authors of CryptoModule did necessary research.
2. While olm is platform independent library, generating securely random bytes needs randomness provided by hardware so it is platform dependent. So olm cannot generate random bytes by itself but requires that we provide them ourselves based on some hardware random bytes generator.

Do we really need to "get" the prekey after generating?

It is convenient that this method returns already generated prekey. If a caller is only interested in generating prekey they will ignore returned value. But it they are indeed interested in the value of new prekey they won't have to issue a call twice.

Does the buffer we pass to ::olm_account_generate_prekey get filled with the generated prekey?

Each function in olm that generates new key stores it inside olm account. To extract the generated key we need to call another method. This buffer is to provide random bytes olm cannot generate itself and it is not filled with newly generated key.