Page MenuHomePhabricator

[lib] Stop requiring sequential messages
ClosedPublic

Authored by kamil on Jul 15 2024, 5:04 AM.
Tags
None
Referenced Files
F2629656: D12749.id42870.diff
Sat, Sep 7, 1:42 PM
F2629654: D12749.id42865.diff
Sat, Sep 7, 1:41 PM
F2629649: D12749.id42872.diff
Sat, Sep 7, 1:41 PM
F2622811: D12749.id42289.diff
Fri, Sep 6, 8:39 PM
Unknown Object (File)
Fri, Sep 6, 8:20 AM
Unknown Object (File)
Sun, Sep 1, 8:35 PM
Unknown Object (File)
Sun, Sep 1, 10:55 AM
Unknown Object (File)
Sat, Aug 31, 4:54 AM
Subscribers

Details

Summary

ENG-8715.

This differential is doing two things:

  1. Renaming decryptSequentialAndPersist -> decryptAndPersist in OlmAPI to properly reflect what this code method is now doing.
  2. Using decrypt instead of decrypt_sequential from olm which is an actual change, stops requiring sequential messages.

This might cause some messages to arrive in an inverted order, and we're planning to handle this here: https://linear.app/comm/issue/ENG-8752/update-the-reducers-to-respect-properties-timestamps

NOTE: This change is not revertable (in logic meaning), in the future if we want to start requiring sequential messages we'll need to reset the olm session.
Test Plan

This is just swapping back to the decrypt method which is used elsewhere in the codebase and has the same params and return type so this should work out of the box. Under the hood, we only avoid one additional check (src).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 15 2024, 5:26 AM
Harbormaster failed remote builds in B30327: Diff 42289!
tomek requested review of this revision.Jul 16 2024, 5:30 AM
kamil requested changes to this revision.Jul 18 2024, 6:41 AM

I think this should be done at different level:

  1. Use decrypt here: https://github.com/CommE2E/comm/blob/8b786f1788c01edce880256d4f5318ce54342f29/native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp#L1619
  2. Rename decryptSequentialAndPersist -> decryptAndPersist

We still want to persist inbound messages as we can decrypt them only once

This revision now requires changes to proceed.Jul 18 2024, 6:41 AM
tomek edited reviewers, added: tomek; removed: kamil.
This revision now requires review to proceed.Jul 26 2024, 6:12 AM
kamil edited the test plan for this revision. (Show Details)
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1627–1628 ↗(On Diff #42865)

Stop requiring sequential messages on native

web/shared-worker/worker/worker-crypto.js
529 ↗(On Diff #42865)

Stop requiring sequential messages on web

This revision is now accepted and ready to land.Jul 29 2024, 3:55 AM
This revision was automatically updated to reflect the committed changes.