Skip to content

Fix NaN handling in currency conversion input validation - #1251

Open
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/currency-input-validation
Open

Fix NaN handling in currency conversion input validation#1251
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/currency-input-validation

Conversation

@pavankumar-vh

Copy link
Copy Markdown

Overview

Fix NaN handling in currency conversion input validation in common/src/util/currency.ts.

Bug Description

The functions didn't validate that credits and amountInCents are finite numbers. If they were NaN or Infinity, Math.ceil(NaN * centsPerCredit) would return NaN.

Fix

Added Number.isFinite() checks to return 0 for invalid inputs.

Testing

No existing tests for this function, but the fix prevents incorrect behavior with invalid inputs.

Files Changed

  • common/src/util/currency.ts - Added NaN validation

Scope

This change only touches common/ which is an approved contribution area per the Contributing Guide.

The functions didn't validate that credits and amountInCents are finite numbers.
If they were NaN or Infinity, Math.ceil(NaN * centsPerCredit) would return NaN.

Added Number.isFinite() checks to return 0 for invalid inputs.
@codebuff-team

Copy link
Copy Markdown
Contributor

Thanks for looking into this — the underlying issue (NaN/Infinity propagating through Math.ceil/Math.floor) is real. But the fix has gaps:

  1. centsPerCredit <= 0 doesn't catch NaNNaN <= 0 is false in JS, so a NaN centsPerCredit still flows into the arithmetic and produces NaN, which is one of the exact cases you're trying to fix. You'd want !Number.isFinite(centsPerCredit) || centsPerCredit <= 0.
  2. Silently returning 0 in currency conversion code is a bigger decision than it looks. If credits/amountInCents is invalid, that's very likely an upstream bug (bad billing state, mis-parsed input, etc). Silently coercing to 0 credits/cents can mask that bug and produce confusing downstream behavior (e.g. a grant silently becomes worth 0 credits) rather than surfacing the error where it happened. Throwing, or at least logging, is usually safer for money-handling code than defaulting to zero.
  3. No tests were added even though this is exactly the kind of pure, easily-testable function that should get a unit test covering NaN, Infinity, zero, and negative inputs.

The PR is small and in-scope (common/), so this is a good candidate to rework rather than abandon. I'd suggest fixing the centsPerCredit check, deciding intentionally between throwing vs. defaulting, and adding a test file next to currency.ts.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants