Address Book encryption follow up tweaks

[LukasKorba]

• Merge strategy (This function mixes the merge with the rest of the store logic. I would factor the merge out, since that's a pure function. Maybe also add a test for the merge.)

• Merge strategy O(n*n)

• Error handling in UI (TODOs in the code)

• merge strategy enhancements

• Deletion of a contact and sync between local vs. remote

• redundancy in store of local contacts optimization

[daira]

Copying some comment threads.

Concerning the call to storeContacts from allLocalContacts: :

@daira:

I read your comment #1409 (comment) and I agree it's necessary to call syncContacts (with storeAfterSync: true), but given that change, I don't understand why we need to also call storeContacts. syncContacts should correctly handle the case where the remote file doesn't already exist.

@LukasKorba replied:

I agree it's redundant in some scenarios but syncContacts is done in a way that it could throw an error and terminate the store completely. The current code ensure there is at least local store done and sync "can" fail. We might want to improve the order and robustness of the solution but I put it to follow up work in #1408

@daira:

If we want syncContacts to always store locally, there's an easy way to do that: set cannotUpdateRemote = true in those error cases (i.e. treat them the same way as RemoteStorageError.containerURL), rather than propagating the error directly. If we want to record the reason then that can be done as well. Anyway this has been merged as-is now, but please consider that approach for #1408.

and in deleteContact: :

@daira:

Non-blocking but please document this on #1408:

If the remote store fails here but a previous store containing the deleted contact succeeded, then the remote file will still contain that contact, and so on the next sync, we'll restore it contrary to the user's intent.

I think the best way to fix this is probably for the deletion to fail if we can't store remotely. That would mean that we should try to delete the contact from the remote store first, so that there's no partial failure case where we delete it locally but then report failure.

[daira]

Also we should probably record the reason for failure (i.e. the exception object) in the .failure case of RemoteStoreResult.

[daira]

https://github.com/Electric-Coin-Company/zashi-ios/pull/1409/files#r1848385658 :

-                     if $0.lastUpdated > contact.lastUpdated {
+                     // If the timestamps are equal, the local entry takes priority.
+                     if $0.lastUpdated >= contact.lastUpdated {

This was an incorrect application of my suggestion. The change from > to >= depended on the change to merge from the local contacts into the remote contacts. Now the comment is wrong: >= here will cause the remote entry to take priority when the timestamps are equal.