Skip to content

fix: improve float handling in calldata encoder #199

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ygd58 wants to merge 1 commit into
base: v2-dev
Choose a base branch
from
+36 −1
Open

fix: improve float handling in calldata encoder #199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/abi/calldata/encoder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,15 @@ function encodeImpl(to: number[], data: CalldataEncodable) {
switch (typeof data) {
case "number": {
if (!Number.isInteger(data)) {
reportError("floats are not supported", data);
if (Number.isFinite(data) && Math.floor(data) === data) {
// Safe: float with no fractional part (e.g. 1.0 → 1)
encodeNum(to, BigInt(Math.trunc(data)));
return;
}
throw new Error(
`calldata encoding error: float value '${data}' is not supported. ` +
`Convert to an integer or pass as a string instead.`
);
Comment on lines 89 to +98

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Dead code: the whole-number float coercion branch is unreachable.

Number.isInteger(1.0) returns true in JavaScript because 1.0 === 1. The Number.isInteger specification checks Math.floor(x) === x for finite numbers — the same condition at line 90. Therefore, when !Number.isInteger(data) is true, Math.floor(data) === data is guaranteed false for all finite numbers, making lines 90–94 unreachable.

The PR's stated goal of handling 1.0 is already achieved by the existing Number.isInteger check at line 89 routing to line 100. The new code never executes.

🔧 Proposed fix: remove dead branch, keep improved error message 
     case "number": {
       if (!Number.isInteger(data)) {
-        if (Number.isFinite(data) && Math.floor(data) === data) {
-          // Safe: float with no fractional part (e.g. 1.0 → 1)
-          encodeNum(to, BigInt(Math.trunc(data)));
-          return;
-        }
         throw new Error(
           `calldata encoding error: float value '${data}' is not supported. ` +
           `Convert to an integer or pass as a string instead.`
         );
       }
       encodeNum(to, BigInt(data));
       return;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!Number.isInteger(data)) {
reportError("floats are not supported", data);
if (Number.isFinite(data) && Math.floor(data) === data) {
// Safe: float with no fractional part (e.g. 1.0 → 1)
encodeNum(to, BigInt(Math.trunc(data)));
return;
}
throw new Error(
`calldata encoding error: float value '${data}' is not supported. ` +
`Convert to an integer or pass as a string instead.`
);
if (!Number.isInteger(data)) {
throw new Error(
`calldata encoding error: float value '${data}' is not supported. ` +
`Convert to an integer or pass as a string instead.`
);
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/abi/calldata/encoder.ts` around lines 89 - 98, The dead whole-number
float coercion branch in `encodeNum` within `calldata` encoding is unreachable
because `Number.isInteger` already accepts values like `1.0`; remove the
`Number.isFinite`/`Math.floor` path and keep the existing integer handling,
while preserving the improved error message in the non-integer `throw` branch
for `src/abi/calldata/encoder.ts`.

}
encodeNum(to, BigInt(data));
return;
Expand Down
27 changes: 27 additions & 0 deletions tests/calldata-encoder.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { describe, it, expect } from "vitest";
import { encode } from "../src/abi/calldata/encoder";
import { decode } from "../src/abi/calldata/decoder";
Comment on lines +2 to +3

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use path aliases @/* for src/ imports per coding guidelines.

As per coding guidelines: **/*.{ts,tsx}: Use path alias @/* for imports from src/ directory.

♻️ Proposed fix 
-import { encode } from "../src/abi/calldata/encoder";
-import { decode } from "../src/abi/calldata/decoder";
+import { encode } from "`@/abi/calldata/encoder`";
+import { decode } from "`@/abi/calldata/decoder`";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import { encode } from "../src/abi/calldata/encoder";
import { decode } from "../src/abi/calldata/decoder";
import { encode } from "`@/abi/calldata/encoder`";
import { decode } from "`@/abi/calldata/decoder`";
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/calldata-encoder.test.ts` around lines 2 - 3, Update the imports in the
test file to use the project path alias for anything coming from src instead of
relative paths. Specifically, adjust the encode and decode imports in the
calldata encoder test to reference the aliased src location, following the same
`@/`* convention used elsewhere in the codebase.

Source: Coding guidelines


describe("calldata encoder - float handling", () => {
it("should encode integer numbers correctly", () => {
const encoded = encode(42);
const decoded = decode(encoded);
expect(decoded).toBe(42n);
});

it("should encode float with no fractional part as integer (e.g. 1.0)", () => {
const encoded = encode(1.0);
const decoded = decode(encoded);
expect(decoded).toBe(1n);
});

it("should throw descriptive error for true float values", () => {
expect(() => encode(1.5)).toThrow(
"calldata encoding error: float value '1.5' is not supported"
);
});

it("should throw descriptive error for NaN", () => {
expect(() => encode(NaN)).toThrow();
});
});
Loading