New method to validate and get content and notif prekeys.
Details
- Reviewers
ashoat marcin kamil - Commits
- rCOMMba74eb9d5757: [native] validateAndGetPrekeys method
called the method multiple times from js and confirmed the prekey values did not change
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Leaving only comments to let others review as well
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
---|---|---|
665 ↗ | (On Diff #36568) | when we have only two elements I think it's better to use std::pair:
But I feel like like those are more personal preferences so if you have a different opinion std::tuple also makes sense to me |
794 ↗ | (On Diff #36568) | in this case, you have to also call this->persistCryptoModule(); |
native/schema/CommCoreModuleSchema.js | ||
88 ↗ | (On Diff #36568) | I think we could deprecate generateAndGetPrekeys to avoid misuse in the future |
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
---|---|---|
665 ↗ | (On Diff #36568) | i can refactor to use pair, i don't have a strong preference |
794 ↗ | (On Diff #36568) | thanks, didn't realize this modifies the account |
native/schema/CommCoreModuleSchema.js | ||
88 ↗ | (On Diff #36568) | sure i'll introduce a diff after this to deprecate |
I think we could deprecate generateAndGetPrekeys to avoid misuse in the future
Yeah, it looks like its only callsite can be replaced with the new validateAndUploadPrekeys.
It also looks like CryptoModule::generateAndGetPrekey can be deprecated.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
---|---|---|
780 ↗ | (On Diff #36595) | If this the right condition? In validateAndUploadPrekeys, we appear to check has_value. |
791 ↗ | (On Diff #36595) | Nit: I would add another newline here, since the contentPrekeySignature is more associated with the contentPrekey than the other two variables |
It also looks like CryptoModule::generateAndGetPrekey can be deprecated.
We actually can't deprecate it since we use it in CryptoModule::validatePrekey, but we can move it out of the public interface. We also don't need NotificationsCryptoModule::generateAndGetNotificationsPrekey anymore
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
---|---|---|
780 ↗ | (On Diff #36595) | Yes, according to cppreference:
This also explicitly says that explicit operator bool() const and bool has_value() const are synonymous: https://en.cppreference.com/w/cpp/utility/optional/operator_bool |
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
---|---|---|
692 ↗ | (On Diff #36601) | I think this code is wrong for the below reason too. @marcin, can you confirm? |
786 ↗ | (On Diff #36601) | I think we need to persistCryptoModule() in this case too. Even if validatePrekey() returns false, it can still call forgetOldPrekey(), so we always need to call persistCryptoModule() after calling validatePrekey() Maybe we should just extract the persistCryptoModule calls into a single one on line 791? |
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
---|---|---|
692 ↗ | (On Diff #36601) | I think you ar right. We should call persistCryptoAccount() after validation without any condition. |
call persistCryptoModule unconditionally since validatePrekey can call forgetOldPrekey, which modifies the olm account
The code is technically correct but I think it needs some adjustments so that it conforms to our C++ conventions.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
---|---|---|
703 ↗ | (On Diff #36760) | I've just realized that there is another bug in this function. Since you've already modified this function, could you put those lines here? if(this->cryptoModule == nullptr) { this->jsInvoker_->invokeAsync([=, &innerRt]() { promise->reject("user has not been initialized"); }); return; } Basically in this function we don't check if cryptoModule has been initialized and we should early reject and return in case it hasn't been. |
788 ↗ | (On Diff #36760) | There is too much indentation here. In this line I think you should reject the promise and return early. Then you could remove else wrapping around the try ... catch. |
792 ↗ | (On Diff #36760) | Technically correct, but we don't use this style of if statements in the codebase. I think you should just assign contentPrekey = this->cryptoModule->validatePrekey() right away and then conditionally perform other assignments if contentPrekey doesn't have value. |
Agree with @marcin's comments about C++ conventions. Please make sure to adjust those before landing, and to add the this->cryptoModule == nullptr check he requested as well