Skip to content

fix: limit decimal precision in ParsePercentageRatio to 18 places#129

Open
flemzord wants to merge 1 commit intomainfrom
fuzz/fixes-20260327
Open

fix: limit decimal precision in ParsePercentageRatio to 18 places#129
flemzord wants to merge 1 commit intomainfrom
fuzz/fixes-20260327

Conversation

@flemzord
Copy link
Member

Summary

  • Fixed a bug discovered by fuzz testing where ParsePercentageRatio accepted unreasonably large decimal precision (100+ digits)
  • Added validation to limit decimal places to 18, which is sufficient for financial calculations and matches common cryptocurrency precision standards
  • Added a fuzz test to prevent regression

Bug Details

The fuzzer discovered that inputs like "0.00000...000%" with 100+ decimal digits were accepted and returned floatingDigits > 100. This could potentially cause:

  • Integer overflow when casting to uint16
  • Performance issues with extremely large scale values
  • Unexpected behavior in downstream code expecting reasonable precision

Fix

Added a check in ParsePercentageRatio to reject percentages with more than 18 decimal places with a clear error message.

Test plan

  • Fuzz test runs for 120 seconds without finding issues
  • All existing tests pass
  • Linter passes
  • Added FuzzParsePercentageRatio to prevent regression

🤖 Generated with Claude Code

Fuzz testing discovered that ParsePercentageRatio accepted unreasonably
large decimal precision (100+ digits), which could cause issues downstream.
Added validation to limit decimal places to 18, which is sufficient for
financial calculations and matches common cryptocurrency precision standards.

Also added a fuzz test to prevent regression of this issue.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 27, 2026

Walkthrough

The changes add decimal precision validation to ParsePercentageRatio, rejecting inputs exceeding 18 fractional digits, and introduce a comprehensive fuzz test with corpus entries to verify the function's robustness across valid and invalid percentage string inputs.

Changes

Cohort / File(s) Summary
Precision Validation
internal/parser/parser.go
Added validation logic to ParsePercentageRatio that rejects inputs with decimal precision exceeding 18 places, returning an error when scale > 18 before proceeding with parsing.
Fuzz Test Coverage
internal/parser/percentage_fuzz_test.go, internal/parser/testdata/fuzz/FuzzParsePercentageRatio/*
Added FuzzParsePercentageRatio fuzz test with seed corpus covering valid cases (integers, decimals, boundaries like 0% and 100%) and invalid cases (out-of-range, negative, non-numeric, malformed inputs); includes assertions that function doesn't panic and respects the 18-digit constraint.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 With a hop and a nibble, precision takes root!
Eighteen decimals max—a boundary to boot.
Fuzz tests now scatter, from edge case to edge,
Our parser stands sturdy, right up to the ledge! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding a decimal precision limit of 18 places to the ParsePercentageRatio function.
Description check ✅ Passed The description is directly related to the changeset, providing context about the bug, the fix, and the test plan with clear details about the validation added.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fuzz/fixes-20260327

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Mar 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.55%. Comparing base (2a4745e) to head (264af01).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
+ Coverage   68.50%   68.55%   +0.05%     
==========================================
  Files          46       46              
  Lines        4648     4650       +2     
==========================================
+ Hits         3184     3188       +4     
+ Misses       1290     1289       -1     
+ Partials      174      173       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/parser/percentage_fuzz_test.go (1)

33-43: Add an explicit assertion for the “>18 decimals must error” rule.

Right now the fuzz test only checks invariants on success paths. It can miss regressions where over-precise inputs are silently accepted but normalized.

💡 Suggested hardening
 package parser
 
 import (
+	"strings"
 	"testing"
 )
@@
 	f.Fuzz(func(t *testing.T, input string) {
 		// Call ParsePercentageRatio and ensure it doesn't panic
 		num, floatingDigits, err := ParsePercentageRatio(input)
 
+		trimmed := strings.TrimSuffix(input, "%")
+		if i := strings.Index(trimmed, "."); i != -1 {
+			scale := len(trimmed) - i - 1
+			if scale > 18 && err == nil {
+				t.Fatalf("expected error for scale=%d (>18), input: %q", scale, input)
+			}
+		}
+
 		if err == nil {
 			// If parsing succeeded, verify the result is reasonable
 			if num == nil {
 				t.Errorf("ParsePercentageRatio succeeded but returned nil num for input: %q", input)
 			}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/parser/percentage_fuzz_test.go` around lines 33 - 43, The fuzz test
in percentage_fuzz_test.go must assert that inputs with more than 18 fractional
digits are rejected: when calling ParsePercentageRatio, compute floatingDigits
as currently done and if floatingDigits > 18 then require err != nil (fail the
test if err == nil), rather than only checking the success-path invariants;
update the test around the ParsePercentageRatio call to explicitly assert this
error case so the parser cannot silently accept or normalize >18-decimal inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/parser/percentage_fuzz_test.go`:
- Around line 33-43: The fuzz test in percentage_fuzz_test.go must assert that
inputs with more than 18 fractional digits are rejected: when calling
ParsePercentageRatio, compute floatingDigits as currently done and if
floatingDigits > 18 then require err != nil (fail the test if err == nil),
rather than only checking the success-path invariants; update the test around
the ParsePercentageRatio call to explicitly assert this error case so the parser
cannot silently accept or normalize >18-decimal inputs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9508ce7e-5e56-4d70-9b9e-d6f214281315

📥 Commits

Reviewing files that changed from the base of the PR and between 2a4745e and 264af01.

📒 Files selected for processing (3)
  • internal/parser/parser.go
  • internal/parser/percentage_fuzz_test.go
  • internal/parser/testdata/fuzz/FuzzParsePercentageRatio/98e3857fbdb2cc2c

@flemzord flemzord requested a review from ascandone March 27, 2026 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant