fix: improve float handling in calldata encoder#199
Conversation
- Encode floats with no fractional part (e.g. 1.0) as integers - Replace generic error with descriptive message for true floats - Add unit tests for float encoding edge cases Fixes genlayerlabs#27
📝 WalkthroughWalkthroughThe calldata encoder's number-handling logic was modified to accept finite numbers with no fractional component by truncating them, while throwing a descriptive error for true fractional floats, NaN, or infinite values. A new test suite validates integer encoding, whole-number float coercion, and error cases. ChangesFloat number encoding fix
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
|
This PR targeted I retargeted it to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/calldata-encoder.test.ts (1)
18-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd edge case tests for
Infinity,-Infinity, and negative fractional floats.
Infinityand-Infinityhit the same!Number.isInteger→!Number.isFinite→ throw path asNaN, but aren't tested. A negative fractional float like-1.5would verify the error message includes the correct value.✨ Suggested additional tests
it("should throw descriptive error for NaN", () => { expect(() => encode(NaN)).toThrow(); }); + + it("should throw descriptive error for Infinity", () => { + expect(() => encode(Infinity)).toThrow(); + }); + + it("should throw descriptive error for negative fractional floats", () => { + expect(() => encode(-1.5)).toThrow( + "calldata encoding error: float value '-1.5' is not supported" + ); + });🤖 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 18 - 26, Add edge case coverage in the calldata encoder tests by extending the existing `encode` expectations alongside the `NaN` case. Add assertions for `Infinity` and `-Infinity` to verify they hit the same non-finite error path, and add a negative fractional float such as `-1.5` to confirm the thrown message includes the exact value. Use the current `calldata-encoder.test.ts` `encode(...)` test block as the place to locate and expand these cases.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/abi/calldata/encoder.ts`:
- Around line 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`.
In `@tests/calldata-encoder.test.ts`:
- Around line 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.
---
Nitpick comments:
In `@tests/calldata-encoder.test.ts`:
- Around line 18-26: Add edge case coverage in the calldata encoder tests by
extending the existing `encode` expectations alongside the `NaN` case. Add
assertions for `Infinity` and `-Infinity` to verify they hit the same non-finite
error path, and add a negative fractional float such as `-1.5` to confirm the
thrown message includes the exact value. Use the current
`calldata-encoder.test.ts` `encode(...)` test block as the place to locate and
expand these cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77e4a583-042f-42db-90d6-b94f562ed94b
📒 Files selected for processing (2)
src/abi/calldata/encoder.tstests/calldata-encoder.test.ts
| 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.` | ||
| ); |
There was a problem hiding this comment.
🎯 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.
| 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`.
| import { encode } from "../src/abi/calldata/encoder"; | ||
| import { decode } from "../src/abi/calldata/decoder"; |
There was a problem hiding this comment.
📐 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.
| 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
|
This PR targeted I retargeted it to |
1 participant
Summary
Improves float value handling in the calldata encoder.
Problem
Currently, passing any
numberwith a fractional part throws a generic error:invalid calldata input. This is unhelpful and also incorrectly rejects floats like1.0that are safely representable as integers.Fixes #27
Changes
1.0,2.0) are now silently coerced to their integer equivalent and encoded correctly1.5) now throw a descriptive error message telling the user to convert to integer or stringTests
Summary by CodeRabbit
1.0are accepted and encoded correctly.NaN, and infinities now fail with a clearer error message that explains how to proceed.