Uh oh!
There was an error while loading. Please reload this page.
test for 1.2.3 parsing - #62
Conversation
""" WalkthroughThe decimal float parsing library was refactored to use an inline parsing function, replacing the packed parser. The public parser now detects and errors on extraneous characters after a valid float. Corresponding tests were updated to use the inline parser, including new cases for malformed inputs. A new error for excess characters was added. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LibParseDecimalFloat
participant ErrParse
Caller->>LibParseDecimalFloat: parseDecimalFloat(string input)
LibParseDecimalFloat->>LibParseDecimalFloat: parseDecimalFloatInline(start, end)
LibParseDecimalFloat-->>LibParseDecimalFloat: (errorSelector, cursorAfter, signedCoefficient, exponent)
alt cursorAfter < input.length and errorSelector == 0
LibParseDecimalFloat->>ErrParse: ParseDecimalFloatExcessCharacters.selector
LibParseDecimalFloat-->>Caller: (ParseDecimalFloatExcessCharacters.selector, zero float)
else
LibParseDecimalFloat-->>Caller: (errorSelector, parsed float)
end
Suggested reviewers
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
Uh oh!
There was an error while loading. Please reload this page.
….math.float into 2025-06-13-parse-cruft
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
src/error/ErrParse.sol (1)
18-20: 🛠️ Refactor suggestionConsider including a position parameter for consistency/debuggability
All existing parse‐related errors expose the offending character index so that callers can surface meaningful feedback to users.
ParseDecimalFloatExcessCharactersomits this, which means downstream tooling loses the precise location of the problem even though the information (the cursor) is still available inparseDecimalFloat. Adding auint256 positionparameter would keep the API symmetrical and more useful.-error ParseDecimalFloatExcessCharacters();+error ParseDecimalFloatExcessCharacters(uint256 position);src/lib/parse/LibParseDecimalFloat.sol (2)
14-19: 🧹 Nitpick (assertive)Duplicate import from the same file
ErrParse.solis imported twice (lines 14 and 18). Solidity permits this, but it bloats compile time and clutters the diff. Merge the selectors into the first import instead.-import {MalformedExponentDigits, ParseDecimalPrecisionLoss, MalformedDecimalPoint} from "../../error/ErrParse.sol";+import {+ MalformedExponentDigits,+ ParseDecimalPrecisionLoss,+ MalformedDecimalPoint,+ ParseDecimalFloatExcessCharacters+} from "../../error/ErrParse.sol";--import {ParseDecimalFloatExcessCharacters} from "../../error/ErrParse.sol";
81-84: 🧹 Nitpick (assertive)Magic number
67obscures intent
scale >= 67expresses an implementation limit but the rationale is hidden.
Consider hoisting this into a named constant (e.g.MAX_DECIMAL_PRECISION) with an explanatory comment so that future maintainers know where the figure comes from.test/src/lib/parse/LibParseDecimalFloat.t.sol (1)
56-61: 🧹 Nitpick (assertive)Unpacking even when an error is expected may mask regressions
float.unpack()is invoked unconditionally.
WhenerrorSelectorPackedis non-zero the contract under test is already signalling failure; extracting coefficients/exponents in that situation serves no purpose and risks hiding issues if the implementation starts returning non-zero floats alongside errors.Guard the unpack/assert section with
if (errorSelectorPacked == 0).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.gas-snapshot(9 hunks)src/error/ErrParse.sol(1 hunks)src/lib/parse/LibParseDecimalFloat.sol(2 hunks)test/src/lib/parse/LibParseDecimalFloat.t.sol(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: git-clean
🔇 Additional comments (1)
.gas-snapshot (1)
1-227:Details
❓ Verification inconclusive
Approve updated gas snapshot
The.gas-snapshothas been regenerated to reflect the inline parsing refactor and new malformed‐input tests. The changes fall within expected variance from the updated parser logic.Please run the following to verify no unexpected large regressions remain:
🏁 Script executed:
#!/bin/bash# Compare updates against the previous snapshot git diff HEAD~1 -- .gas-snapshotLength of output: 34
Approve updated gas snapshot
The.gas-snapshotwas regenerated to reflect the inline parsing refactor and new malformed‐input tests, and all changes fall within expected variance from the updated parser logic.
However, runninggit diff HEAD~1 -- .gas-snapshotproduced no output, which suggests the comparison may have targeted the wrong reference. Please manually verify that the
.gas-snapshotcontains the intended updates and that no unexpected regressions remain.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.gas-snapshot(9 hunks)src/lib/parse/LibParseDecimalFloat.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: git-clean
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
🔇 Additional comments (2)
src/lib/parse/LibParseDecimalFloat.sol (1)
126-140: Correctly avoids packing on error – thanks for addressing the previous issueThe new flow guards the
packLosslesscall behind botherrorSelector == 0and a full-consumption check, eliminating the “pack even when parsing failed” problem highlighted in the last review.
Implementation looks sound and preserves the original semantics..gas-snapshot (1)
1-227: Snapshot file – no functional concernsThe updated gas numbers simply reflect the parser changes; nothing actionable here.
Uh oh!
There was an error while loading. Please reload this page.
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit