Page MenuHomePhabricator

[CommCoreModule] implement creating outbound `olm` session
ClosedPublic

Authored by kamil on Dec 18 2023, 4:36 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 24, 7:25 AM
Unknown Object (File)
Fri, Oct 18, 1:09 AM
Unknown Object (File)
Fri, Oct 18, 1:09 AM
Unknown Object (File)
Fri, Oct 18, 1:09 AM
Unknown Object (File)
Fri, Oct 18, 1:09 AM
Unknown Object (File)
Fri, Oct 18, 1:08 AM
Unknown Object (File)
Thu, Oct 17, 11:00 AM
Unknown Object (File)
Sun, Oct 13, 11:59 AM
Subscribers

Details

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 held this revision as a draft.
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

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

This revision is now accepted and ready to land.Jan 2 2024, 5:33 AM