[NO QA] stop generating ids with leading zeros#82038
Conversation
melvin-bot
Bot
requested review from
heyjennahay
and removed request for
a team
melvin-bot
Bot
requested review from
joekaufmanexpensify
and removed request for
a team
|
@marcochavezf Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
blimpich
changed the title
tylerkaraszewski
left a comment
tylerkaraszewski
left a comment
There was a problem hiding this comment.
shifting the range to 1–92233,
Does it not shift it to 1–92234, leaving the possibility that we are overflowing the maximum allowed?
Would it not be equally simple and more straightforward to just trim leading zeros at the end? I think that would be easier to follow.
But if not, I think we need to generate a random value up to CONST.MAX_64BIT_LEFT_PART - 1 before adding 1 at the end.
But I think it's much easier to explain why we strip leading zeros just before returning at the bottom of the function.
|
It won't ever get to But, I agree that stripping the leading zero is simpler and much easier to understand at a future date. |
|
Ah, I somehow skipped over that we were already adding 1 there, but just in a different place. I agree that this solution works then. |
|
Eh, I sort of take back my previous comment. It works in that it does not produce invalid numbers. It creates a weird gap in the available number space. The smallest number we intend to be able to create is 0, but with this change, the smallest number we can actually produce is 100,000,000,000,000, because we force a 1 into that particular digit. In the scheme of the amount of random number space this takes up, it doesn't really matter, but it's still sort of weird so I think I still prefer trimming 0s at the end. |
|
Updated to strip leading zeros 👍 |
|
|
||
| return left + middleString + rightString; | ||
| // Strip leading zeros by converting through BigInt | ||
| return BigInt(left + middleString + rightString).toString(); |
There was a problem hiding this comment.
I'm approving this because I assume BIgInt is a thing that we are allowed to use here, but if we have BigInt available I think this whole function can be a one liner or very close to it, because the whole reason behind splitting the number like this is the lack of real 64-bit numbers in JS.
There was a problem hiding this comment.
I believe BigInt should be fine here, but it doesn't give us an easy way to generate that random number still. We still need a way to generate the random number without overflowing. Is there anyone else we should bring this up with you think?
There was a problem hiding this comment.
This also requires crypto but I think would replace this entire function:
function randomInt64() {
const a = new BigUint64Array(1);
crypto.getRandomValues(a);
const positiveSignedOnly = a[0] >> 1n;
return BigInt.asIntN(64, positiveSignedOnly).toString();
}
There was a problem hiding this comment.
Good suggestion! Updated 👍
There was a problem hiding this comment.
We had the longest possible conversation about this in Spain when we first did it so even though it's a small change, I'd love to get a couple more eyes on it.
| const middleString = middle.toString().padStart(7, '0'); | ||
| const rightString = right.toString().padStart(7, '0'); | ||
| const buffer = new BigUint64Array(1); | ||
| crypto.getRandomValues(buffer); |
There was a problem hiding this comment.
Pretty certain this won't work out of the box in Hermes: facebook/hermes#915 (comment)
If you want to use the crypto API, I think https://github.com/margelo/react-native-quick-crypto is the best option, but it's an implementation of the Node.js crypto API, not the crypto Web API. So you'd need to use a sharded implementation:
web:
const buffer = new BigUint64Array(1);
crypto.getRandomValues(buffer);iOS/Android:
import crypto from 'react-native-quick-crypto';
const buffer = new BigUint64Array(1);
crypto.randomFillSync(buffer);Or find some other more portable implementation without the library 🤷🏼
There was a problem hiding this comment.
Yeah I don't think we should be making this simple utility into sharded implementation. Lets just go with the previous implementation but make it so that we don't have leading zeros
There was a problem hiding this comment.
Reverted it to original implementation. Only change is using BigInt to strip leading zeros
// Strip leading zeros by converting through BigInt
return BigInt(left + middleString + rightString).toString();That should work right?
There was a problem hiding this comment.
So that does work, but I still think we can simplify this if we have BigInt now.
The existing implementation uses Math.random() which is worse than crypto.getRandomValues(), but given that we're already using it, we can continue to use it here in a way that is simpler than what we have now.
What about this?
function rand64() {
// Generate two bigInts that are each 32-bit random numbers.
// We do 2 because Math.random() really only generates 53-bit numbers internally.
const hi32 = BigInt(Math.floor(Math.random() * 0x100000000));
const lo32 = BigInt(Math.floor(Math.random() * 0x100000000));
// Combine the two into a single 64-bit value.
const u64 = (hi32 << 32n) | lo32;
// Leave the top bit blank so these are not negative.
return (u64 >> 1n).toString();
}
There was a problem hiding this comment.
I still think we can simplify this if we have BigInt now
We do have BigInt in Hermes now (and I believe we didn't when this was first created)
blimpich
force-pushed
the
blimpich-fixLeadingZeroIDGeneration
branch
from
a57956b to
2160100
Compare
tylerkaraszewski
left a comment
tylerkaraszewski
left a comment
There was a problem hiding this comment.
I bet we can remove the constants for MAX_INT_FOR_RANDOM_7_DIGIT_VALUE etc as well.
tylerkaraszewski
left a comment
tylerkaraszewski
left a comment
There was a problem hiding this comment.
So we made this much more work than the original one line change was but I think the code is better for it.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari
MacOS: Desktop |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.19-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.19-5 🚀
|
4 participants
Explanation of Change
Prevents us from creating ids with leading zeros.
Why? The old code
Math.floor(Math.random() * (CONST.MAX_64BIT_LEFT_PART + 1))generated the left part as a random integer from 0 to 92233, so about 1 in 92,234 times left would be 0. When left = 0 and it gets concatenated with the middle and right strings (e.g., 0 + "3446715" + "2727472"), JavaScript coerces the number 0 to the string "0", producing an ID like "034467152727472" with a leading zero. The new code generates a random 64 bit number using crypto.randomValues instead, and makes sure it doesn't have leading zeros by casting it to BigInt and then back to a string.Slack discussion here: https://expensify.slack.com/archives/C03TQ48KC/p1770751691775419
Fixes Issues
$ https://github.com/Expensify/Expensify/issues/597251
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari