Page MenuHomePhabricator

[native] Add a function for creating unique device ID on mobile in cpp/jsi
AbandonedPublic

Authored by inka on Jul 25 2022, 3:23 AM.
Tags
None
Referenced Files
F3352651: D4616.diff
Sat, Nov 23, 6:48 AM
Unknown Object (File)
Fri, Nov 15, 4:28 PM
Unknown Object (File)
Thu, Nov 14, 8:45 PM
Unknown Object (File)
Thu, Nov 14, 6:13 PM
Unknown Object (File)
Thu, Nov 14, 6:06 PM
Unknown Object (File)
Thu, Nov 14, 5:59 PM
Unknown Object (File)
Thu, Nov 14, 5:39 PM
Unknown Object (File)
Thu, Nov 14, 5:31 PM

Details

Summary

Add a function for creating unique device ID on mobile in cpp/jsi
A separate function in cpp might not be necessary, but we don't know yet where the function will be called, so it has been written to give us flexibility.
Linear issue: https://linear.app/comm/issue/ENG-1279/initialize-deviceid-on-clients

Test Plan

Log to the console the functions output given 'KEYSERVER', 'WEB', 'MOBILE' and some other random
string. See that in the first three cases it produces an ID compatible with what was described in
the linear issue comment, and that in the third case the app throws an error - as we cannot proceed
without a device ID (assuming the tunnelbroker is being used when this function gets called).

Diff Detail

Repository
rCOMM Comm
Branch
inka/userID_cpp
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Jul 25 2022, 3:36 AM

Fix the return value to match the specification

Can you remind me why this is necessary to do in C++? I'm sure there is a good reason, I just don't remember reading the reasoning. (It can be good to include this info in the diff description, or maybe a link to this info if it was previously covered on a diff or Linear.)

Probably the answer is from my line in the Linear issue:

On native it's important that the ID be accessible from outside of JS, so we probably shouldn't store it in Redux.

This seems like part of the story, but it's still possible for the ID to be generated inside JS but accessible outside of it. Why do we need to generate it from C++ instead of JS?

I talked to Tomek, and the reason for this decision was that we don't yet know, where the code will be called from. We thought we might want to call the function for example when: the user is logged, he updated the app and before he opens it again a notification comes. So it has been written in c++ to give us flexibility. Is this a problem?

That's reasonable, thanks for explaining @inka! The reason I asked is that it's generally better to implement in JS than in a low-level language like C++... JS is more simple, and more people on the team know JS. That said, in this case I can understand the reason for using C++.

Looks ok to me (with a small comment), passing to @tomek.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
973

It's not obvious what string is wrong, let's make this message more informative. And you forget to change the function name from the getDeviceID.

This revision is now accepted and ready to land.Jul 25 2022, 12:52 PM
This revision now requires review to proceed.Jul 25 2022, 12:53 PM

Change how the ID is generated

Change the error message

Correct the error message

tomek requested changes to this revision.Jul 26 2022, 3:00 AM
tomek added inline comments.
native/cpp/CommonCpp/CryptoTools/Tools.cpp
33–35 ↗(On Diff #14918)

This method has the same issue as D4587 - the generated string isn't uniformly distributed. It shouldn't be a big issue - but let's see what's the result of a discussion from mentioned diff.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
974–975 ↗(On Diff #14918)

The formatting is strange - can we put "KEYSERVER, WEB, " and "MOBILE." on the same line?

977 ↗(On Diff #14918)

It is a good idea to define 64 as a constant somewhere.

This revision now requires changes to proceed.Jul 26 2022, 3:00 AM

Cleanup code - create a constant for device ID length

inka retitled this revision from Add a function for creating unique device ID on mobile in cpp/jsi to [native] Add a function for creating unique device ID on mobile in cpp/jsi.Jul 26 2022, 5:09 AM
tomek added inline comments.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
960 ↗(On Diff #14960)

I think we should use CONSTANT_CASE for constants in C++

This revision is now accepted and ready to land.Jul 26 2022, 5:53 AM

Change constants name to match convention

ashoat requested changes to this revision.Jul 26 2022, 11:21 AM
  1. Since we are duplicating logic across JS (D4587) and C++ (here), I think we should have some comment on the functions pointing to the other one, and warning the reader that the two functions must be kept in sync
  2. Would like to see if we can find a way to generate a uniform distribution. We could consider writing this in Rust so we can use Rust crates (like NPM packages) for that
This revision now requires changes to proceed.Jul 26 2022, 11:21 AM