[CommCoreModule] implement creating outbound `olm` session
ClosedPublic
Actions

Authored by kamil on Dec 18 2023, 4:36 AM.

Details

Reviewers

marcin
varun
michal

Commits

rCOMMf5ef9cf62d6b: [CommCoreModule] implement creating outbound `olm` session

Summary

Implemented method to creating olm session.

Depends on D10374

Test Plan

This code is really hard to test as it's impossible to verify partial results.
I generated some keys and values, verified them with olm-utils.test.js, and checked if they succeeded/failed when calling the method introduced here in the same cases as JS tests.
Full functionality tested later in the stack.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Dec 18 2023, 4:36 AM

kamil held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 18 2023, 4:36 AM

kamil added a child revision: D10376: [CommCoreModule] implement creating inbound `olm` session.Dec 18 2023, 4:38 AM

Harbormaster completed remote builds in B25174: Diff 34789.Dec 18 2023, 6:18 AM

kamil published this revision for review.Dec 18 2023, 6:50 AM

kamil added inline comments.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
753 ↗	(On Diff #34789)	this probably could be defined in a different place

marcin accepted this revision.Dec 21 2023, 3:01 AM

This revision is now accepted and ready to land.Dec 21 2023, 3:01 AM

marcin requested changes to this revision.Dec 21 2023, 3:19 AM

marcin added inline comments.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
756 ↗	(On Diff #34789)	Looks like this call is is not blocking execution of the code below. What happens if `persistOlmAccount` fails and rejects the promise but we were able to resolve it to a correct value first? If the rejection after resolution is ignored then it is a bug and we want to avoid it. Simplest solution would be to block on successful persistence completion. Alternatively I am wondering if we could return two promises one for crypto and the other for persistence and make JS await them in parallel. However I might be wrong and the code above is actually correct. If you justify that there is no risk of an ignored error I will accept the revision.

This revision now requires changes to proceed.Dec 21 2023, 3:19 AM

kamil requested review of this revision.Jan 2 2024, 4:03 AM

kamil added inline comments.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
756 ↗	(On Diff #34789)	That could lead to issues, good catch. I updated D10371 to be blocking call and D10372 - when those two are accepted I'll rebase rest of the stack to avoid re-requesting review or all diffs