Same feedback as on iOS diffs – we should merge these so we only have to do a single pass

This is an artifact from debugging. I forgot to set it to FUTURE_CODE_VERSION.

Is it guaranteed that device tokes match notifications?

It is guaranted since:

1. Function prepareEncryptedIOSNotifications returns notifications that are encrypted in the order of cookieIDs array that is passed to this function.
2. Array cookieIDs matches device tokens since it is constructed by iterating over rows from the db. Each row that we iterate contains one cookieID and related device token.

Can we make the API safer by merging these into one object with deviceToken and notification fields? (and add an array of these as a parameter)

I think we can

Artifact from debugging. I forgot to set it to FUTURE_CODE_VERSION.

Is it a good idea to encrypt both of these?

We must encrypt both since it is the only way to get to know if adding messageInfos does not overflow fcm push limit.

Is the session state affected by that?

It is but not in a dangerous way. Only one of those will be sent to client, and the other can be viewed as skipped messages. The client can handle skipped messages by advancing its ratchet keys. Every ratchet-encrypted message contains a header with the counter, so that the recipent can advance their keys if they received messages no. 1, 2 and 5 but didn't receive message 3 and 4. It can therefore decrypt 5 when it arrives and 3, 4 in case they happen to arrive later.

Sure - I will replace it with $ReadOnlyArray. Mutability is not necessary here.

We're iterating over cookies but not use them. Is it a mistake? We're returning the same notif multiple times.

Looking at changes made in this differential it can be concluded that this is intentional. It was introduced in order to meet requirements of new typing. New typing here is that send{APNs/Android}Notification requires the array of pairs of deviceToken and notification to ensure that each encrypted notification is delivered to the particular deviceToken associated with cookieID used to encrypt notif (olm session related to this cookieID, to be precise). Therefore in order to meet this typing we have to repeat the same notification for each deviceToken in case notification is not encrypted.

I think that design would be more readable (less weird lines like the one @tomek pointed out), and less brittle (if a future developer changes something, they are less likely to break an assumption).

I agree that this code might be of better quality and change suggested by @ashoat should be made.

However the fact that this snippet of code was enough to the reviewer to request changes is concerning for me. Looking at this differential holistically was enough to figure out that it is not a mistake, but rather a consequence of types we agreed on. Additionally, it is worth to note that this differential already had many cycles during which we detected and addressed much more serious issues. It could be a flag for the reviewer that it is unlikely there is a mistake here.

The fact that code quality is not perfect here could well be handled by a simple comment with suggestion on how to beautify this code. This team has a strong culture of not landing a diff with unaddressed comments. We should be taking advantage of this good convention to speed-up the process of delivering features where we know the code was tested and works correctly but there are some minor issues left about code quality.

@marcin, what do you think? If we make this change, we should make it for both iOS and Android, assuming it applies for both.

I agree we shouldn't leave discrepancy between platforms and apply new approach to both. That said I will introduce those changes to iOS diff. However I would pursue landing the entire stack, apart from this diff, once I am back to work on Monday in order not to block entire goal, other people who might be interested in some parts of this stack and my future work regarding e2e rescinds and large notif download.

I think I should add, that I expressed my concerns before I noticed there has been a change in reviewers and believed the stack will be blocked until June.