Skip to content

Throw errors for invalid currency conversion inputs - #1381

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

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

Conversation

@pavankumar-vh

Copy link
Copy Markdown

Overview

Fix currency conversion functions in common/src/util/currency.ts to throw explicit errors for invalid inputs instead of silently returning 0.

Bug Description

The previous version silently returned 0 when:

  • centsPerCredit was 0 or negative (division by zero or negative rate)
  • credits or amountInCents was NaN or Infinity

This is dangerous in money-conversion paths because misconfigurations would silently produce wrong values rather than failing loudly. A bad Stripe price setup or calculation error would result in users getting 0 credits/cents without any indication that something went wrong.

Fix

Changed to throw explicit errors with descriptive messages so invalid inputs are caught immediately during development and testing:

if (!Number.isFinite(centsPerCredit) || centsPerCredit <= 0) {
  throw new Error(
    `convertCreditsToUsdCents: centsPerCredit must be a positive finite number, got ${centsPerCredit}`,
  )
}
if (!Number.isFinite(credits)) {
  throw new Error(
    `convertCreditsToUsdCents: credits must be a finite number, got ${credits}`,
  )
}

Testing

Added comprehensive test coverage:

  • 8 test cases covering normal conversions and all error paths
  • Tests for NaN, Infinity, zero, and negative inputs
  • Tests for both functions

All 8 tests pass.

Call Site Audit

Searched for call sites of both functions:

  • convertCreditsToUsdCents: No external call sites found in codebase (only used internally)
  • convertStripeGrantAmountToCredits: No external call sites found in codebase (only used internally)

Both functions appear to be used internally within the codebase, and throwing errors will help catch configuration issues early.

Files Changed

  • common/src/util/currency.ts - Added validation and error throwing
  • common/src/util/__tests__/currency.test.ts - Added comprehensive test coverage

Scope

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

The previous version silently returned 0 for invalid inputs, which could mask
serious bugs in money-conversion paths. Changed to throw explicit errors with
descriptive messages so invalid inputs are caught immediately during development
and testing.

- Fixed the validation to use Number.isFinite() instead of relying on
  comparison operators that don't catch NaN
- Changed from returning 0 to throwing errors for invalid inputs
- Added comprehensive test coverage for all error paths and happy paths

All 8 tests pass.
@codebuff-team

Copy link
Copy Markdown
Contributor

Good instinct: silently returning 0 on invalid currency inputs is a real risk, and the added test coverage in currency.test.ts is solid and easy to follow.

The concern is the change in behavior at call sites. The PR body says "no external call sites found," but these are money-conversion functions almost certainly invoked from billing/Stripe webhook or grant-processing code elsewhere in the private tree (not visible in this mirror). Converting silent-zero to a thrown exception is the right long-term fix, but it needs to be paired with call-site handling - e.g. wrapping the Stripe webhook handler in a try/catch that logs and alerts, rather than letting an uncaught exception crash a webhook handler or background job. Right now this is a pure library change with no visibility into how callers will react to a new thrown error, which is risky for a payments path.

Also minor: credits <= 0 isn't rejected (only NaN/Infinity), so negative credits still silently produce negative cents rather than erroring - if the intent is to catch misconfiguration this asymmetry is worth reconsidering, or at least documenting why negative credits are allowed (e.g. refunds/debits).

Worth porting in spirit, but a maintainer will want to confirm/adjust the calling code before taking this as-is, since it changes a shared utility's contract without touching the callers that depend on it not throwing.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Sep 18, 2026

This branch has not been deployed

No deployments
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