fix: prevent IEEE 754 precision loss for large acquirer BIN values#4939
fix: prevent IEEE 754 precision loss for large acquirer BIN values#4939itsaryanchauhan wants to merge 1 commit into
Conversation
XyneSpaces
left a comment
There was a problem hiding this comment.
Review Summary
Classification: Bugfix — IEEE 754 Precision Loss Fix
Risk Level: Low (localized form input fix)
Files Changed: 1 file (MerchantAcquirerDetailsUtils.res)
Tier 1 — High Signal Issues
No critical issues found.
Tier 2 — Medium Signal Observations
The validateForm function was simplified to use valuesDict->getString(key, "")->isNonEmptyString for all required fields including AcquirerBin. Previously, AcquirerBin had special handling using getOptionFloat to validate presence.
Fix: Verify that the empty-string check is sufficient for AcquirerBin validation. If the form initializes with undefined or null values that render as empty strings, the validation still works. If there's any edge case where the field might be missing entirely from the dict, consider keeping explicit key existence checking.
Tier 3 — Minor Nits
🔍 Regex replacement on every keystroke
let filteredValue = value->String.replaceRegExp(%re("/[^0-9]/g"), "")The regex runs on every character input. While negligible for this use case, consider memoizing the regex or using a simple character-filter function for consistency with the codebase pattern.
Fix (optional):
let filteredValue = value->String.split("")->Array.filter(c => c >= "0" && c <= "9")->Array.join("")Positive Notes
- Correct identification of IEEE 754 precision loss as the root cause
- Proper use of string-based storage for arbitrary-length numeric identifiers
- Appropriate max length validation (4-20 digits)
- Clean removal of
AcquirerBinfromnormalizeNumericStringFields - The fix aligns with the pattern used for other text fields in the same form
Pre-existing Issues (not introduced by this PR)
🔍 AcquirerIca still uses numericTextInput but may face same precision issue
The icaField still uses numericTextInput which converts input to float. If ICA values can exceed 15-16 digits, they will suffer the same precision loss. Consider applying the same string-input fix to icaField if the backend expects exact digit preservation.
Verdict: ✅ Approve — Clean fix for a subtle but impactful bug.
XyneSpaces
left a comment
There was a problem hiding this comment.
The validation change from getOptionFloat to getString for AcquirerBin assumes the value will always be a string in the form. Verify that:
- Form initialization doesn't leave the field absent from the dict
- Backend submission still receives the value as a string, not a number
This change is likely safe given the text input, but confirm the form state initialization sets an empty string default for acquirer_bin.
| @@ -137,23 +146,18 @@ let validateForm = (~requiredKeys: array<acquirerField>, values: JSON.t): JSON.t | |||
|
|
|||
| requiredKeys->Array.forEach(field => { | |||
There was a problem hiding this comment.
The validation change from getOptionFloat to getString for AcquirerBin assumes the value will always be a string in the form. Verify that:
- Form initialization doesn't leave the field absent from the dict
- Backend submission still receives the value as a string, not a number
This change is likely safe given the text input, but confirm the form state initialization sets an empty string default for acquirer_bin.
| let handleChange = event => { | ||
| let value = ReactEvent.Form.target(event)["value"] | ||
| let filteredValue = value->String.replaceRegExp(%re("/[^0-9]/g"), "") | ||
| input.onChange(filteredValue->Identity.stringToFormReactEvent) |
There was a problem hiding this comment.
🔍 Regex runs on every keystroke — consider optimization
While the %re("/[^0-9]/g") regex is compiled at build time, it still executes on every change event. For consistency with the codebase's functional style, consider:
let filteredValue = value->String.split("")->Array.filter(c => c >= "0" && c <= "9")->Array.join("")Or if regex is preferred, lift it to module scope to avoid reallocation.
Detailed Review FindingsOverviewClean fix for IEEE 754 precision loss in the Acquirer BIN field. The root cause analysis is accurate, and the solution properly addresses the issue. Key Changes Reviewed1. Input Component Replacement (Lines 36-45)
2. Normalization Cleanup (Line 132)
3. Validation Updates (Lines 149-156)
Pre-existing Issue NotedThe VerdictApprove — Well-executed bugfix with appropriate test coverage noted in PR description. |
XyneSpaces
left a comment
There was a problem hiding this comment.
Review Summary
Classification: Bugfix — IEEE 754 precision loss prevention for Acquirer BIN
Risk Level: Low-Medium
Files Changed: 1
Findings Overview
| Severity | Count | Issue |
|---|---|---|
| 1 | Edge case allows empty BIN submission after filtering | |
| 💡 | 1 | Identity conversion obscures intent |
| 🔍 | 1 | No user feedback on filtered characters |
Verdict: One blocking issue (empty string edge case) needs addressing before merge.
| let handleChange = event => { | ||
| let value = ReactEvent.Form.target(event)["value"] | ||
| let filteredValue = value->String.replaceRegExp(%re("/[^0-9]/g"), "") | ||
| input.onChange(filteredValue->Identity.stringToFormReactEvent) |
There was a problem hiding this comment.
If the user types only non-digit characters (e.g., "abc"), the regex filter strips them all, resulting in an empty string. This passes both validation checks:
requiredKeyscheck passes because""is technically a present value in the dict- BIN length check is skipped because
isNonEmptyStringreturns false
The form would submit with an empty BIN despite the field being marked as required.
Fix: Either validate the filtered result is non-empty before setting, or ensure empty string after filtering fails the required check:
let filteredValue = value->String.replaceRegExp(%re("/[^0-9]/g"), "")
if filteredValue->isNonEmptyString || value->isEmptyString {
// Only update if we have digits OR the original was already empty
input.onChange(filteredValue->Identity.stringToFormReactEvent)
}Or move the BIN length validation outside the empty check in validateForm.
| input.onChange(filteredValue->Identity.stringToFormReactEvent) | ||
| } | ||
| <TextInputAdapter | ||
| input={...input, onChange: handleChange} placeholder autoComplete="off" maxLength=20 |
There was a problem hiding this comment.
💡 Identity conversion obscures intent
The ->Identity.stringToFormReactEvent conversion wraps a string in a fake event structure just to satisfy the type system. If input.onChange accepts the raw string in other contexts, consider passing filteredValue directly for clarity.
If the type system strictly requires an event-shaped object, add a brief inline comment explaining why this conversion is necessary.
| } | ||
|
|
||
| valuesDict | ||
| ->getOptionFloat((AcquirerFraudRate :> string)) |
There was a problem hiding this comment.
🔍 Validation only runs on non-empty strings
If the user types only non-digit characters, the filtered value becomes empty, and this length validation is skipped entirely. Combined with the requiredKeys check using getString (which returns "" for missing keys), this creates a gap where an all-non-digit input could slip through validation.
Consider consolidating the BIN presence and length validation into a single check:
let binStr = valuesDict->getString((AcquirerBin :> string), "")
switch binStr->String.length {
| 0 => setErr((AcquirerBin :> string), "This field is required")
| n if n < 4 || n > 20 => setErr((AcquirerBin :> string), "Acquirer BIN must be between 4 and 20 digits")
| _ => ()
}|
I think the same sort of bug lives in sibling screens: |
Untitled.mov
Type of Change
Description
The Acquirer BIN field was silently truncating values with more than 15-16 significant digits before sending them to the API.
Example:
12345678901234567890was being sent as12345678901234567000.This happened because:
binFieldusednumericTextInput, which stores input as a JavaScript float. IEEE 754 double precision can only safely represent around 15-16 significant digits.normalizeNumericStringFieldsusedgetFloatto read the BIN value, which also parsed string values as floats and caused truncation even after the input was changed to store strings.The fix:
numericTextInputwith a custom text input forbinFieldthat stores BIN as a string.AcquirerBinfromnormalizeNumericStringFieldsso the value passes through unchanged.validateFormto validate BIN length as a string instead of parsing it as a float.Motivation and Context
Fixes #4118
How did you test it?
Manually entered a 20-digit BIN value via the Add Network modal:
12345678901234567890Confirmed that the full value is preserved exactly in the API request payload without precision loss.
Where to test it?
Backend Dependency
Feature Flag
Checklist
npm run re:build