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
Details
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
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++.
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. |
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
---|---|---|
960 ↗ | (On Diff #14960) | I think we should use CONSTANT_CASE for constants in C++ |
- 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
- 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