wip on consolidating packed#30
Conversation
## Walkthrough
This change refactors the decimal float handling across the codebase to use a single packed `Float` type alias (wrapping `bytes32`) instead of a memory-based `Float` struct. All functions in the main library and related modules now accept and return the packed `Float` type, unpacking it internally as needed. A new custom error, `CoefficientOverflow`, is introduced for coefficient overflows. All test contracts and formatting/parsing utilities are updated to use the packed representation, removing or adapting memory-based code and tests accordingly. Function signatures, parameter passing, and internal logic are aligned with this packed approach. Additionally, the `LibDecimalFloatImplementation` library is extended with comprehensive arithmetic and logarithmic operations, normalization, and overflow checks. Many test contracts are simplified, removing redundant tests and focusing on core equivalence and consistency validations.
## Changes
| Files / Paths | Change Summary |
|----------------------------------------------------------------------|----------------|
| src/error/ErrDecimalFloat.sol | Added new custom error `CoefficientOverflow(int256 signedCoefficient, int256 exponent)` for coefficient overflow conditions. |
| src/lib/LibDecimalFloat.sol | Replaced `PackedFloat` and memory-based `Float` struct with a packed `Float` type alias; updated all functions to use packed `Float`, added overflow error, removed downscaling on overflow, and unified internal logic by delegating arithmetic to `LibDecimalFloatImplementation`. |
| src/lib/format/LibFormatDecimalFloat.sol | Updated `toDecimalString` to accept packed `Float` and use unpacking instead of direct struct field access. |
| src/lib/parse/LibParseDecimalFloat.sol | Changed parsing functions to return packed `Float`, updated error handling, removed memory struct usage, lowered fractional precision threshold, and adjusted function signatures accordingly. |
| src/lib/implementation/LibDecimalFloatImplementation.sol | Added comprehensive arithmetic and logarithmic operations (add, sub, multiply, divide, minus, eq, inv, log10, power10) for decimal floats with normalization, overflow checks, and table-driven approximations. |
| src/lib/table/LibLogTable.sol | Extended fixed-size lookup tables by one row to support interpolation beyond original bounds; adjusted array sizes and assembly loops accordingly; removed console import. |
| test/src/lib/LibDecimalFloat.abs.t.sol<br>test/src/lib/LibDecimalFloat.add.t.sol<br>test/src/lib/LibDecimalFloat.decimal.t.sol<br>test/src/lib/LibDecimalFloat.decimalLossless.t.sol<br>test/src/lib/LibDecimalFloat.divide.t.sol<br>test/src/lib/LibDecimalFloat.eq.t.sol<br>test/src/lib/LibDecimalFloat.floor.t.sol<br>test/src/lib/LibDecimalFloat.frac.t.sol<br>test/src/lib/LibDecimalFloat.gt.t.sol<br>test/src/lib/LibDecimalFloat.inv.t.sol<br>test/src/lib/LibDecimalFloat.log10.t.sol<br>test/src/lib/LibDecimalFloat.lt.t.sol<br>test/src/lib/LibDecimalFloat.max.t.sol<br>test/src/lib/LibDecimalFloat.min.t.sol<br>test/src/lib/LibDecimalFloat.minus.t.sol<br>test/src/lib/LibDecimalFloat.multiply.t.sol<br>test/src/lib/LibDecimalFloat.pack.t.sol<br>test/src/lib/LibDecimalFloat.power.t.sol<br>test/src/lib/LibDecimalFloat.power10.t.sol<br>test/src/lib/LibDecimalFloat.sub.t.sol | Updated all tests to use packed `Float` type, removed or adapted memory-based struct usage, changed function signatures, and updated assertions to use unpacked values; removed redundant external wrappers and many individual test cases, focusing on core equivalence and consistency tests. |
| test/src/lib/format/LibFormatDecimalFloat.t.sol | Updated test contract to use packed `Float` type, adjusted function signatures, and replaced memory-based struct handling. |
| test/src/lib/parse/LibParseDecimalFloat.t.sol | Updated to use packed `Float`, adapted tests to use unpacking and new function signatures. |
| test/src/lib/implementation/LibDecimalFloatImplementation.add.t.sol | Added new test contract with comprehensive tests for the `add` function in `LibDecimalFloatImplementation`. |
| test/src/lib/implementation/LibDecimalFloatImplementation.divide.t.sol | Added new test contract with multiple division test cases and fuzz tests for `divide` in `LibDecimalFloatImplementation`. |
| test/src/lib/implementation/LibDecimalFloatImplementation.eq.t.sol | Added new test contract with extensive equality tests for `eq` in `LibDecimalFloatImplementation`. |
| test/src/lib/implementation/LibDecimalFloatImplementation.inv.t.sol | Added new test contract validating the `inv` function against a slow reference implementation. |
| test/src/lib/implementation/LibDecimalFloatImplementation.log10.t.sol | Added new test contract verifying `log10` behavior with exact, lookup, interpolated, and sub-1 cases. |
| test/src/lib/implementation/LibDecimalFloatImplementation.minus.t.sol | Added new test contract verifying `minus` function equivalence to subtraction from zero. |
| test/src/lib/implementation/LibDecimalFloatImplementation.multiply.t.sol | Added new test contract covering multiple multiplication scenarios and property tests for `multiply`. |
| test/src/lib/implementation/LibDecimalFloatImplementation.power10.t.sol | Added new test contract with tests for exact powers, lookups, interpolation, and fuzzing for `power10`. |
| test/src/lib/implementation/LibDecimalFloatImplementation.sub.t.sol | Added new test contract verifying subtraction behavior and edge cases for `sub`. |
| test/abstract/LogTest.sol | Modified import to also re-export `console2` for convenience. |
| test/lib/LibDecimalFloatSlow.sol | Updated to use `LibDecimalFloatImplementation.divide` in `invSlow` function. |
| test/src/lib/LibDecimalFloat.mixed.t.sol | Refactored test code to use `Float` struct methods for division and multiplication instead of raw integer parameters. |
| test/src/lib/LibDecimalFloat.normalize.t.sol | Deleted entire contract testing normalization functions. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Caller
participant LibDecimalFloat
participant ErrorModule
Caller->>LibDecimalFloat: pack(signedCoefficient, exponent)
LibDecimalFloat->>LibDecimalFloat: check coefficient fits 224 bits
alt Overflow
LibDecimalFloat->>ErrorModule: revert CoefficientOverflow(signedCoefficient, exponent)
else No Overflow
LibDecimalFloat->>LibDecimalFloat: pack into Float (bytes32)
LibDecimalFloat-->>Caller: Float
endPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (10)
test/src/lib/LibDecimalFloat.inv.t.sol (1)
20-32:⚠️ Potential issueTest failure detected: ExponentOverflow error needs addressing.
The updated test correctly unpacks and compares Float components, but pipeline failures indicate an
ExponentOverflow(-5e37, -2.147e9)error is occurring during testing.The test implementation is correct, but the pipeline error suggests that there may be:
- A limitation in the new packed representation
- An edge case in the
invfunction when handling extreme values- A need for additional bounds checking before calling
invConsider adding appropriate input bounds checking or handling for extreme exponent values in the
testInvMemfunction.test/src/lib/LibDecimalFloat.log10.t.sol (1)
23-36:⚠️ Potential issueTest failure detected: CoefficientOverflow error needs addressing.
The test correctly unpacks and compares Float components, but pipeline failures indicate a
CoefficientOverflow(5e69, -36)error is occurring.The test implementation follows the same pattern as other updated tests, but the pipeline error suggests:
- The input values may be causing coefficient overflow in the new packed representation
- Additional bounds checking may be needed in the
testLog10MemfunctionConsider adding appropriate input bounds checking or handling for large coefficient values to ensure compatibility with the packed Float representation.
test/src/lib/LibDecimalFloat.power10.t.sol (1)
21-34:⚠️ Potential issueFix ExponentOverflow in testPower10Mem
The pipeline is failing with an ExponentOverflow error in this function. This occurs when the exponent exceeds the allowed range in the packed representation.
#!/bin/bash # Analyze how the ExponentOverflow is occurring in the test # Look at the power10 implementation and test function rg -A 5 "ExponentOverflow" cat test/src/lib/LibDecimalFloat.power10.t.sol | grep -A 15 "testPower10Mem"The test needs additional bounds checking on the input float to prevent exponent overflow:
function testPower10Mem(Float float) external { + // Avoid excessively large exponents that cause overflow + (int256 signedCoeff, int256 exp) = float.unpack(); + exp = bound(exp, -70, 70); + float = LibDecimalFloat.pack(signedCoeff, exp); (int256 signedCoefficientFloat, int256 exponentFloat) = float.unpack();test/src/lib/LibDecimalFloat.multiply.t.sol (2)
33-47:⚠️ Potential issueFix CoefficientOverflow in testMultiplyMem
The pipeline is failing with a CoefficientOverflow error in this function. This occurs when the multiplication result exceeds the coefficient capacity in the packed representation.
#!/bin/bash # Analyze multiplication coefficient overflow issues # Look at multiply implementation rg -A 10 "multiply.*Float" src/The test needs additional bounds on the input values to prevent coefficient overflow during multiplication:
function testMultiplyMem(Float a, Float b) external { + // Bound coefficients to prevent overflow in multiplication + (int256 aCoeff, int256 aExp) = a.unpack(); + (int256 bCoeff, int256 bExp) = b.unpack(); + aCoeff = bound(aCoeff, -1e30, 1e30); + bCoeff = bound(bCoeff, -1e30, 1e30); + a = LibDecimalFloat.pack(aCoeff, aExp); + b = LibDecimalFloat.pack(bCoeff, bExp); (int256 signedCoefficientA, int256 exponentA) = a.unpack();
126-141: 🧹 Nitpick (assertive)Consider updating fuzzing test for packed Float representation
This fuzzing test should also be updated to work with the packed Float representation to ensure comprehensive testing of the new implementation.
function testMultiplyNotRevertAnyExpectation( - int256 signedCoefficientA, - int256 exponentA, - int256 signedCoefficientB, - int256 exponentB + Float a, + Float b ) external pure { + (int256 signedCoefficientA, int256 exponentA) = a.unpack(); + (int256 signedCoefficientB, int256 exponentB) = b.unpack(); exponentA = bound(exponentA, EXPONENT_MIN, EXPONENT_MAX); exponentB = bound(exponentB, EXPONENT_MIN, EXPONENT_MAX); + + a = LibDecimalFloat.pack(signedCoefficientA, exponentA); + b = LibDecimalFloat.pack(signedCoefficientB, exponentB); + + Float result = LibDecimalFloat.multiply(a, b); + (int256 signedCoefficient, int256 exponent) = result.unpack(); - (int256 signedCoefficient, int256 exponent) = - LibDecimalFloat.multiply(signedCoefficientA, exponentA, signedCoefficientB, exponentB); (int256 expectedSignedCoefficient, int256 expectedExponent) = LibDecimalFloatSlow.multiplySlow(signedCoefficientA, exponentA, signedCoefficientB, exponentB);test/src/lib/LibDecimalFloat.divide.t.sol (1)
24-38:⚠️ Potential issueRenamed test function and associated ExponentOverflow failure.
The test was appropriately renamed from
testDivideMemtotestDividePackedto reflect its use of packed values, but it's failing withExponentOverflow(1e38, 2.454e9). This suggests the refactored version has stricter exponent validation than before.You need to either:
- Modify the fuzz tests to bound inputs within exponent limits
- Add exception handling for this specific overflow case
function testDividePacked(Float a, Float b) external { + // Skip test if inputs would cause exponent overflow + (int256 signedCoefficientA, int256 exponentA) = a.unpack(); + (int256 signedCoefficientB, int256 exponentB) = b.unpack(); + if (exponentA > 1e8 || exponentB > 1e8) return; + (int256 signedCoefficientA, int256 exponentA) = a.unpack(); (int256 signedCoefficientB, int256 exponentB) = b.unpack();test/src/lib/LibDecimalFloat.floor.t.sol (1)
1-113: 🧹 Nitpick (assertive)Consider adding bound checks for all fuzzing tests.
Since other similar tests are failing with overflow errors, it might be valuable to add appropriate bounds for this test as well to prevent similar issues in the future.
function testFloorMem(Float float) external { + // Skip test cases that might cause coefficient or exponent overflow + (int256 signedCoefficient, int256 exponent) = float.unpack(); + if (exponent > 1e8 || exponent < -1e8) return; + (int256 signedCoefficient, int256 exponent) = float.unpack();test/src/lib/LibDecimalFloat.pack.t.sol (1)
19-28: 💡 Verification agent🧩 Analysis chain
Fix failing test due to incorrect exponent handling for zero
The test is failing with "exponent mismatch: 1 != 0" because the test is not handling zero coefficients correctly.
When the coefficient is zero, the test expects the exponent to be zero regardless of what was packed. Check the implementation of
unpackto see how it normalizes zero values:function testPartsRoundTrip(int224 signedCoefficient, int32 exponent) external pure { + // Special handling for zero - we know exponent will always be normalized to 0 + if (signedCoefficient == 0) { + exponent = 0; + } + Float float = LibDecimalFloat.pack(signedCoefficient, exponent); (int256 signedCoefficientOut, int256 exponentOut) = LibDecimalFloat.unpack(float); assertEq(signedCoefficient, signedCoefficientOut, "coefficient"); if (signedCoefficient != 0) { assertEq(exponent, exponentOut, "exponent"); } else { // 0 exponent is always 0. assertEq(exponentOut, 0, "exponent"); }
🏁 Script executed:
#!/bin/bash # Check how the unpack function handles zero coefficients ast-grep --pattern $'function unpack(Float float) internal pure returns'Length of output: 73
🏁 Script executed:
#!/bin/bash set -e # Locate the LibDecimalFloat library file file=$(rg -l "library LibDecimalFloat" -t sol) echo "Library file: $file" echo "----- pack() implementation -----" rg -n "function pack" -A20 "$file" echo "----- unpack() implementation -----" rg -n "function unpack" -A20 "$file"Length of output: 86
🏁 Script executed:
#!/bin/bash set -e # List all Solidity files to locate the library echo "Solidity files in repo:" fd --extension sol # Search for the LibDecimalFloat library definition lib_file=$(rg -l "library LibDecimalFloat" --glob "*.sol") echo "Found LibDecimalFloat file: $lib_file" # Show the pack() implementation echo "----- pack() implementation -----" rg -n "function pack" -A20 "$lib_file" # Show the unpack() implementation echo "----- unpack() implementation -----" rg -n "function unpack" -A20 "$lib_file"Length of output: 3140
🏁 Script executed:
#!/bin/bash set -e echo "----- function pack definitions -----" rg -n "function pack" -g "src/lib/*.sol" -g "src/lib/implementation/*.sol" || echo "No pack definitions found in these paths." echo "----- function unpack definitions -----" rg -n "function unpack" -g "src/lib/*.sol" -g "src/lib/implementation/*.sol" || echo "No unpack definitions found in these paths."Length of output: 602
🏁 Script executed:
#!/bin/bash set -e # Display pack implementation with context echo "----- pack() implementation (lines around 304) -----" rg -n -C5 "function pack" src/lib/LibDecimalFloat.sol # Display unpack implementation with context echo "----- unpack() implementation (lines around 329) -----" rg -n -C5 "function unpack" src/lib/LibDecimalFloat.solLength of output: 1570
Align test with actual pack/unpack behavior: always assert exponent matches
The current implementation of
pack/unpackpreserves the exponent even when the coefficient is zero. The test should therefore always compare the unpacked exponent to the input exponent instead of special‑casing zero.• File:
test/src/lib/LibDecimalFloat.pack.t.sol
Update thetestPartsRoundTripfunction (around lines 19–28) as follows:- if (signedCoefficient != 0) { - assertEq(exponent, exponentOut, "exponent"); - } else { - // 0 exponent is always 0. - assertEq(exponentOut, 0, "exponent"); - } + // Exponent is preserved by unpack(), even for zero coefficients + assertEq(exponent, exponentOut, "exponent");This change removes the incorrect zero‑coefficient branch and ensures the test matches the library’s behavior.
src/lib/LibDecimalFloat.sol (2)
304-318: 🛠️ Refactor suggestion
packcan be micro‑optimised and made safer
- Branchless masking of the exponent is cheaper than an
int32cast/revert, and avoids silent truncation if the cast is ever removed.- The current mask/OR works but allocates a local
maskevery call; consider an immutable constant.+uint256 constant COEFF_MASK = type(uint224).max; +uint256 constant EXP_SHIFT = 224; function pack(int256 signedCoefficient, int256 exponent) internal pure returns (Float float) { - if (int224(signedCoefficient) != signedCoefficient) { + if (signedCoefficient < type(int224).min || signedCoefficient > type(int224).max) { revert CoefficientOverflow(signedCoefficient, exponent); } - if (int32(exponent) != exponent) { + if (exponent < type(int32).min || exponent > type(int32).max) { revert ExponentOverflow(signedCoefficient, exponent); } - uint256 mask = type(uint224).max; assembly ("memory-safe") { - float := or(and(signedCoefficient, mask), shl(0xe0, exponent)) + float := or(and(signedCoefficient, COEFF_MASK), shl(EXP_SHIFT, exponent)) } }
128-140: 💡 Verification agent🧩 Analysis chain
Doc‑strings still refer to a “Float struct”
After migrating to the packed
type Float is bytes32, the NatSpec comments referencing “Float struct” (e.g. “returns a Float struct containing …”) are misleading.Please update all affected comments (
fromFixedDecimal*,toFixedDecimal*, etc.) to avoid confusion for integrators and automated documentation generators.
🏁 Script executed:
#!/bin/bash # Search for doc-comments referencing "Float struct" across the codebase rg -n "Float struct" src # Search for any @return tags mentioning "Float" rg -n "@return.*Float" srcLength of output: 5958
Update NatSpec comments to reflect packed Float type (bytes32)
The documentation still refers to a non‑existent “Float struct” even though
Floatis now a packed alias overbytes32. Please update all NatSpec references insrc/lib/LibDecimalFloat.solto remove “struct” language and simply refer to theFloattype. For example:
- Change
/// @return float The Float struct containing the signed coefficient and exponent.
to/// @return float The Float containing the signed coefficient and exponent.
- Change/// @param float The Float struct containing the signed coefficient and ...
to/// @param float The Float containing the signed coefficient and …
Affected sections include, but aren’t limited to: - `fromFixedDecimal*` / `toFixedDecimal*` conversions - Arithmetic methods (`add`, `sub`, `multiply`, `divide`, etc.) - Comparison methods (`eq`, `lt`, `gt`, etc.) - Utility methods (`abs`, `minus`, `power10`, `inv`, etc.) Ensuring these comments accurately describe the packed `Float` type will improve clarity for integrators and automated documentation generators. </blockquote></details> </blockquote></details> <details> <summary>♻️ Duplicate comments (1)</summary><blockquote> <details> <summary>test/src/lib/LibDecimalFloat.min.t.sol (1)</summary><blockquote> `24-33`: **Same `expectRevert` positioning issue as in the max test** Move `vm.expectRevert(err)` *before* the second call, or assert the revert reason from the first call directly (see comment in `max.t.sol`). </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: ASSERTIVE** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 23112899eb095001e9bdfd829cfe9149ea232d7a and f37077005380c617e76afea17822b8424c3b536d. </details> <details> <summary>📒 Files selected for processing (27)</summary> * `src/error/ErrDecimalFloat.sol` (1 hunks) * `src/lib/LibDecimalFloat.sol` (27 hunks) * `src/lib/format/LibFormatDecimalFloat.sol` (1 hunks) * `src/lib/parse/LibParseDecimalFloat.sol` (3 hunks) * `test/src/lib/LibDecimalFloat.abs.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.add.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.decimal.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.decimalLossless.t.sol` (3 hunks) * `test/src/lib/LibDecimalFloat.divide.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.eq.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.floor.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.frac.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.gt.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.inv.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.log10.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.lt.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.max.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.min.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.minus.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.multiply.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.normalize.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.pack.t.sol` (2 hunks) * `test/src/lib/LibDecimalFloat.power.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.power10.t.sol` (1 hunks) * `test/src/lib/LibDecimalFloat.sub.t.sol` (1 hunks) * `test/src/lib/format/LibFormatDecimalFloat.t.sol` (2 hunks) * `test/src/lib/parse/LibParseDecimalFloat.t.sol` (3 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 GitHub Actions: Rainix CI</summary> <details> <summary>test/src/lib/LibDecimalFloat.power.t.sol</summary> [error] 1-1: Test 'testPowerMem(bytes32,bytes32)' failed due to ExponentOverflow(6683, 8.745e18). </details> <details> <summary>test/src/lib/LibDecimalFloat.decimal.t.sol</summary> [error] 1-1: Test 'testFromFixedDecimalLossyPacked(uint256,uint8)' failed due to CoefficientOverflow(1.347e67, 0). </details> <details> <summary>test/src/lib/LibDecimalFloat.divide.t.sol</summary> [error] 1-1: Test 'testDividePacked(bytes32,bytes32)' failed due to ExponentOverflow(1e38, 2.454e9). </details> <details> <summary>test/src/lib/LibDecimalFloat.sub.t.sol</summary> [error] 1-1: Test 'testSubMem(bytes32,bytes32)' failed due to CoefficientOverflow(-9.999e70, -34). </details> <details> <summary>test/src/lib/LibDecimalFloat.multiply.t.sol</summary> [error] 1-1: Test 'testMultiplyMem(bytes32,bytes32)' failed due to CoefficientOverflow(1.348e67, 0). </details> <details> <summary>test/src/lib/LibDecimalFloat.log10.t.sol</summary> [error] 1-1: Test 'testLog10Mem(bytes32)' failed due to CoefficientOverflow(5e69, -36). </details> <details> <summary>test/src/lib/LibDecimalFloat.inv.t.sol</summary> [error] 1-1: Test 'testInvMem(bytes32)' failed due to ExponentOverflow(-5e37, -2.147e9). </details> <details> <summary>test/src/lib/format/LibFormatDecimalFloat.t.sol</summary> [error] 1-1: Test 'testRoundTrip(uint256)' failed due to CoefficientOverflow(1.347e67, -18). </details> <details> <summary>test/src/lib/LibDecimalFloat.power10.t.sol</summary> [error] 1-1: Test 'testPower10Mem(bytes32)' failed due to ExponentOverflow(1000, 1.422e66). </details> <details> <summary>test/src/lib/LibDecimalFloat.decimalLossless.t.sol</summary> [error] 1-1: Test 'testFromFixedDecimalLosslessMem(uint256,uint8)' failed due to CoefficientOverflow(1.347e67, 0). </details> <details> <summary>test/src/lib/LibDecimalFloat.pack.t.sol</summary> [error] 1-1: Test 'testPartsRoundTrip(int224,int32)' failed due to exponent mismatch: 1 != 0. </details> <details> <summary>test/src/lib/LibDecimalFloat.add.t.sol</summary> [error] 1-1: Test 'testAddPacked(bytes32,bytes32)' failed due to CoefficientOverflow(1e70, -34). --- [error] 1-1: Test 'testAddUnpacked(int256,int256,int256,int256)' failed due to ExponentOverflow(0, 2.147e9). </details> </details> </details> <details> <summary>🔇 Additional comments (48)</summary><blockquote> <details> <summary>src/error/ErrDecimalFloat.sol (1)</summary> `5-6`: **New error type adds proper overflow handling for coefficients** The addition of `CoefficientOverflow` error follows the same pattern as the existing `ExponentOverflow` error. This enhances error handling specificity for decimal float operations. </details> <details> <summary>src/lib/format/LibFormatDecimalFloat.sol (1)</summary> `23-25`: **Good adaption to packed Float representation** Function properly modified to use the packed `Float` type, unpacking it with `LibDecimalFloat.unpack()` instead of directly accessing struct fields. This is consistent with the codebase-wide transition to using packed representations. </details> <details> <summary>test/src/lib/LibDecimalFloat.frac.t.sol (2)</summary> `15-17`: **Correct adoption of packed Float type** Function signature properly updated to use packed `Float` type by value rather than `Float memory`, aligning with the codebase-wide changes to use packed representation. --- `19-31`: **Test correctly adapted for packed Float values** Test function properly updated to: 1. Unpack the input Float to get components 2. Compare against unpacked values from the result 3. Maintain the same try-catch error handling logic This maintains test integrity while adopting the new packed Float representation. </details> <details> <summary>test/src/lib/LibDecimalFloat.minus.t.sol (2)</summary> `16-18`: **Correct adoption of packed Float type** Function signature properly updated to use packed `Float` type by value rather than `Float memory`, aligning with the codebase-wide changes to use packed representation. --- `21-33`: **Test correctly adapted for packed Float values** Test function properly updated to: 1. Unpack the input Float to get components 2. Compare against unpacked values from the result 3. Maintain the same try-catch error handling logic This maintains test integrity while adopting the new packed Float representation. </details> <details> <summary>test/src/lib/LibDecimalFloat.lt.t.sol (2)</summary> `21-23`: **Good addition of a wrapper function for testing the packed Float representation.** The new `ltExternal` function provides a clear way to test the packed `Float` version of the `lt` function. This overloaded function helps maintain test coverage while transitioning to the packed representation. --- `25-35`: **Well-structured update to test the packed Float type.** This test implementation now properly: 1. Unpacks the input `Float` values to get their components 2. Tests both the unpacked component version and the packed version 3. Maintains error handling behavior consistency Good use of the `unpack()` method to facilitate the comparison between the two function variants. </details> <details> <summary>test/src/lib/parse/LibParseDecimalFloat.t.sol (4)</summary> `11-11`: **Good update to import the required library for Float type.** The import correctly brings in the `Float` type from `LibDecimalFloat` to support the packed representation changes. --- `16-16`: **Correct addition of the using directive for LibDecimalFloat.** Adding `using LibDecimalFloat for Float` properly enables access to the `unpack()` method needed for the updated test implementation. --- `28-34`: **Well-named function change reflecting packed representation.** Renaming from `parseDecimalFloatExternalMem` to `parseDecimalFloatExternalPacked` clearly communicates the change in representation, and the return type is correctly updated to use the packed `Float` type. --- `42-50`: **Properly updated test to handle the packed Float representation.** The test now: 1. Calls the packed version of the parse function 2. Correctly verifies error selectors match 3. Unpacks the returned Float to compare individual components 4. Maintains error handling behavior consistency Good job ensuring backward compatibility with the test while adapting to the new type system. </details> <details> <summary>test/src/lib/LibDecimalFloat.inv.t.sol (1)</summary> `15-17`: **Correctly updated function signature for packed Float.** The `invExternal` function now accepts and returns the packed `Float` type without the memory modifier, aligning with the consolidated representation changes. </details> <details> <summary>test/src/lib/LibDecimalFloat.log10.t.sol (1)</summary> `17-20`: **Correctly updated function signature for packed Float.** The `log10External` function now accepts and returns the packed `Float` type without the memory modifier, aligning with the consolidated representation changes. </details> <details> <summary>test/src/lib/LibDecimalFloat.gt.t.sol (2)</summary> `21-22`: **LGTM - Float passing by value instead of memory.** This new overload properly handles the packed `Float` values directly, aligning with the codebase refactor to standardize on a single packed representation. --- `26-29`: **Correct implementation of unpacking in test.** The function correctly unpacks Float values into their components before passing to the component-based implementation, allowing for proper comparison of packed versus unpacked implementations. </details> <details> <summary>test/src/lib/LibDecimalFloat.normalize.t.sol (2)</summary> `15-16`: **LGTM - Consistent with packed Float refactor.** The function signature properly handles the packed `Float` type by value rather than by memory reference. --- `20-28`: **Correct approach to testing normalized values.** The implementation properly unpacks the input Float, calls the component-wise normalization, and then compares against the packed version. The assertions are updated to correctly compare the normalized unpacked values with the corresponding components of the normalized packed Float. </details> <details> <summary>src/lib/parse/LibParseDecimalFloat.sol (6)</summary> `16-16`: **LGTM - Updated import for consolidated Float type.** Import has been properly updated to the new consolidated Float type from LibDecimalFloat. --- `20-20`: **LGTM - Return type updated to Float from PackedFloat.** Function signature properly updated to return the unified `Float` type rather than the previous `PackedFloat` type. --- `24-24`: **LGTM - Using Float.wrap for error cases.** Error cases now return a wrapped zero value for the packed Float type. --- `27-27`: **LGTM - Using LibDecimalFloat.pack consistently.** Return statement correctly uses the pack function to create a Float from its components. --- `128-128`: **LGTM - Simplified function signature.** Function now returns `Float` directly rather than `Float memory`, consistent with the packed representation. --- `135-138`: **Unused variable and simplified implementation.** The `cursor` variable is assigned but not used in the return statement. The code has been simplified to avoid unnecessary variable declarations. </details> <details> <summary>test/src/lib/LibDecimalFloat.eq.t.sol (2)</summary> `21-22`: **LGTM - Float passing by value.** This new overload properly handles the packed `Float` values directly, aligning with the codebase refactor. --- `25-28`: <details> <summary>✅ Verification successful</summary> **Function renamed from testEqMem to testEqPacked with correct implementation.** The test function has been renamed from `testEqMem` to `testEqPacked` to better reflect its use of packed Float values. The implementation correctly unpacks Float values into their components before passing to the component-based implementation. --- 🏁 Script executed: ```shell #!/bin/bash # Check for any references to the old function name that might need to be updated rg "testEqMem" --glob "*.sol" --glob "*.md" --glob "*.js" --glob "*.ts"Length of output: 71
Rename Verified: No Residual References to testEqMem
All occurrences of the old
testEqMemname have been removed, and the newtestEqPackedfunction correctly unpacks both Float values before delegating toeqExternal. Ready to merge.
test/src/lib/format/LibFormatDecimalFloat.t.sol (2)
20-22: Parameter type updated to use packed Float
The toString function has been correctly updated to use the packed Float type by value instead of a memory struct. This aligns with the global refactoring to use a single packed representation.
25-34: Unpacking strategy correctly implemented
The testFormatMem function now properly unpacks the Float value into its components before calling the external function. The use of explicit unpacking replaces the previous direct struct field access.
test/src/lib/LibDecimalFloat.abs.t.sol (2)
15-17: Parameter type updated to use packed Float
The absExternal function signature has been correctly updated to use the packed Float type by value instead of a memory struct. This aligns with the global refactoring to use a single packed representation.
19-28: Test function renamed and updated to use unpacking
The test has been appropriately renamed from testAbsMem to testAbsPacked to reflect the new underlying representation. The implementation correctly unpacks values using LibDecimalFloat.unpack() and compares the raw bytes using Float.unwrap() for proper equality testing.
test/src/lib/LibDecimalFloat.power10.t.sol (1)
15-18: Parameter type updated to use packed Float
The power10External function has been correctly updated to use the packed Float type by value instead of a memory struct. This aligns with the global refactoring to use a single packed representation.
test/src/lib/LibDecimalFloat.multiply.t.sol (1)
28-30: Parameter type updated to use packed Float
The multiplyExternal function has been correctly updated to use the packed Float type by value instead of a memory struct. This aligns with the global refactoring to use a single packed representation.
test/src/lib/LibDecimalFloat.power.t.sol (1)
20-22: Good addition of value-type interface.
The new powerExternal function that accepts and returns packed Float values aligns with the PR's goal of consolidating to a packed representation.
test/src/lib/LibDecimalFloat.divide.t.sol (1)
20-22: Good addition of value-type interface.
The new divideExternal function for packed Float values is consistent with the consolidation pattern used throughout the codebase.
test/src/lib/LibDecimalFloat.sub.t.sol (2)
20-22: Good addition of value-type interface.
The new subExternal function using the packed Float type is consistent with the overall refactoring approach.
25-38:
Details
❓ Verification inconclusive
Test failing with CoefficientOverflow.
The test implementation correctly unpacks and compares component values, but it's failing with CoefficientOverflow(-9.999e70, -34). This suggests the new packed implementation has stricter coefficient validation than before.
🏁 Script executed:
#!/bin/bash
# Find where CoefficientOverflow is defined and used
rg -A 3 "CoefficientOverflow" --glob "*.sol"
# Check the coefficient bit size limitations in LibDecimalFloat
rg "coefficient.*bit" --glob "*LibDecimalFloat.sol"
# Look for functions that might be handling coefficient bounds
rg "coefficientMax|maxCoefficient" --glob "*.sol"Length of output: 1117
🏁 Script executed:
#!/bin/bash
# Locate the subExternal overloads in LibDecimalFloat.sol
rg -A 5 "function subExternal" --glob "src/lib/LibDecimalFloat.sol"Length of output: 67
🏁 Script executed:
#!/bin/bash
# Search for subExternal definitions across the repo
rg -n "subExternal" --glob "*.sol"Length of output: 631
🏁 Script executed:
#!/bin/bash
# Search for pack and unpack functions in the repository
rg -n "pack(" --glob "*.sol"
rg -n "unpack(" --glob "*.sol"Length of output: 189
Verify coefficient bit‑size constraints causing overflow
The test value Float(-9.999e70, -34) appears to exceed the packed coefficient limit in LibDecimalFloat.sol. The implementation reserves 32 bits for the exponent (int32) and the remaining 224 bits for the signed coefficient, so any absolute coefficient > 2^223 − 1 will trigger CoefficientOverflow.
Please confirm the limits in:
src/error/ErrDecimalFloat.sol:error CoefficientOverflow(int256 signedCoefficient, int256 exponent);src/lib/LibDecimalFloat.sol: the packing logic and coefficient bound checks around where it reverts on overflow
Then adjust the test inputs or expectations (e.g. expect a revert) to align with these constraints.
test/src/lib/LibDecimalFloat.floor.t.sol (2)
15-17: Good addition of value-type interface.
The new floorExternal function that accepts and returns packed Float values aligns with the consolidated representation approach.
20-33: Test updated correctly.
This test has been properly updated to handle the packed Float type by unpacking values and comparing components individually. Unlike the other tests, this one doesn't appear to have any pipeline failures.
test/src/lib/LibDecimalFloat.add.t.sol (1)
20-22: LGTM - Function signature updated to use packed Float type
The function now accepts and returns the packed Float type instead of unpacked components, aligning with the codebase consolidation around packed representations.
test/src/lib/LibDecimalFloat.decimal.t.sol (2)
27-29: LGTM - Function updated to use packed Float type
Function signature properly updated to accept a packed Float value instead of separate components.
44-57: LGTM - Test correctly updated to use packed Float
This test properly unpacks the Float value and tests both packed and unpacked paths, with appropriate error handling.
test/src/lib/LibDecimalFloat.pack.t.sol (4)
4-4: LGTM - Import updated to remove reference to PackedFloat
The import statement has been correctly updated to remove PackedFloat and other unused imports, aligning with the overall refactoring.
9-11: LGTM - Function updated to use packed Float type
Function signature correctly updated to return Float directly instead of PackedFloat.
13-15: LGTM - Function updated to use packed Float type
Function parameter correctly updated to accept Float directly instead of PackedFloat.
37-39: LGTM - Test updated to use packed Float type
Test correctly updated to use the consolidated Float type instead of PackedFloat.
test/src/lib/LibDecimalFloat.decimalLossless.t.sol (3)
21-23: LGTM - Function updated to use packed Float type
Function signature properly updated to return packed Float directly.
33-35: LGTM - Function updated to use packed Float type
Function signature properly updated to accept packed Float by value.
53-63: LGTM - Test updated to use packed Float type
Test correctly updated to use the packed Float type, with proper unpacking and error handling.
| function testPowerMem(Float a, Float b) external { | ||
| (int256 signedCoefficientA, int256 exponentA) = a.unpack(); | ||
| (int256 signedCoefficientB, int256 exponentB) = b.unpack(); | ||
| try this.powerExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB) returns ( | ||
| int256 signedCoefficient, int256 exponent | ||
| ) { | ||
| Float memory float = this.powerExternal(a, b); | ||
| assertEq(signedCoefficient, float.signedCoefficient); | ||
| assertEq(exponent, float.exponent); | ||
| Float float = this.powerExternal(a, b); | ||
| (int256 signedCoefficientUnpacked, int256 exponentUnpacked) = float.unpack(); | ||
| assertEq(signedCoefficient, signedCoefficientUnpacked); | ||
| assertEq(exponent, exponentUnpacked); | ||
| } catch (bytes memory err) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Test failures due to ExponentOverflow condition.
The refactored test properly unpacks values and compares components individually, but it's failing with ExponentOverflow(6683, 8.745e18) according to pipeline failures. This suggests the packed implementation has stricter validation for exponent ranges.
🏁 Script executed:
#!/bin/bash
# Find more instances where ExponentOverflow occurs in tests
rg -A 2 "ExponentOverflow" --glob "*.sol"
# Check where exponent bounds are defined in the codebase
rg "EXPONENT_(MIN|MAX)" --glob "*.sol"Length of output: 9517
I need to inspect the exponent‐bound constants and the power/powerExternal implementations in src/lib/LibDecimalFloat.sol to confirm where the tighter overflow check is coming from. I'll gather those details next.
I need to inspect the powerExternal wrapper and the underlying power implementation to see where the exponent check is applied. Let me pull those definitions.
🏁 Script executed:
#!/bin/bash
# Show the external wrapper in the main library
rg -n -C 5 "powerExternal" src/lib/LibDecimalFloat.sol
# Show the internal power logic in the implementation
rg -n -C 5 "function power" src/lib/implementation/LibDecimalFloatImplementation.solLength of output: 141
Handle ExponentOverflow in testPowerMem
The power implementation now reverts with ExponentOverflow whenever the computed exponent (exponentA * signedCoefficientB) exceeds EXPONENT_MAX. Your testPowerMem must explicitly assert that revert and return early.
• File: test/src/lib/LibDecimalFloat.power.t.sol
In function testPowerMem, replace the empty catch block with:
} catch (bytes memory err) {
- // currently unhandled revert
+ // Expect ExponentOverflow for inputs that overflow the exponent range
+ assertEq(abi.decode(err, (bytes4)), ExponentOverflow.selector);
+ return;
}• (Optional) After the try/catch, add a revert() or fail() to catch any unexpected code paths.
📝 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.
| function testPowerMem(Float a, Float b) external { | |
| (int256 signedCoefficientA, int256 exponentA) = a.unpack(); | |
| (int256 signedCoefficientB, int256 exponentB) = b.unpack(); | |
| try this.powerExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB) returns ( | |
| int256 signedCoefficient, int256 exponent | |
| ) { | |
| Float memory float = this.powerExternal(a, b); | |
| assertEq(signedCoefficient, float.signedCoefficient); | |
| assertEq(exponent, float.exponent); | |
| Float float = this.powerExternal(a, b); | |
| (int256 signedCoefficientUnpacked, int256 exponentUnpacked) = float.unpack(); | |
| assertEq(signedCoefficient, signedCoefficientUnpacked); | |
| assertEq(exponent, exponentUnpacked); | |
| } catch (bytes memory err) { | |
| function testPowerMem(Float a, Float b) external { | |
| (int256 signedCoefficientA, int256 exponentA) = a.unpack(); | |
| (int256 signedCoefficientB, int256 exponentB) = b.unpack(); | |
| try this.powerExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB) returns ( | |
| int256 signedCoefficient, int256 exponent | |
| ) { | |
| Float float = this.powerExternal(a, b); | |
| (int256 signedCoefficientUnpacked, int256 exponentUnpacked) = float.unpack(); | |
| assertEq(signedCoefficient, signedCoefficientUnpacked); | |
| assertEq(exponent, exponentUnpacked); | |
| } catch (bytes memory err) { | |
| // Expect ExponentOverflow for inputs that overflow the exponent range | |
| assertEq(abi.decode(err, (bytes4)), ExponentOverflow.selector); | |
| return; | |
| } | |
| } |
| function testAddUnpacked(int256 signedCoefficientA, int256 exponentA, int256 signedCoefficientB, int256 exponentB) | ||
| external | ||
| { | ||
| try this.addExternal( | ||
| LibDecimalFloat.pack(signedCoefficientA, exponentA), LibDecimalFloat.pack(signedCoefficientB, exponentB) | ||
| ) returns (Float resultPacked) { | ||
| (int256 signedCoefficient, int256 exponent) = | ||
| this.addExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | ||
| assertEq(Float.unwrap(resultPacked), Float.unwrap(LibDecimalFloat.pack(signedCoefficient, exponent))); | ||
| } catch (bytes memory err) { | ||
| vm.expectRevert(err); | ||
| this.addExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix failing test to handle exponent overflow
This new test properly covers both packed and unpacked addition paths, but it's failing due to exponent overflow issues.
The test is failing with ExponentOverflow(0, 2.147e9) which suggests the exponent values need to be bounded.
function testAddUnpacked(int256 signedCoefficientA, int256 exponentA, int256 signedCoefficientB, int256 exponentB)
external
{
+ // Avoid exponent overflow errors
+ exponentA = bound(exponentA, EXPONENT_MIN, EXPONENT_MAX);
+ exponentB = bound(exponentB, EXPONENT_MIN, EXPONENT_MAX);
+ // Avoid coefficient overflow errors
+ signedCoefficientA = bound(signedCoefficientA, type(int224).min, type(int224).max);
+ signedCoefficientB = bound(signedCoefficientB, type(int224).min, type(int224).max);
+
try this.addExternal(
LibDecimalFloat.pack(signedCoefficientA, exponentA), LibDecimalFloat.pack(signedCoefficientB, exponentB)
) returns (Float resultPacked) {📝 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.
| function testAddUnpacked(int256 signedCoefficientA, int256 exponentA, int256 signedCoefficientB, int256 exponentB) | |
| external | |
| { | |
| try this.addExternal( | |
| LibDecimalFloat.pack(signedCoefficientA, exponentA), LibDecimalFloat.pack(signedCoefficientB, exponentB) | |
| ) returns (Float resultPacked) { | |
| (int256 signedCoefficient, int256 exponent) = | |
| this.addExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | |
| assertEq(Float.unwrap(resultPacked), Float.unwrap(LibDecimalFloat.pack(signedCoefficient, exponent))); | |
| } catch (bytes memory err) { | |
| vm.expectRevert(err); | |
| this.addExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | |
| } | |
| } | |
| function testAddUnpacked(int256 signedCoefficientA, int256 exponentA, int256 signedCoefficientB, int256 exponentB) | |
| external | |
| { | |
| // Avoid exponent overflow errors | |
| exponentA = bound(exponentA, EXPONENT_MIN, EXPONENT_MAX); | |
| exponentB = bound(exponentB, EXPONENT_MIN, EXPONENT_MAX); | |
| // Avoid coefficient overflow errors | |
| signedCoefficientA = bound(signedCoefficientA, type(int224).min, type(int224).max); | |
| signedCoefficientB = bound(signedCoefficientB, type(int224).min, type(int224).max); | |
| try this.addExternal( | |
| LibDecimalFloat.pack(signedCoefficientA, exponentA), | |
| LibDecimalFloat.pack(signedCoefficientB, exponentB) | |
| ) returns (Float resultPacked) { | |
| (int256 signedCoefficient, int256 exponent) = | |
| this.addExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | |
| assertEq( | |
| Float.unwrap(resultPacked), | |
| Float.unwrap(LibDecimalFloat.pack(signedCoefficient, exponent)) | |
| ); | |
| } catch (bytes memory err) { | |
| vm.expectRevert(err); | |
| this.addExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | |
| } | |
| } |
| function testAddPacked(Float a, Float b) external { | ||
| (int256 signedCoefficientA, int256 exponentA) = LibDecimalFloat.unpack(a); | ||
| (int256 signedCoefficientB, int256 exponentB) = LibDecimalFloat.unpack(b); | ||
| try this.addExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB) returns ( | ||
| int256 signedCoefficient, int256 exponent | ||
| ) { | ||
| Float memory resultMem = a.add(b); | ||
| assertEq(signedCoefficient, resultMem.signedCoefficient); | ||
| assertEq(exponent, resultMem.exponent); | ||
| Float resultMem = a.add(b); | ||
| assertEq(Float.unwrap(resultMem), Float.unwrap(LibDecimalFloat.pack(signedCoefficient, exponent))); | ||
| } catch (bytes memory err) { | ||
| vm.expectRevert(err); | ||
| a.add(b); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix failing test to handle coefficient overflow
The test is correctly adapting to use the packed Float type, but it's failing in CI due to coefficient overflow errors.
You need to bound the fuzzer inputs to prevent the CoefficientOverflow(1e70, -34) error. Add coefficient bounds to prevent values outside the valid range.
function testAddPacked(Float a, Float b) external {
+ // Avoid coefficient overflow errors by bounding inputs
+ vm.assume(Float.unwrap(a) != 0 && Float.unwrap(b) != 0);
+ try LibDecimalFloat.unpack(a) returns (int256 sc, int256 e) {
+ vm.assume(sc >= type(int224).min && sc <= type(int224).max);
+ } catch {
+ return;
+ }
+ try LibDecimalFloat.unpack(b) returns (int256 sc, int256 e) {
+ vm.assume(sc >= type(int224).min && sc <= type(int224).max);
+ } catch {
+ return;
+ }
(int256 signedCoefficientA, int256 exponentA) = LibDecimalFloat.unpack(a);
(int256 signedCoefficientB, int256 exponentB) = LibDecimalFloat.unpack(b);
try this.addExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB) returns (🏁 Script executed:
#!/bin/bash
# Check the boundaries of possible inputs and the implementation of pack
ast-grep --pattern $'function pack(int256 signedCoefficient, int256 exponent) internal pure returns (Float)'Length of output: 109
🏁 Script executed:
#!/bin/bash
# Locate the pack and unpack function definitions in the repository
rg "function pack" -n .
rg "function unpack" -n .Length of output: 593
🏁 Script executed:
#!/bin/bash
# Display the `pack` implementation and related constants
sed -n '280,360p' src/lib/LibDecimalFloat.sol
# Search for coefficient/exponent bounds definitions
rg -n "constant" src/lib/LibDecimalFloat.sol | grep -E "COEFFICIENT|EXPONENT"Length of output: 4748
🏁 Script executed:
#!/bin/bash
# Inspect the add test file to see external wrappers and current setup
sed -n '1,200p' test/src/lib/LibDecimalFloat.add.t.solLength of output: 8437
🏁 Script executed:
#!/bin/bash
# Locate and display the internal add implementation and normalize function
rg -n "function add" -n src/lib/LibDecimalFloat.sol
# Show the internal add and normalize implementations
sed -n '360,550p' src/lib/LibDecimalFloat.solLength of output: 9673
Update testAddPacked to correctly handle pack overflows
The current test catches only errors from the unpacked‐add path, but the CoefficientOverflow actually arises in the pack call inside a.add(b). Simply bounding the inputs to int224 isn’t sufficient to prevent an overflow on the resulting coefficient. Instead, pull the raw result from addExternal, then:
- If it fits the
int224/int32pack bounds, assert thata.add(b)matchespack(scRes, eRes). - Otherwise, explicitly
vm.expectRevertwhen callinga.add(b).
Pinpointed change in test/src/lib/LibDecimalFloat.add.t.sol:
function testAddPacked(Float a, Float b) external {
- (int256 signedCoefficientA, int256 exponentA) = LibDecimalFloat.unpack(a);
- (int256 signedCoefficientB, int256 exponentB) = LibDecimalFloat.unpack(b);
- try this.addExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB) returns (
- int256 signedCoefficient, int256 exponent
- ) {
- Float resultMem = a.add(b);
- assertEq(Float.unwrap(resultMem), Float.unwrap(LibDecimalFloat.pack(signedCoefficient, exponent)));
- } catch (bytes memory err) {
- vm.expectRevert(err);
- a.add(b);
- }
+ // Unpack inputs and get the raw add result
+ (int256 scA, int256 eA) = LibDecimalFloat.unpack(a);
+ (int256 scB, int256 eB) = LibDecimalFloat.unpack(b);
+ (int256 scRes, int256 eRes) = this.addExternal(scA, eA, scB, eB);
+
+ // If the result fits pack bounds, compare packed values,
+ // otherwise expect a CoefficientOverflow on pack.
+ if (int224(scRes) == scRes && int32(eRes) == eRes) {
+ Float resultMem = a.add(b);
+ assertEq(
+ Float.unwrap(resultMem),
+ Float.unwrap(LibDecimalFloat.pack(scRes, eRes))
+ );
+ } else {
+ bytes memory err = abi.encodeWithSelector(
+ LibDecimalFloat.CoefficientOverflow.selector,
+ scRes,
+ eRes
+ );
+ vm.expectRevert(err);
+ a.add(b);
+ }This ensures the test only asserts on valid packable results and correctly catches overflow reverts otherwise.
📝 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.
| function testAddPacked(Float a, Float b) external { | |
| (int256 signedCoefficientA, int256 exponentA) = LibDecimalFloat.unpack(a); | |
| (int256 signedCoefficientB, int256 exponentB) = LibDecimalFloat.unpack(b); | |
| try this.addExternal(signedCoefficientA, exponentA, signedCoefficientB, exponentB) returns ( | |
| int256 signedCoefficient, int256 exponent | |
| ) { | |
| Float memory resultMem = a.add(b); | |
| assertEq(signedCoefficient, resultMem.signedCoefficient); | |
| assertEq(exponent, resultMem.exponent); | |
| Float resultMem = a.add(b); | |
| assertEq(Float.unwrap(resultMem), Float.unwrap(LibDecimalFloat.pack(signedCoefficient, exponent))); | |
| } catch (bytes memory err) { | |
| vm.expectRevert(err); | |
| a.add(b); | |
| } | |
| } | |
| function testAddPacked(Float a, Float b) external { | |
| // Unpack inputs and get the raw add result | |
| (int256 scA, int256 eA) = LibDecimalFloat.unpack(a); | |
| (int256 scB, int256 eB) = LibDecimalFloat.unpack(b); | |
| (int256 scRes, int256 eRes) = this.addExternal(scA, eA, scB, eB); | |
| // If the result fits pack bounds, compare packed values, | |
| // otherwise expect a CoefficientOverflow on pack. | |
| if (int224(scRes) == scRes && int32(eRes) == eRes) { | |
| Float resultMem = a.add(b); | |
| assertEq( | |
| Float.unwrap(resultMem), | |
| Float.unwrap(LibDecimalFloat.pack(scRes, eRes)) | |
| ); | |
| } else { | |
| bytes memory err = abi.encodeWithSelector( | |
| LibDecimalFloat.CoefficientOverflow.selector, | |
| scRes, | |
| eRes | |
| ); | |
| vm.expectRevert(err); | |
| a.add(b); | |
| } | |
| } |
| function testMinXYEqual(int256 signedCoefficientX, int256 exponentX) external pure { | ||
| int256 signedCoefficientY = signedCoefficientX; | ||
| int256 exponentY = exponentX; | ||
| (int256 signedCoefficientZ, int256 exponentZ) = | ||
| LibDecimalFloat.min(signedCoefficientX, exponentX, signedCoefficientY, exponentY); | ||
| assertEq(signedCoefficientZ, signedCoefficientX, "signedCoefficientZ != signedCoefficientX"); | ||
| assertEq(exponentZ, exponentX, "exponentZ != exponentX"); | ||
| assertEq(signedCoefficientZ, signedCoefficientY, "signedCoefficientZ != signedCoefficientY"); | ||
| assertEq(exponentZ, exponentY, "exponentZ != exponentY"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Component‑level assertions are correct, but you lost the structural check
The previous version compared packed Float values directly.
Now that you unpack, you could still assert that repacking yields the same original value to ensure the round‑trip invariance:
assertEq(exponentZ, exponentY, "exponentZ != exponentY");
+assertEq(
+ LibDecimalFloat.pack(signedCoefficientZ, exponentZ),
+ LibDecimalFloat.pack(signedCoefficientX, exponentX),
+ "packed result != packed input"
+);📝 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.
| function testMinXYEqual(int256 signedCoefficientX, int256 exponentX) external pure { | |
| int256 signedCoefficientY = signedCoefficientX; | |
| int256 exponentY = exponentX; | |
| (int256 signedCoefficientZ, int256 exponentZ) = | |
| LibDecimalFloat.min(signedCoefficientX, exponentX, signedCoefficientY, exponentY); | |
| assertEq(signedCoefficientZ, signedCoefficientX, "signedCoefficientZ != signedCoefficientX"); | |
| assertEq(exponentZ, exponentX, "exponentZ != exponentX"); | |
| assertEq(signedCoefficientZ, signedCoefficientY, "signedCoefficientZ != signedCoefficientY"); | |
| assertEq(exponentZ, exponentY, "exponentZ != exponentY"); | |
| } | |
| function testMinXYEqual(int256 signedCoefficientX, int256 exponentX) external pure { | |
| int256 signedCoefficientY = signedCoefficientX; | |
| int256 exponentY = exponentX; | |
| (int256 signedCoefficientZ, int256 exponentZ) = | |
| LibDecimalFloat.min(signedCoefficientX, exponentX, signedCoefficientY, exponentY); | |
| assertEq(signedCoefficientZ, signedCoefficientX, "signedCoefficientZ != signedCoefficientX"); | |
| assertEq(exponentZ, exponentX, "exponentZ != exponentX"); | |
| assertEq(signedCoefficientZ, signedCoefficientY, "signedCoefficientZ != signedCoefficientY"); | |
| assertEq(exponentZ, exponentY, "exponentZ != exponentY"); | |
| + assertEq( | |
| + LibDecimalFloat.pack(signedCoefficientZ, exponentZ), | |
| + LibDecimalFloat.pack(signedCoefficientX, exponentX), | |
| + "packed result != packed input" | |
| + ); | |
| } |
| function unpack(Float float) internal pure returns (int256 signedCoefficient, int256 exponent) { | ||
| uint256 mask = type(uint224).max; | ||
| assembly ("memory-safe") { | ||
| signedCoefficient := signextend(27, and(packed, mask)) | ||
| exponent := sar(0xe0, packed) | ||
| signedCoefficient := signextend(27, and(float, mask)) | ||
| exponent := sar(0xe0, float) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
signextend(27, …) is correct but deserves a comment
A quick note that byte 27 corresponds to bit 223 (the sign bit of the 224‑bit coefficient) will save future maintainers from recalculating the magic number.
// 27 = byte index of bit 223 (sign bit for 224‑bit two’s‑complement)
signedCoefficient := signextend(27, and(float, COEFF_MASK));| library LibDecimalFloat { | ||
| using LibDecimalFloat for Float; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
using LibDecimalFloat for Float; inside the same library is redundant
Because the functions are defined in this library, callers only need to write
using LibDecimalFloat for Float;where they consume the library.
Keeping it here does nothing inside the library itself and slightly increases byte‑code size.
There was a problem hiding this comment.
Actionable comments posted: 19
🔭 Outside diff range comments (3)
test/src/lib/implementation/LibDecimalFloatImplementation.inv.t.sol (1)
1-37: 🧹 Nitpick (assertive)Test coverage for inv function looks good but could be expanded
The implementation properly tests the
invfunction against a reference implementation and includes gas benchmarks. The fuzzing with appropriate bounds ensures comprehensive testing.Consider adding a few specific test cases with predefined inputs and expected outputs to make the test more robust and to provide clearer documentation of expected behavior for common cases.
+ function testSpecificInvCases() external { + // Test case 1: Inverse of 2 + (int256 outputSignedCoefficient, int256 outputExponent) = LibDecimalFloatImplementation.inv(2, 0); + assertEq(outputSignedCoefficient, 5, "coefficient"); + assertEq(outputExponent, -1, "exponent"); + + // Test case 2: Inverse of 0.5 + (outputSignedCoefficient, outputExponent) = LibDecimalFloatImplementation.inv(5, -1); + assertEq(outputSignedCoefficient, 2, "coefficient"); + assertEq(outputExponent, 0, "exponent"); + + // Test case 3: Inverse of -10 + (outputSignedCoefficient, outputExponent) = LibDecimalFloatImplementation.inv(-1, 1); + assertEq(outputSignedCoefficient, -1, "coefficient"); + assertEq(outputExponent, -1, "exponent"); + }test/src/lib/LibDecimalFloat.eq.t.sol (1)
24-34: 🧹 Nitpick (assertive)Good defensive parity check – tiny style nit
eqExternal(the packed variant) is alreadypure; marking the whole test
functiontestEqPackedaspureas well tightens the contract and gives the
optimizer more room to work. It currently uses only pure operations.-function testEqPacked(Float a, Float b) external { +function testEqPacked(Float a, Float b) external pure {src/lib/implementation/LibDecimalFloatImplementation.sol (1)
693-714: 🧹 Nitpick (assertive)
compareRescaleuses wrapped‑overflow sentinel; clarify intentThe wrapped multiplication
rescaled = signedCoefficientA * scaleintentionally
allows overflow and detects it viarescaled / scale != signedCoefficientA.
This is clever but brittle—future Solidity versions may change wrap semantics.
A safer pattern is the checked‐mul helper fromOpenZeppelinwhich reverts on
overflow, letting you fall back to the “infinitely larger” branch in the
catchclause.
♻️ Duplicate comments (4)
src/lib/parse/LibParseDecimalFloat.sol (1)
90-90: Reduced precision loss threshold from 77 to 67 digitsThe precision loss threshold for fractional parsing has been reduced from 77 to 67 digits. This change might cause more cases to be rejected as precision loss.
#!/bin/bash # Check for existing tests that rely on the previous threshold value rg -A 2 -B 2 "77" --glob "test/*.sol" | grep -i "precision"test/src/lib/format/LibFormatDecimalFloat.t.sol (1)
37-46: Fix CoefficientOverflow in testFormatDecimalRoundTripThe function was renamed from testRoundTrip and updated to use packed Float, but the value bound may still be too large, potentially causing CoefficientOverflow errors as noted in previous reviews.
#!/bin/bash # Check if there are any CoefficientOverflow errors occurring in tests # Look at error definitions and implementation to understand coefficient limits rg -A 5 "CoefficientOverflow" # Check if test failures are occurring related to this error rg -A 5 "testFormatDecimalRoundTrip.*fail" -g "*.json"Consider limiting the bound to a smaller value to prevent overflow:
- value = bound(value, 0, type(uint256).max / 10); + // Ensure value doesn't exceed coefficient capacity for packed float + value = bound(value, 0, 1e40);src/lib/LibDecimalFloat.sol (2)
88-89: Redundantusing … forinside the defining library (repeat)This was called out in a previous review and is still present.
Declaringusing LibDecimalFloat for Float;inside LibDecimalFloat has no effect and only inflates the byte‑code.
343-345: Add the explanatory comment forsignextend(27, …)Prior feedback suggested documenting why byte 27 is chosen.
This magic constant still lacks context and forces every reader to recalculate where bit 223 lives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (35)
src/lib/LibDecimalFloat.sol(13 hunks)src/lib/implementation/LibDecimalFloatImplementation.sol(4 hunks)src/lib/parse/LibParseDecimalFloat.sol(3 hunks)test/abstract/LogTest.sol(1 hunks)test/lib/LibDecimalFloatSlow.sol(2 hunks)test/src/lib/LibDecimalFloat.abs.t.sol(1 hunks)test/src/lib/LibDecimalFloat.add.t.sol(2 hunks)test/src/lib/LibDecimalFloat.divide.t.sol(2 hunks)test/src/lib/LibDecimalFloat.eq.t.sol(3 hunks)test/src/lib/LibDecimalFloat.floor.t.sol(2 hunks)test/src/lib/LibDecimalFloat.frac.t.sol(2 hunks)test/src/lib/LibDecimalFloat.gt.t.sol(1 hunks)test/src/lib/LibDecimalFloat.inv.t.sol(1 hunks)test/src/lib/LibDecimalFloat.log10.t.sol(1 hunks)test/src/lib/LibDecimalFloat.lt.t.sol(1 hunks)test/src/lib/LibDecimalFloat.max.t.sol(1 hunks)test/src/lib/LibDecimalFloat.min.t.sol(1 hunks)test/src/lib/LibDecimalFloat.minus.t.sol(2 hunks)test/src/lib/LibDecimalFloat.mixed.t.sol(1 hunks)test/src/lib/LibDecimalFloat.multiply.t.sol(2 hunks)test/src/lib/LibDecimalFloat.normalize.t.sol(0 hunks)test/src/lib/LibDecimalFloat.pack.t.sol(1 hunks)test/src/lib/LibDecimalFloat.power.t.sol(3 hunks)test/src/lib/LibDecimalFloat.power10.t.sol(1 hunks)test/src/lib/LibDecimalFloat.sub.t.sol(1 hunks)test/src/lib/format/LibFormatDecimalFloat.t.sol(2 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.add.t.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.divide.t.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.eq.t.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.inv.t.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.log10.t.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.minus.t.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.multiply.t.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.power10.t.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.sub.t.sol(1 hunks)
💤 Files with no reviewable changes (1)
- test/src/lib/LibDecimalFloat.normalize.t.sol
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: git-clean
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
🔇 Additional comments (91)
test/abstract/LogTest.sol (1)
4-5: Good addition of console logging facilities.The re-export of
console2from forge-std's Test.sol enhances debugging capabilities across test contracts. This change aligns well with the broader refactoring effort, providing a convenient way for test contracts to access logging functionality without additional imports.test/lib/LibDecimalFloatSlow.sol (2)
5-8: Import and using directive properly align with Float refactoring.The import of the
Floattype and addition of the using directive are consistent with the codebase refactoring to use the packedFloattype alias.
43-43: Implementation correctly updated to use core implementation library.The
invSlowfunction now properly callsLibDecimalFloatImplementation.divideinstead of the previous implementation, consistent with the architectural changes to delegate core arithmetic operations to the implementation library.test/src/lib/LibDecimalFloat.mixed.t.sol (3)
4-10: Proper setup for using the packed Float type.Correctly imports the
Floattype and sets up the using directive to enable method-based operations on the Float type.
14-17: Test refactored correctly to use packed Float operations.The division operation has been updated to use the packed Float representation and method-based approach, consistent with the broader refactoring effort.
21-22: Multiply operation properly updated to use packed Float.The multiplication step correctly uses the method-based approach on packed Float values, maintaining the logical equivalence with the previous implementation while aligning with the new architecture.
test/src/lib/implementation/LibDecimalFloatImplementation.minus.t.sol (1)
1-26: Well-structured test for the minus operation.This new test contract effectively verifies that the
minusoperation (negation) produces the same results as subtracting the value from zero. The test bounds input exponents appropriately to prevent overflow conditions and includes clear assertions to validate both the coefficient and exponent outputs.The test complements the other implementation tests and helps ensure the correctness of the core arithmetic operations in the refactored codebase.
test/src/lib/implementation/LibDecimalFloatImplementation.log10.t.sol (1)
1-53: Well-structured test implementation for log10 functionalityThis is a good implementation of tests for the
log10function. The test coverage is comprehensive, testing exact powers of 10, exact lookup values, interpolated lookups, and values less than 1. The helper functioncheckLog10effectively centralizes the verification logic and includes gas measurement for performance analysis.src/lib/parse/LibParseDecimalFloat.sol (3)
16-16: Updated import to use the packed Float typeThe import has been updated to use the packed
Floattype alias fromLibDecimalFloat.sol, which aligns with the broader refactoring of decimal float handling in the codebase.
20-28: Refactored to use the packed Float typeThe function signature has been updated to return the packed
Floattype instead ofPackedFloat, and the error handling and return path now use the packed type. This aligns with the broader refactoring of decimal float handling in the codebase.
128-139: Simplified function signature to use packed Float typeThe function signature has been simplified by removing named return variables and now returns a tuple
(bytes4, Float)instead of(bytes4, Float memory). It also directly returns the packed float from the parsed coefficient and exponent.test/src/lib/LibDecimalFloat.minus.t.sol (4)
4-5: Updated import and simplified dependencyThe import has been updated to use the packed
Floattype alias fromLibDecimalFloat.soland removed unnecessary constants. This aligns with the broader refactoring of decimal float handling in the codebase.
12-14: Redirected external function to implementation libraryThe
minusExternalfunction now delegates toLibDecimalFloatImplementation.minusinstead ofLibDecimalFloat.minus, consistent with the architectural changes where core arithmetic operations are moved to the implementation library.
16-18: Updated function signature to use packed Float typeThe function signature has been updated to take and return
Floatby value rather than memory references, reflecting the new packed type usage.
20-32: Refactored test to handle packed Float valuesThe test function has been renamed from
testMinusMemtotestMinusPackedand updated to work with packedFloatvalues. It explicitly unpacks and repacks values for comparison.test/src/lib/implementation/LibDecimalFloatImplementation.sub.t.sol (1)
11-49: Well-structured tests for the subtraction implementationThe implementation tests for the subtraction operation are thorough and well-designed:
- The main test verifies that subtraction is equivalent to addition with a negated second coefficient
- A separate test handles the edge case with the minimum signed value
- Bounds are properly set for exponents to prevent overflow issues
- The test coverage is comprehensive with proper documentation
test/src/lib/format/LibFormatDecimalFloat.t.sol (2)
20-21: Changed signature to use packed Float type instead of memory structThe function now correctly accepts a packed Float value instead of a memory struct, aligning with the consolidation to packed Float types.
25-33: Updated to unpack Float before comparisonThe test now appropriately unpacks the Float value to get its components before calling the external function, which is consistent with the new packed Float representation approach.
test/src/lib/LibDecimalFloat.pack.t.sol (4)
4-4: Updated imports to include Float type and error definitionsThe import now correctly includes the Float type, ExponentOverflow error, and EXPONENT_MAX constant from LibDecimalFloat.
9-11: Updated packing function to use new packLossy APIThe function signature has been improved to return a tuple with the packed Float value and a boolean indicating if the packing was lossless, which provides better error handling.
13-15: Updated unpacking function to use new Float typeThe function signature has been correctly updated to accept a packed Float value instead of the previous type, aligning with the consolidation approach.
19-22: Updated round-trip test to use the new packed Float typeThe test now correctly:
- Uses packLossy instead of the previous packing method
- Captures the lossless flag and asserts it's true
- Unpacks the Float to verify coefficient and exponent values
These changes properly validate the round-trip functionality with the new API.
test/src/lib/LibDecimalFloat.frac.t.sol (3)
11-13: Simplified test to use Float type directlyThe test has been simplified to directly use the Float type and its frac() method instead of external function calls with separate parameters.
16-19: Updated helper function to use packed Float representationThe checkFrac helper function now:
- Creates a packed Float using packLossless
- Calls the frac() method on the Float directly
- Unpacks the result to verify against expected values
This is consistent with the overall refactoring to use packed Float values throughout the codebase.
73-74: Updated gas test functions to use packed FloatAll gas benchmark functions have been correctly updated to use the new packed Float representation with packLossless and the frac() method.
Also applies to: 78-79, 83-84
test/src/lib/LibDecimalFloat.divide.t.sol (4)
4-5: Import changes align with the consolidated Float approach.The imports have been updated to include both the refactored
LibDecimalFloatwith the packedFloattype and the new implementation library. This matches the architectural shift toward a packed representation.
17-17: Delegation to implementation library is properly implemented.The division logic is now delegated to
LibDecimalFloatImplementation, which is consistent with the new architecture that separates interface from implementation.
20-20: Function signature updated to use packed Float by value.The external division function now accepts
Floatparameters by value instead of memory reference, aligning with the packedFloattype alias approach.
24-37: Test function properly updated to use packed representation.The test has been renamed from
testDivideMemtotestDividePackedand now:
- Correctly unpacks the input Floats using
.unpack()- Tests both the primitive and packed implementations
- Properly compares results using unpacked components
- Maintains equivalent error handling
This ensures that both implementation approaches produce consistent results.
test/src/lib/implementation/LibDecimalFloatImplementation.add.t.sol (7)
1-29: Well-structured basic addition tests.The test contract starts with clear, well-commented test cases for fundamental addition scenarios including zero addition, normalization handling, and boundary cases. The tests use descriptive names and include clear expected outcomes in comments.
46-58: Comprehensive normalization tests.These tests verify that addition correctly handles normalization with both non-normalized and pre-normalized inputs, ensuring the implementation maintains numerical precision regardless of input format.
76-82: Gas efficiency tests included.Good practice to include gas consumption tests for common operations like adding zero and adding one. This will help identify any performance regressions in future changes.
84-97: Robust fuzz testing approach.The test properly bounds exponent inputs to prevent overflow/underflow and uses fuzzing to verify the addition implementation never reverts within valid input ranges. This is important for ensuring stability of the library.
99-124: Thorough testing of precision behavior.This fuzz test verifies an important property: when adding a small number to a large one (beyond a certain exponent difference threshold), the result should equal the larger number. This validates proper handling of precision limitations.
126-178: Comprehensive examples with expected precision loss.These examples clearly demonstrate and verify the expected behavior for precision loss cases, validating the implementation handles edge cases correctly. The tests cover both positive and negative numbers and different exponent combinations.
180-221: Property-based testing for addition edge cases.These tests verify two important properties:
- Adding numbers with the same exponent correctly adds their coefficients
- Adding zero to any non-zero number returns the non-zero number unchanged
Both properties are tested with thorough input variations and edge cases.
test/src/lib/implementation/LibDecimalFloatImplementation.power10.t.sol (4)
8-20: Well-structured helper for power10 testing.The
checkPower10helper function improves test readability and maintenance by centralizing the assertion logic, retrieving the log tables, and providing descriptive error messages.
22-30: Tests for basic power10 calculations.These tests verify the core functionality for basic integer exponents, ensuring the implementation returns correctly normalized results.
32-57: Comprehensive tests for exact lookup values.The tests verify both integer and fractional powers of 10, covering various lookup table entries. The inclusion of negative inputs (line 56) is particularly valuable for edge case validation.
59-64: Tests for interpolated values verify approximation quality.These tests ensure the library correctly handles fractional powers that require interpolation between lookup table entries, including a test with a very large exponent (line 63).
test/src/lib/implementation/LibDecimalFloatImplementation.multiply.t.sol (4)
14-53: Thorough testing of basic multiplication cases.The tests cover all fundamental cases (zero multiplication, multiplying by one) with various exponent combinations. Proper validation against the normalized zero constants ensures consistent behavior.
55-80: Comprehensive tests for number multiplication.Tests verify exact multiplication results for specific inputs (like 123456789 * 987654321) both with and without exponents, as well as power-of-ten calculations with positive and negative exponents.
82-90: Gas consumption tests included.Including gas tests for common operations (multiplying by zero and one) is good practice for identifying potential performance regressions.
92-107: Strong property-based validation against reference implementation.This comprehensive fuzz test:
- Bounds inputs to valid exponent ranges
- Compares results against a reference implementation (
LibDecimalFloatSlow.multiplySlow)- Verifies both coefficient and exponent correctness
This approach provides high confidence in the implementation's correctness across a wide range of inputs.
test/src/lib/LibDecimalFloat.multiply.t.sol (4)
4-4: Good refactoring of imports.Simplified the import to only include essential constants and types from LibDecimalFloat, making dependencies clearer.
17-17: Implementation delegation looks good.Updated to call LibDecimalFloatImplementation.multiply instead of direct functionality, aligning with the codebase's refactoring toward centralized implementation.
20-20: Good signature refactoring.Function now takes Float parameters by value rather than memory references, consistent with the shift to packed representation.
24-34: Test function properly updated for packed representation.Renamed from testMultiplyMem to testMultiplyPacked and correctly updated to unpack Float inputs before calling the external function, then compare unpacked results.
test/src/lib/implementation/LibDecimalFloatImplementation.eq.t.sol (5)
1-7: Well-organized imports and license declaration.Clean organization with proper license declaration and focused imports.
9-27: Comprehensive gas benchmark tests.Good coverage of edge cases including different signs, zero values, and exponent overflow scenarios.
30-52: Thorough property-based equality tests.The test validates the mathematical relationship between equal decimal floats with different representations.
55-93: Excellent coverage of equality edge cases.Tests handle different values, same values with different exponents, zero values, and reflexivity properties.
95-103: Good reference test implementation.Comparing against a reference implementation ensures correctness of the equality function.
test/src/lib/LibDecimalFloat.floor.t.sol (3)
11-12: Test properly updated for packed representation.Updated to use the packed Float type for testing, consistent with the codebase refactoring approach.
16-17: Helper function correctly adjusted.The checkFloor function now packs inputs into Float structs before applying floor operations, maintaining test integrity while using the new representation.
79-80: Gas benchmark tests properly updated.Gas benchmark tests have been updated to use the new Float struct representation consistently.
Also applies to: 84-85, 89-90
test/src/lib/LibDecimalFloat.inv.t.sol (3)
5-6: Appropriate import updates.Imports updated to reference the correct modules and types for the refactored implementation.
12-12: Implementation delegation looks good.Updated to call LibDecimalFloatImplementation.inv, aligning with the centralized implementation pattern.
15-15: Good signature refactoring.Function now takes the Float parameter by value rather than a memory reference, consistent with the packed representation approach.
test/src/lib/LibDecimalFloat.log10.t.sol (4)
5-5: Added dependency on LibDecimalFloatImplementation.This implements the architectural change to delegate core operations to the implementation library.
13-13: Delegated log10 to LibDecimalFloatImplementation.The function now calls the implementation library version of log10 instead of the original library.
16-18: Updated signature to use packed Float by value.The function now takes and returns Float by value (stack) rather than memory reference, aligning with the broader shift to the packed representation.
20-29: Refactored test to use packed Float values.The renamed
testLog10Packedfunction properly:
- Unpacks the Float type into components
- Tests the implementation function with unpacked components
- Tests the Float interface directly
- Compares the unpacked components for equality
This provides good test coverage for the equivalence between the packed and unpacked interfaces.
test/src/lib/LibDecimalFloat.min.t.sol (4)
11-14: Simplified function signature to use packed Float by value.Changed function to accept and operate on the packed
Floattype directly rather than a memory reference, aligning with the architectural shift away from memory-based structs.
17-21: Updated function parameters to use packed Float.Changed function to accept packed
Floatvalues and removed memory references for local variables, aligning with the architectural change.
24-29: Simplified function to use packed Float values.Changed test to operate on packed values directly, while maintaining the same test logic and assertions.
32-37: Updated function to use packed Float values while maintaining test logic.Changed to use the packed
Floattype directly, keeping the core test logic intact.test/src/lib/LibDecimalFloat.max.t.sol (4)
11-14: Updated function to use packed Float type.Function now receives and handles Float as a value type rather than a memory reference.
17-21: Refactored to use packed Float values.Updated the function to use the new packed Float representation, maintaining the same test assertions.
24-29: Updated function parameters and variables to use packed Float.Simplified to use the packed representation directly, keeping the same test logic.
40-45: Updated to use packed Float values.Function now uses the packed Float type directly, maintaining the same test semantics.
test/src/lib/LibDecimalFloat.sub.t.sol (3)
17-17: Delegated subtraction logic to LibDecimalFloatImplementation.Changed to call the implementation library directly for the core logic, aligning with the architectural shift.
20-22: Updated function signature to use packed Float by value.Changed to take packed Float values and return a packed Float, removing memory references to align with the architectural change.
24-34: Refactored test to use packed Float values.The test now:
- Takes packed Float parameters
- Unpacks them for the primitive component test
- Calls both APIs (implementation and Float interface)
- Verifies component-level equivalence
This approach properly tests the compatibility between the packed interface and primitive component implementation.
test/src/lib/LibDecimalFloat.abs.t.sol (3)
12-19: LGTM: Non-negative absolute value test looks correct.The test properly uses the new packed
Floatapproach, creating and validating values with appropriate bounds. The coefficient is bounded totype(int224).maxwhich matches the expected internal storage format.
22-29: LGTM: Negative value handling looks good.The test correctly bounds the coefficient between
type(int224).min + 1and-1, which avoids the special case of the minimum int224 value that's tested separately. It then properly validates that the absolute value negates the coefficient while preserving the exponent.
32-39: LGTM: Special case handling for minimum value is correct.The test handles the special edge case where negating
type(int224).minwould overflow int224. The implementation correctly shifts the result one order of magnitude (dividing by 10 and incrementing the exponent).test/src/lib/implementation/LibDecimalFloatImplementation.divide.t.sol (5)
12-25: Well-structured test helper for division validation.The
checkDivisionhelper function properly verifies both the coefficient and exponent parts of division results, making the test cases more readable and maintainable.
27-39: LGTM: Gas benchmark for repeated division operations.This test provides valuable gas usage insights by executing the same division operation multiple times in sequence.
41-75: LGTM: Good coverage of basic division cases.These tests cover important division scenarios including:
- Normal division (1/3)
- Negative division (-1/3)
- Large decimals (1e18/3)
- Various magnitude combinations
77-94: LGTM: Comprehensive compound division test.The test validates that (1/9) divided by (1/3) correctly equals 1/3, demonstrating that division works correctly with results of previous divisions.
96-125: Excellent fuzz test for division with varying scales.This fuzz test systematically tests division across different scales while ensuring precision is maintained. The nested loops verify that all combinations of scales produce normalized results with the expected precision.
test/src/lib/LibDecimalFloat.power.t.sol (3)
7-7: Added implementation import looks good.Adding the import for
LibDecimalFloatImplementationaligns with the new architecture where core operations are implemented in this separate library.
14-32: Well-refactored power test function.The
checkPowerfunction has been properly updated to:
- Create packed
Floatinstances usingpackLossless- Call power through the
Floatmethod with log tables- Unpack the result for assertion against expected values
- Maintain gas measurements for performance tracking
This aligns well with the new architecture.
39-50: Properly updated round-trip test.The
checkRoundTripfunction has been correctly refactored to use the packedFloatapproach, usingpackLosslessfor inputs and the appropriate Float methods for power, inverse, division, subtraction, and comparison operations.test/src/lib/LibDecimalFloat.add.t.sol (4)
4-8: Good import structure with specific constants.The import has been improved to specifically include the
ADD_MAX_EXPONENT_DIFFconstant from the implementation library, which provides better context for the allowed exponent difference in addition operations.
15-21: LGTM: Updated external test function delegates to implementation.The
addExternalfunction now correctly delegates toLibDecimalFloatImplementation.addfor the unpacked parameters, aligning with the architecture update.
23-25: LGTM: Simplified external function signature.The packed
addExternalfunction now returns aFloatdirectly instead of a tuple, which simplifies the interface and better reflects the updated library architecture.
27-41:Details
❌ Incorrect review comment
Handle overflow conditions in the test.
The test does not explicitly handle potential coefficient or exponent overflow conditions. According to past review comments, this test was failing due to overflow issues.
Consider adding explicit bounds checks for inputs:
- Bound coefficients to
type(int224).min/max- Bound exponents appropriately
- Handle packing overflow cases explicitly
🏁 Script executed:
#!/bin/bash # Find instances of overflow handling in similar test files rg -A 5 "overflow|bound\(" --glob "*.t.sol" | grep -A 2 "bound"Length of output: 19080
Overflow handling already covered by try/catch in testAddPacked
The
testAddPackedfunction explicitly wraps the low‑level and high‑leveladdExternalcalls in atry { … } catch (bytes memory err) { vm.expectRevert(err); … }block, so both successful additions and overflow‐induced reverts are already being tested. No additionalbound(…)constraints are needed here.Likely an incorrect or invalid review comment.
test/src/lib/LibDecimalFloat.eq.t.sol (1)
37-41: Excellent cross‑relation assertionLinking
eq,lt,gtthe way you did guards against subtle ordering bugs.src/lib/LibDecimalFloat.sol (1)
291-309: 🧹 Nitpick (assertive)
while (int224(signedCoefficient) != signedCoefficient)may hot‑loop on huge inputs
signedCoefficientcan be a 256‑bit value supplied by users ofpackLossy.
If it is close to2²⁵⁵thewhileloop will execute up to 32 iterations (one per dropped decimal), which is fine.
But for pathological values (e.g.10**255) the loop will iterate ~ 255 times and burn > 60 k gas.Consider short‑circuiting with a logarithmic approach:
uint256 digits = _numDigits(signedCoefficient); if (digits > 224) { uint256 trim = digits - 224; uint256 step = trim > 1 ? trim - 1 : 1; signedCoefficient /= 10 ** step; exponent += int256(step); }That reduces the worst‑case iterations to two.
Likely an incorrect or invalid review comment.
| function testInvMem(Float float) external { | ||
| (int256 signedCoefficient, int256 exponent) = float.unpack(); | ||
| try this.invExternal(signedCoefficient, exponent) returns ( | ||
| int256 signedCoefficientResult, int256 exponentResult | ||
| ) { | ||
| Float memory floatInv = this.invExternal(float); | ||
| assertEq(signedCoefficient, floatInv.signedCoefficient); | ||
| assertEq(exponent, floatInv.exponent); | ||
| Float floatInv = this.invExternal(float); | ||
| (int256 signedCoefficientResultUnpacked, int256 exponentResultUnpacked) = floatInv.unpack(); | ||
| assertEq(signedCoefficientResultUnpacked, signedCoefficientResult); | ||
| assertEq(exponentResultUnpacked, exponentResult); | ||
| } catch (bytes memory err) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Test function needs name update.
While the test logic has been correctly updated to work with packed Float values, the function name "testInvMem" suggests it's testing memory-based implementation.
Rename the function to better reflect its purpose:
-function testInvMem(Float float) external {
+function testInvPacked(Float float) external {📝 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.
| function testInvMem(Float float) external { | |
| (int256 signedCoefficient, int256 exponent) = float.unpack(); | |
| try this.invExternal(signedCoefficient, exponent) returns ( | |
| int256 signedCoefficientResult, int256 exponentResult | |
| ) { | |
| Float memory floatInv = this.invExternal(float); | |
| assertEq(signedCoefficient, floatInv.signedCoefficient); | |
| assertEq(exponent, floatInv.exponent); | |
| Float floatInv = this.invExternal(float); | |
| (int256 signedCoefficientResultUnpacked, int256 exponentResultUnpacked) = floatInv.unpack(); | |
| assertEq(signedCoefficientResultUnpacked, signedCoefficientResult); | |
| assertEq(exponentResultUnpacked, exponentResult); | |
| } catch (bytes memory err) { | |
| function testInvPacked(Float float) external { | |
| (int256 signedCoefficient, int256 exponent) = float.unpack(); | |
| try this.invExternal(signedCoefficient, exponent) returns ( | |
| int256 signedCoefficientResult, int256 exponentResult | |
| ) { | |
| Float floatInv = this.invExternal(float); | |
| (int256 signedCoefficientResultUnpacked, int256 exponentResultUnpacked) = floatInv.unpack(); | |
| assertEq(signedCoefficientResultUnpacked, signedCoefficientResult); | |
| assertEq(exponentResultUnpacked, exponentResult); | |
| } catch (bytes memory err) { |
| internal | ||
| pure | ||
| returns (int256, int256) | ||
| { | ||
| unchecked { | ||
| (signedCoefficientA, exponentA) = LibDecimalFloatImplementation.normalize(signedCoefficientA, exponentA); | ||
| (signedCoefficientB, exponentB) = LibDecimalFloatImplementation.normalize(signedCoefficientB, exponentB); | ||
|
|
||
| int256 signedCoefficient = (signedCoefficientA * 1e38) / signedCoefficientB; | ||
| int256 exponent = exponentA - exponentB - 38; | ||
| return (signedCoefficient, exponent); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing explicit divisor‑zero check
divide relies on EVM’s / to revert on zero‑divisor. Explicitly reverting
with a domain‑specific error (e.g. DivisionByZero()), as you did elsewhere
(Log10Zero), makes the API clearer and prevents silent 0 results if a
future refactor accidentally uses unchecked {} division.
- int256 signedCoefficient = (signedCoefficientA * 1e38) / signedCoefficientB;
+ if (signedCoefficientB == 0) revert DivisionByZero();
+ int256 signedCoefficient = (signedCoefficientA * 1e38) / signedCoefficientB;📝 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.
| internal | |
| pure | |
| returns (int256, int256) | |
| { | |
| unchecked { | |
| (signedCoefficientA, exponentA) = LibDecimalFloatImplementation.normalize(signedCoefficientA, exponentA); | |
| (signedCoefficientB, exponentB) = LibDecimalFloatImplementation.normalize(signedCoefficientB, exponentB); | |
| int256 signedCoefficient = (signedCoefficientA * 1e38) / signedCoefficientB; | |
| int256 exponent = exponentA - exponentB - 38; | |
| return (signedCoefficient, exponent); | |
| } | |
| } | |
| internal | |
| pure | |
| returns (int256, int256) | |
| { | |
| unchecked { | |
| (signedCoefficientA, exponentA) = LibDecimalFloatImplementation.normalize(signedCoefficientA, exponentA); | |
| (signedCoefficientB, exponentB) = LibDecimalFloatImplementation.normalize(signedCoefficientB, exponentB); | |
| if (signedCoefficientB == 0) revert DivisionByZero(); | |
| int256 signedCoefficient = (signedCoefficientA * 1e38) / signedCoefficientB; | |
| int256 exponent = exponentA - exponentB - 38; | |
| return (signedCoefficient, exponent); | |
| } | |
| } |
| function testGtReference(int256 signedCoefficientA, int256 exponentA, int256 signedCoefficientB, int256 exponentB) | ||
| external | ||
| pure | ||
| { | ||
| bool actual = LibDecimalFloat.gt(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | ||
| Float a = LibDecimalFloat.packLossless(signedCoefficientA, exponentA); | ||
| Float b = LibDecimalFloat.packLossless(signedCoefficientB, exponentB); | ||
| bool actual = a.gt(b); | ||
| bool expected = LibDecimalFloatSlow.gtSlow(signedCoefficientA, exponentA, signedCoefficientB, exponentB); |
There was a problem hiding this comment.
Same un‑bounded exponents issue here
testGtReference also builds packed floats straight from un‑bounded int256 exponents and will revert for out‑of‑range values. Add the same bound() guard as suggested for the lt suite.
| function add(Float a, Float b) internal pure returns (Float) { | ||
| (int256 signedCoefficientA, int256 exponentA) = a.unpack(); | ||
| (int256 signedCoefficientB, int256 exponentB) = b.unpack(); | ||
| (int256 signedCoefficient, int256 exponent) = | ||
| LibDecimalFloatImplementation.add(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | ||
| (Float c, bool lossless) = packLossy(signedCoefficient, exponent); | ||
| // Addition can be lossy. | ||
| (lossless); | ||
| return c; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unused lossless variables – replace with _ to silence the compiler
Each arithmetic wrapper stores the second return value of packLossy in lossless and then discards it with (lossless);.
Using an underscore makes the intent clearer and avoids future lint warnings:
- (Float c, bool lossless) = packLossy(signedCoefficient, exponent);
- // Addition can be lossy.
- (lossless);
+ (Float c, ) = packLossy(signedCoefficient, exponent); // addition can be lossyApply to sub, multiply, divide, minus, abs, frac, floor, power10, log10 and power.
📝 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.
| function add(Float a, Float b) internal pure returns (Float) { | |
| (int256 signedCoefficientA, int256 exponentA) = a.unpack(); | |
| (int256 signedCoefficientB, int256 exponentB) = b.unpack(); | |
| (int256 signedCoefficient, int256 exponent) = | |
| LibDecimalFloatImplementation.add(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | |
| (Float c, bool lossless) = packLossy(signedCoefficient, exponent); | |
| // Addition can be lossy. | |
| (lossless); | |
| return c; | |
| } | |
| function add(Float a, Float b) internal pure returns (Float) { | |
| (int256 signedCoefficientA, int256 exponentA) = a.unpack(); | |
| (int256 signedCoefficientB, int256 exponentB) = b.unpack(); | |
| (int256 signedCoefficient, int256 exponent) = | |
| LibDecimalFloatImplementation.add(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | |
| (Float c, ) = packLossy(signedCoefficient, exponent); // addition can be lossy | |
| return c; | |
| } |
| function packLossless(int256 signedCoefficient, int256 exponent) internal pure returns (Float) { | ||
| (Float c, bool lossless) = packLossy(signedCoefficient, exponent); | ||
| if (!lossless) { | ||
| revert CoefficientOverflow(signedCoefficient, exponent); | ||
| } | ||
| return c; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
packLossless re‑packs even when the lossy path already reverted
packLossy will already revert on an exponent overflow, so the additional check here is only guarding the coefficient width. To avoid the double‑work (and gas) you can inline the two checks:
if (int224(signedCoefficient) != signedCoefficient) {
revert CoefficientOverflow(signedCoefficient, exponent);
}
if (int32(exponent) != exponent) {
revert ExponentOverflow(signedCoefficient, exponent);
}This also documents exactly which invariant failed.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
test/src/lib/implementation/LibDecimalFloatImplementation.power10.t.sol (1)
77-81: 🛠️ Refactor suggestionFuzz test only checks “no‑revert”; validate at least basic invariants
(This is the same remark made on earlier commits.)
Relying solely ontestNoRevertleaves a vast surface for silent numeric errors. Consider assertinginv(power10(x)) ≈ 10^xfor random inputs or comparing against a high‑precision reference in Foundry’svm.remember/vm.recall.src/lib/implementation/LibDecimalFloatImplementation.sol (3)
16-17: 🧹 Nitpick (assertive)Magic number
37still undocumentedPast reviews asked for an inline explanation of why
ADD_MAX_EXPONENT_DIFFis precisely 37.
A one‑line comment (e.g. “// 37 keepssignedCoefficientA * 10^diff≤ 1e75 < 2²⁵⁵”) would prevent future maintainers from “fixing” the number and silently re‑introducing overflow.
314-323: 🧹 Nitpick (assertive)Gas: computing
10 ** alignmentExponentDiffin every call is expensive
alignmentExponentDiffis restricted to[0,37], so a small constant lookup table (or even thePOW10array suggested in earlier reviews) avoids the dynamic exponentiation and saves ~200 gas on each addition where exponents differ.uint256 constant POW10[38] = [1, 10, 100, … 1e37]; multiplier = POW10[alignmentExponentDiff];
845-860: 🧹 Nitpick (assertive)Multiplication inside
unitLinearInterpolationcan still overflow
Previous feedback highlighted thatmultiply(xDiff, yDiff)can hitint256limits before normalisation rescales. The new code callsmultiplydirectly but doesn’t bound either operand; a defensivenormalize(or the overflow‑aware multiply logic reused here) would make the path provably safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/lib/implementation/LibDecimalFloatImplementation.sol(4 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.power10.t.sol(1 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)
| function multiply(int256 signedCoefficientA, int256 exponentA, int256 signedCoefficientB, int256 exponentB) | ||
| internal | ||
| pure | ||
| returns (int256, int256) | ||
| { | ||
| unchecked { | ||
| // Unchecked mul the coefficients and add the exponents. | ||
| int256 signedCoefficient = signedCoefficientA * signedCoefficientB; | ||
|
|
||
| // Need to return early if the result is zero to avoid divide by | ||
| // zero in the overflow check. | ||
| if (signedCoefficient == 0) { | ||
| return (NORMALIZED_ZERO_SIGNED_COEFFICIENT, NORMALIZED_ZERO_EXPONENT); | ||
| } | ||
|
|
||
| int256 exponent = exponentA + exponentB; | ||
|
|
||
| // No jumps to see if we overflowed. | ||
| bool didOverflow; | ||
| assembly ("memory-safe") { | ||
| didOverflow := | ||
| or( | ||
| iszero(eq(sdiv(signedCoefficient, signedCoefficientA), signedCoefficientB)), | ||
| iszero(eq(sub(exponent, exponentA), exponentB)) | ||
| ) | ||
| } | ||
| // If we did overflow, normalize and try again. Normalized values | ||
| // cannot overflow, so this will always succeed, provided the | ||
| // exponents are not out of bounds. | ||
| if (didOverflow) { | ||
| (signedCoefficientA, exponentA) = normalize(signedCoefficientA, exponentA); | ||
| (signedCoefficientB, exponentB) = normalize(signedCoefficientB, exponentB); | ||
| return multiply(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | ||
| } | ||
| return (signedCoefficient, exponent); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
multiply can recurse unnecessarily; pre‑normalize instead
When didOverflow is true you normalise both operands and then call multiply again, causing an extra stack frame and doubling gas for the happy path.
Normalising before the first multiplication avoids the recursive call altogether:
- unchecked {
- int256 signedCoefficient = signedCoefficientA * signedCoefficientB;
+ unchecked {
+ (signedCoefficientA, exponentA) = normalize(signedCoefficientA, exponentA);
+ (signedCoefficientB, exponentB) = normalize(signedCoefficientB, exponentB);
+ int256 signedCoefficient = signedCoefficientA * signedCoefficientB;This keeps the overflow check but guarantees a single execution path.
📝 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.
| function multiply(int256 signedCoefficientA, int256 exponentA, int256 signedCoefficientB, int256 exponentB) | |
| internal | |
| pure | |
| returns (int256, int256) | |
| { | |
| unchecked { | |
| // Unchecked mul the coefficients and add the exponents. | |
| int256 signedCoefficient = signedCoefficientA * signedCoefficientB; | |
| // Need to return early if the result is zero to avoid divide by | |
| // zero in the overflow check. | |
| if (signedCoefficient == 0) { | |
| return (NORMALIZED_ZERO_SIGNED_COEFFICIENT, NORMALIZED_ZERO_EXPONENT); | |
| } | |
| int256 exponent = exponentA + exponentB; | |
| // No jumps to see if we overflowed. | |
| bool didOverflow; | |
| assembly ("memory-safe") { | |
| didOverflow := | |
| or( | |
| iszero(eq(sdiv(signedCoefficient, signedCoefficientA), signedCoefficientB)), | |
| iszero(eq(sub(exponent, exponentA), exponentB)) | |
| ) | |
| } | |
| // If we did overflow, normalize and try again. Normalized values | |
| // cannot overflow, so this will always succeed, provided the | |
| // exponents are not out of bounds. | |
| if (didOverflow) { | |
| (signedCoefficientA, exponentA) = normalize(signedCoefficientA, exponentA); | |
| (signedCoefficientB, exponentB) = normalize(signedCoefficientB, exponentB); | |
| return multiply(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | |
| } | |
| return (signedCoefficient, exponent); | |
| } | |
| } | |
| function multiply(int256 signedCoefficientA, int256 exponentA, int256 signedCoefficientB, int256 exponentB) | |
| internal | |
| pure | |
| returns (int256, int256) | |
| { | |
| unchecked { | |
| // Pre‑normalize inputs to avoid the recursive overflow‑retry path. | |
| (signedCoefficientA, exponentA) = normalize(signedCoefficientA, exponentA); | |
| (signedCoefficientB, exponentB) = normalize(signedCoefficientB, exponentB); | |
| // Unchecked mul the coefficients and add the exponents. | |
| int256 signedCoefficient = signedCoefficientA * signedCoefficientB; | |
| // Need to return early if the result is zero to avoid divide by | |
| // zero in the overflow check. | |
| if (signedCoefficient == 0) { | |
| return (NORMALIZED_ZERO_SIGNED_COEFFICIENT, NORMALIZED_ZERO_EXPONENT); | |
| } | |
| int256 exponent = exponentA + exponentB; | |
| // No jumps to see if we overflowed. | |
| bool didOverflow; | |
| assembly ("memory-safe") { | |
| didOverflow := | |
| or( | |
| iszero(eq(sdiv(signedCoefficient, signedCoefficientA), signedCoefficientB)), | |
| iszero(eq(sub(exponent, exponentA), exponentB)) | |
| ) | |
| } | |
| // If we did overflow, normalize and try again. Normalized values | |
| // cannot overflow, so this will always succeed, provided the | |
| // exponents are not out of bounds. | |
| if (didOverflow) { | |
| (signedCoefficientA, exponentA) = normalize(signedCoefficientA, exponentA); | |
| (signedCoefficientB, exponentB) = normalize(signedCoefficientB, exponentB); | |
| return multiply(signedCoefficientA, exponentA, signedCoefficientB, exponentB); | |
| } | |
| return (signedCoefficient, exponent); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (3)
src/lib/table/LibLogTable.sol (2)
10-34: 🧹 Nitpick (assertive)Prefer deriving row‑count & byte‑size from
table.lengthinstead of hard‑coding magic numbersThe assembly initialises
iwithmul(0x20, 90)and stores1820as the payload size.
Those literals tie the helper tightly to a 91‑row table; future size tweaks will silently corrupt the copy or mis‑encode the payload.Consider:
- mstore(0x40, add(encoded, add(1820, 0x20))) + let rows := mload(table) // 91 + let bytesPerRow := 0x14 // 10 * 2 + mstore(0x40, add(encoded, add(mul(rows, bytesPerRow), 0x20))) … - let i := add(table, mul(0x20, 90)) + let i := add(table, mul(0x20, sub(rows, 1)))This removes two magic numbers and guarantees correctness if the table is extended again.
66-92:⚠️ Potential issue
toBytes(uint8[10][101]): cursor decrements by 1 while writing 32 bytesThe loop:
cursor := sub(cursor, 1) mstore(cursor, mload(...)) // writes 32 bytescauses each 32‑byte chunk to overlap the previous by 31 bytes, duplicating data and overshooting
encoded.
It worked accidentally before because the consumer only read the first byte per chunk, but it is extremely brittle.Either copy one byte at a time or advance the cursor by 32:
- cursor := sub(cursor, 1) + cursor := sub(cursor, 0x20)Review the two other
uint8helpers – they exhibit the same pattern.src/lib/implementation/LibDecimalFloatImplementation.sol (1)
725-753:⚠️ Potential issue
withTargetExponentsilently returns 0 for large positiveexponentDiffFor
targetExponent > exponentthe function emits0instead of reverting whenexponentDiff > 76.
The caller cannot distinguish between a legitimate zero and a scaling failure, which can corrupt results (e.g.power10exponent calculation).Recommend consistent behaviour:
- if (exponentDiff > 76 || exponentDiff < 0) { - return (NORMALIZED_ZERO_SIGNED_COEFFICIENT); - } + if (exponentDiff > 76 || exponentDiff < 0) { + revert WithTargetExponentOverflow(signedCoefficient, exponent, targetExponent); + }
♻️ Duplicate comments (10)
test/src/lib/implementation/LibDecimalFloatImplementation.power10.t.sol (3)
54-60: 🧹 Nitpick (assertive)Improve readability of large numeric literals.
Using scientific notation literals like
5.012531328320802005012531328320802005e37makes the code hard to read and maintain. Scientific notation can also introduce subtle rounding issues when the compiler interprets these as rational numbers.Consider using underscore-separated integer constants:
- checkPower10(-0.3e37, -37, 5.012531328320802005012531328320802005e37, -38); + int256 constant COEFF = 5_012_531_328_320_802_005_012_531_328_320_802_005e19; + checkPower10(-0.3e37, -37, COEFF, -38);This approach makes the code more readable and guarantees the value is treated as an exact
int256.
62-67: 🧹 Nitpick (assertive)Improve readability of large numeric literals.
Same issue as above with scientific notation literals in interpolated lookups tests.
Consider using underscore-separated integer constants for large numbers to improve readability and prevent compiler rounding:
- checkPower10(1.55555e37, -37, 35935e37, -40); + int256 constant LOOKUP_COEFF = 35_935e37; + checkPower10(1.55555e37, -37, LOOKUP_COEFF, -40);
71-77:⚠️ Potential issueFix potential reversion in boundFloat before vm.assume filters run.
The
packLosslessfunction can revert ifxis out of range, butxisn't constrained before calling the function. This could cause the fuzzer to abort test runs rather than discard invalid cases.Apply this fix to bound
xbefore attempting to pack:function boundFloat(int224 x, int32 exponent) internal pure returns (int224, int32) { exponent = int32(bound(exponent, -76, 76)); + x = int224(bound(x, -1e38 + 1, 1e38 - 1)); // prevent coefficient overflow Float a = LibDecimalFloat.packLossless(x, exponent); vm.assume(a.gt(LibDecimalFloat.packLossless(-1e38, 0))); vm.assume(a.lt(LibDecimalFloat.packLossless(type(int224).max, 0))); return (x, exponent); }test/src/lib/LibDecimalFloat.power10.t.sol (2)
11-13: 🧹 Nitpick (assertive)Add view modifier to the power10External function.
This function only reads from state (via
logTables()) and performs computations without modifying state. Adding theviewmodifier would communicate this intent, provide minor gas savings, and prevent accidental state modifications.-function power10External(int256 signedCoefficient, int256 exponent) external returns (int256, int256) { +function power10External(int256 signedCoefficient, int256 exponent) external view returns (int256, int256) { return LibDecimalFloatImplementation.power10(logTables(), signedCoefficient, exponent); }
15-17: 🧹 Nitpick (assertive)Add view modifier to the Float-based power10External function.
For consistency with the other
power10Externalfunction and to accurately represent the function's behavior, add theviewmodifier here as well.-function power10External(Float float) external returns (Float) { +function power10External(Float float) external view returns (Float) { return LibDecimalFloat.power10(logTables(), float); }src/lib/implementation/LibDecimalFloatImplementation.sol (2)
109-145: Recursive overflow‑retry inmultiplypersists – replace with single‑pass normalisation
This was flagged previously and is still present.Each overflow triggers an extra stack frame and ~2× gas. Pre‑normalising operands before multiplication is simpler and cheaper.
319-324: 🧹 Nitpick (assertive)
10 ** alignmentExponentDiffis gas‑heavy; use a tiny lookup table
alignmentExponentDiff ≤ 37, so auint256[38] POW10constant saves ~200 gas peradd.- multiplier = 10 ** alignmentExponentDiff; + multiplier = POW10[alignmentExponentDiff];(Initialisation cost is paid once at deployment.)
src/lib/LibDecimalFloat.sol (3)
88-89:usingdirective is still redundant inside the defining libraryThis directive has no effect inside the same library that declares the functions (library methods are always in scope here) and slightly increases byte‑code size.
Removing it keeps the byte‑code lean and avoids confusing future readers.
343-345: A short comment would save future head‑scratching
signextend(27, …)is mathematically correct because byte 27 holds bit 223 (the sign bit of the 224‑bit coefficient). Documenting this “magic number” makes the intention crystal‑clear for auditors and contributors.
355-363: Unusedlosslessvariable – use_to signal its intentional discardStoring then discarding the flag is fine, but replacing it with the unnamed tuple slot keeps the byte‑code a little smaller and silences some linters:
- (Float c, bool lossless) = packLossy(signedCoefficient, exponent); - // Addition can be lossy. - (lossless); + (Float c, ) = packLossy(signedCoefficient, exponent); // addition can be lossyApply the same pattern to
sub,minus,abs,multiply,divide,inv,frac,floor,power10,log10, andpower.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
src/generated/LogTables.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (8)
src/lib/LibDecimalFloat.sol(14 hunks)src/lib/implementation/LibDecimalFloatImplementation.sol(5 hunks)src/lib/table/LibLogTable.sol(11 hunks)test/src/lib/LibDecimalFloat.power.t.sol(2 hunks)test/src/lib/LibDecimalFloat.power10.t.sol(1 hunks)test/src/lib/LibDecimalFloat.sub.t.sol(1 hunks)test/src/lib/format/LibFormatDecimalFloat.t.sol(2 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.power10.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: git-clean
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
🔇 Additional comments (16)
test/src/lib/format/LibFormatDecimalFloat.t.sol (4)
39-40: Address the CoefficientOverflow by using appropriate bounds.The current code now bounds the value to
uint256(int256(type(int224).max)), which is more appropriate than the previous unbounded approach, as it prevents theCoefficientOverflowerror mentioned in past reviews.
37-44: LGTM: Renamed function and updated implementation for packed Float representation.The function has been appropriately renamed from
testRoundTriptotestFormatDecimalRoundTripand updated to use the new packedFloatrepresentation. The implementation correctly usesfromFixedDecimalLosslessPackedand handles the parsed result as a tuple.
25-33: LGTM: Updated test to handle packed Float representation.The test has been properly updated to unpack the
Floatvalue before calling the external formatting function, reflecting the shift to packed representation while maintaining the same test logic.
20-22: LGTM: Updated function signature for packed Float type.Function signature correctly changed to accept
Floatas a value parameter instead of a memory reference, consistent with the codebase-wide shift to packed representation.test/src/lib/implementation/LibDecimalFloatImplementation.power10.t.sol (4)
12-23: LGTM: Well-structured helper function for power10 tests.The
checkPower10function is well-designed to simplify test assertions with clean parameter passing and explicit error messages for each assertion.
79-83: LGTM: Fuzz test ensures no reversion within valid ranges.The fuzz test appropriately uses the
boundFloathelper to constrain inputs, ensuring the function doesn't revert for valid inputs.
25-33: LGTM: Comprehensive test coverage for exact powers.The test cases cover important power of 10 scenarios with appropriate normalization checks.
35-60: LGTM: Good coverage of exact lookup values.The test cases thoroughly verify both integer and fractional powers across the expected range of the lookup table.
test/src/lib/LibDecimalFloat.sub.t.sol (3)
17-18: LGTM: Updated to use the implementation library.Correctly updated to call
LibDecimalFloatImplementation.subinstead ofLibDecimalFloat.sub, aligning with the refactoring approach.
20-22: LGTM: Updated function signature for packed Float type.Function signature correctly changed to accept and return
Floatas value parameters instead of memory references, consistent with the codebase-wide shift to packed representation.
24-38: LGTM: Renamed and restructured test for packed representation.The test function has been appropriately renamed from
testSubMemtotestSubPackedand restructured to handle the packed representation, with proper unpacking and comparison logic.test/src/lib/LibDecimalFloat.power10.t.sol (2)
19-37: LGTM: Improved test with explicit exponent overflow handling.The test function has been renamed to
testPower10Packedand now properly handles the exponent overflow case with explicit assertions. The unpacking and comparison logic correctly validates the results.
12-12: LGTM: Updated to use the implementation library.Correctly updated to call
LibDecimalFloatImplementation.power10instead ofLibDecimalFloat.power10, aligning with the refactoring approach.test/src/lib/LibDecimalFloat.power.t.sol (1)
37-38: Extremely large expected exponent looks suspicious
expectedExponent = 60907is five orders of magnitude larger than any exponent produced elsewhere in the suite.
Was this intended, or is the coefficient/exponent pair swapped? A typo here will make the test vacuous because theassertEqwill always fail and fall into thetry/catchpath later.src/lib/LibDecimalFloat.sol (2)
150-152: 🧹 Nitpick (assertive)Same shadowing issue as above (
floatvs.Float)Renaming here keeps the codebase consistent and readable.
- (int256 signedCoefficient, int256 exponent) = fromFixedDecimalLossless(value, decimals); - return packLossless(signedCoefficient, exponent); + (int256 signedCoefficient, int256 exponent) = fromFixedDecimalLossless(value, decimals); + return packLossless(signedCoefficient, exponent);(The return value is already anonymous; the local variable can be removed entirely.)
Likely an incorrect or invalid review comment.
291-320:Details
❌ Incorrect review comment
Mask the exponent before shifting to guarantee a clean 32‑bit payload
shl(0xe0, exponent)relies on the fact thatexponenthas been range‑checked to fit anint32, but its high bits are still set for negative numbers.
Although the resulting top‑32‑bits are correct two’s‑complement, explicitly masking communicates intent and removes any possibility of stray bits leaking in if the range check ever changes.- assembly ("memory-safe") { - float := or(and(signedCoefficient, mask), shl(0xe0, exponent)) - } + assembly ("memory-safe") { + // Cast + mask keeps only the low 32‑bit two’s‑complement representation. + let packedExponent := and(exponent, 0xffffffff) + float := or(and(signedCoefficient, mask), shl(0xe0, packedExponent)) + }This is a micro‑optimisation (1 extra
AND) but improves auditability and future‑proofs the packing logic.
Unnecessary explicit masking of the exponent
The Yul
shloperator shifts left modulo 2²⁵⁶, discarding any high bits beyond the 256‑bit word. Combined with your priorint32range‐check, the shifted result already contains exactly the 32‑bit two’s‑complement exponent in bits 224–255. Adding an extraANDtherefore does not change the outcome—it’s purely a stylistic clarification, not a functional necessity. If you want to document intent, a comment on the modulo semantics is sufficient without inserting the mask.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
src/lib/table/LibLogTable.sol (1)
44-52: 💡 Verification agent🧩 Analysis chain
mstoreis writing 32 bytes but the cursor only advances 1 byte – high risk of data corruptionIn the
toBytes(uint8[10][91])implementation the loop decrements the cursor by 1 (cursor := sub(cursor, 1)) yet each iteration writes 32 bytes withmstore.
This means every write overlaps (and therefore overwrites) the previous 31 bytes, so only the last byte copied per 32‑byte chunk ends up inencoded.
Unless this overlapping trick is deliberate and proven correct, it is very easy to introduce silent corruption the moment table sizes change or another engineer refactors the code.
- Use
mstore8(and decrement the cursor by 1) when copying a singleuint8.- Or, keep
mstorebut decrement the cursor by 32 to avoid overlap.- Add a unit test that round‑trips
toBytes→ decode → compare original table to catch regressions.- cursor := sub(cursor, 1) - … - mstore(cursor, mload(add(mload(i), j))) + cursor := sub(cursor, 1) // keep for mstore8 + let value := byte(0, mload(add(mload(i), j))) + mstore8(cursor, value)
🏁 Script executed:
#!/bin/bash # Locate and inspect the toBytes implementation in LibLogTable.sol # Show the function signature and a few lines around it rg -n -C3 "function toBytes" src/lib/table/LibLogTable.sol # Show the loop with mstore and cursor decrement to confirm the current behavior rg -n -C3 "mstore(cursor" src/lib/table/LibLogTable.solLength of output: 1474
🏁 Script executed:
#!/bin/bash # Display the uint8[10][91] toBytes implementation around lines 35–65 sed -n '35,65p' src/lib/table/LibLogTable.solLength of output: 964
Fix overlapping 32‑byte writes in
toBytes(uint8[10][91])In the Yul assembly loop you decrement the cursor by 1 byte but call
mstore(which writes 32 bytes), causing each write to overlap the previous 31 bytes. This will silently corruptencodedif the table size or indexing ever changes.Please update the loop in
src/lib/table/LibLogTable.sol(lines ~44–52) as follows:
- If you want to copy one
uint8at a time, extract the byte and usemstore8, keepingcursor := sub(cursor, 1).- If you intend to write full 32‑byte words, step the cursor by 32:
cursor := sub(cursor, 0x20).Also, add a unit test that round‑trips a full 91×10 table through
toBytes→ decode → compare original to catch regressions.Suggested diff:
-for { - let i := add(table, mul(0x20, 90)) - let j := mul(0x20, 9) -} gt(cursor, encoded) { - cursor := sub(cursor, 1) - j := sub(j, 0x20) -} { - mstore(cursor, mload(add(mload(i), j))) +for { + let i := add(table, mul(0x20, 90)) + let j := mul(0x20, 9) +} gt(cursor, encoded) { + cursor := sub(cursor, 1) // step by 1 byte + j := sub(j, 0x20) +} { + // extract the single uint8 and write one byte + let byteVal := byte(0, mload(add(mload(i), j))) + mstore8(cursor, byteVal) if iszero(j) { i := sub(i, 0x20) j := mul(0x20, 10) } }src/lib/implementation/LibDecimalFloatImplementation.sol (1)
38-53:⚠️ Potential issueBroken constant‑ordering – compilation fails before tests even run
NORMALIZED_JUMP_DOWN_THRESHOLDandNORMALIZED_JUMP_UP_THRESHOLDreference
SIGNED_NORMALIZED_MAX/SIGNED_NORMALIZED_MINbefore those constants are
declared.
Solidity requires a constant to be declared above every place it is used,
otherwise the compiler raises “Identifier not found or not unique”.-/// @dev Every value above or equal to this can jump down while normalizing -/// without overshooting and causing unnecessary precision loss. -int256 constant NORMALIZED_JUMP_DOWN_THRESHOLD = SIGNED_NORMALIZED_MAX * PRECISION_JUMP_MULTIPLIER; -/// @dev Every value below this can jump up while normalizing without -/// overshooting the normalized range. -int256 constant NORMALIZED_JUMP_UP_THRESHOLD = SIGNED_NORMALIZED_MIN / PRECISION_JUMP_MULTIPLIER; - /// @dev The minimum absolute value of a normalized signed coefficient. uint256 constant NORMALIZED_MIN = 1e37; int256 constant SIGNED_NORMALIZED_MIN = 1e37; /// @dev The maximum absolute value of a normalized signed coefficient. uint256 constant NORMALIZED_MAX = 1e38 - 1; int256 constant SIGNED_NORMALIZED_MAX = 1e38 - 1; + +/// @dev Every value above or equal to this can jump down while normalizing +/// without overshooting and causing unnecessary precision loss. +int256 constant NORMALIZED_JUMP_DOWN_THRESHOLD = SIGNED_NORMALIZED_MAX * PRECISION_JUMP_MULTIPLIER; +/// @dev Every value below this can jump up while normalizing without +/// overshooting the normalized range. +int256 constant NORMALIZED_JUMP_UP_THRESHOLD = SIGNED_NORMALIZED_MIN / PRECISION_JUMP_MULTIPLIER;Moving the MIN/MAX definitions above the derived thresholds (or vice‑versa)
unblocks compilation.
♻️ Duplicate comments (9)
test/src/lib/LibDecimalFloat.decimal.t.sol (1)
31-41: Test updated with proper bounds to prevent overflowThe test function (renamed from
testFromFixedDecimalLossyMemtotestFromFixedDecimalLossyPacked) now properly:
- Bounds the input value to prevent coefficient overflow errors
- Uses the packed API
fromFixedDecimalLossyPacked- Unpacks the result to verify coefficient and exponent values
- Checks losslessness consistently
This addresses the previous overflow issue mentioned in past reviews and correctly tests the packed representation.
test/src/lib/LibDecimalFloat.inv.t.sol (1)
22-22: 🧹 Nitpick (assertive)Update function name to reflect packed implementation
While the test logic has been correctly updated to work with packed Float values, the function name "testInvMem" suggests it's testing memory-based implementation.
Rename the function to better reflect its purpose:
-function testInvMem(Float float) external { +function testInvPacked(Float float) external {src/lib/table/LibLogTable.sol (2)
70-80: Same single‑byte overlap issue repeated intoBytes(uint8[10][101])The logic mirrors the previous function and ships with the same 1‑byte / 32‑byte overlap hazard.
Please fix both implementations together (see previous comment for details) or extract a common, well‑tested helper.
350-352: Placeholder comment still contradicts the row contentPrevious reviews already pointed this out: the comment claims “The last entry is 10000, so we just use that” but the placeholder row in
logTableDecSmallis all zeros.
Either change the numbers to10000or update the comment so future readers are not mis‑led.test/src/lib/LibDecimalFloat.power.t.sol (2)
32-33: Negative coefficients logged as huge unsigned numbersCasting
signedCoefficientAtouint256underflows when the value is negative, producing unreadable log output.-console2.log("%d %d Gas used: %d", uint256(signedCoefficientA), uint256(exponentA), …); +console2.log("%d %d Gas used: %d", signedCoefficientA, exponentA, …);
79-94:catch {}blocks silently swallow all reverts – hides genuine failuresThe fuzz test ignores every error, including assertion failures, overflow checks and unexpected revert reasons.
At minimum assert that only the known, acceptable selectors are swallowed (e.g.CoefficientOverflow,ExponentOverflow, division‑by‑zero).- } catch (bytes memory err) {} + } catch (bytes memory err) { + // Only overflow‑related errors are acceptable; anything else is a failure. + bytes4 sel; + assembly { sel := mload(add(err, 0x20)) } + if (sel != CoefficientOverflow.selector && sel != ExponentOverflow.selector) { + fail("unexpected revert"); + } + }This keeps the fuzz test meaningful and prevents silent false‑negatives.
src/lib/implementation/LibDecimalFloatImplementation.sol (1)
535-538:⚠️ Potential issueUnchecked exponent addition can still overflow (
power10)This is the exact issue flagged in the previous review and it remains
unaddressed – adding the characteristic shift toexponentis done in an
uncheckedcontext, sotype(int256).maxcan silently wrap.return ( signedCoefficient, - 1 + exponent + withTargetExponent(characteristicCoefficient, characteristicExponent, 0) + normalize( // <- cheap guard, no precision loss + 1, + exponent + withTargetExponent(characteristicCoefficient, characteristicExponent, 0) + ).exponent );Either normalise the result before returning or insert an explicit range
check.test/src/lib/LibDecimalFloat.lt.t.sol (1)
99-105: 🛠️ Refactor suggestionDuplicate source of undefined
Floats (see above)
aandbare unconstrained random bytes. Convert them from bounded ints to
keep the property‑based test meaningful and revert‑free.test/src/lib/LibDecimalFloat.gt.t.sol (1)
92-98: 🛠️ Refactor suggestionRandom
Floatinputs again – may bypass test intentThe three‑way compare relies on numeric properties. If either packed value
is invalid the test reverts before the assertions run, giving a false sense of
security.Adopt the same bounded‑int strategy as suggested for the
lttests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
.gas-snapshot(1 hunks)src/lib/implementation/LibDecimalFloatImplementation.sol(7 hunks)src/lib/table/LibLogTable.sol(11 hunks)test/src/lib/LibDecimalFloat.add.t.sol(2 hunks)test/src/lib/LibDecimalFloat.decimal.t.sol(1 hunks)test/src/lib/LibDecimalFloat.decimalLossless.t.sol(2 hunks)test/src/lib/LibDecimalFloat.divide.t.sol(2 hunks)test/src/lib/LibDecimalFloat.floor.t.sol(2 hunks)test/src/lib/LibDecimalFloat.frac.t.sol(2 hunks)test/src/lib/LibDecimalFloat.gt.t.sol(1 hunks)test/src/lib/LibDecimalFloat.inv.t.sol(1 hunks)test/src/lib/LibDecimalFloat.log10.t.sol(1 hunks)test/src/lib/LibDecimalFloat.lt.t.sol(1 hunks)test/src/lib/LibDecimalFloat.multiply.t.sol(2 hunks)test/src/lib/LibDecimalFloat.pack.t.sol(1 hunks)test/src/lib/LibDecimalFloat.power.t.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: git-clean
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
🔇 Additional comments (32)
test/src/lib/LibDecimalFloat.pack.t.sol (4)
4-4: Import statement updated to reflect new Float type definitionThe import statement has been updated to include
ExponentOverflow,Float, andEXPONENT_MAXfrom the LibDecimalFloat.sol file. This supports the codebase's consolidation around a single packedFloattype alias.
9-11: Function updated for packed Float typeThe function has been updated to return a
Floattype along with a boolean indicating losslessness. This correctly implements the updated API for packing operations using the consolidated Float type.
13-15: Unpacking function updated for consistencyThe unpacking function now accepts a
Floatparameter directly instead of a separate PackedFloat type, maintaining consistency with the new consolidated type system.
19-24: Test refactored to use the new packed Float APIThe test now correctly:
- Calls
packLossyto create a packed Float- Verifies losslessness before proceeding
- Unpacks the Float and verifies coefficient and exponent values
The test properly validates the round-trip behavior with the new consolidated type.
test/src/lib/LibDecimalFloat.divide.t.sol (3)
4-5: Imports simplified to match consolidated type systemThe imports have been updated to focus on the core
Floattype and the implementation library, removing unnecessary imports and reflecting the codebase's consolidation around a single packed Float type.
24-26: Function signature updated for value-based parameter passingThe function now takes
Floatparameters by value instead of by memory reference, which is more efficient for the packed Float type (likely a bytes32 alias).
28-43: Test function refactored for packed Float testingThe test has been renamed from
testDivideMemtotestDividePackedand updated to:
- Unpack Float inputs to get raw values
- Test division using the implementation directly
- Compare against the packed Float implementation
- Properly handle error cases
This ensures the packed and unpacked implementations produce identical results, validating the refactoring approach.
test/src/lib/LibDecimalFloat.decimal.t.sol (2)
27-29: Function updated to use packed Float parameterThe function now takes a
Floatparameter by value instead of by memory reference, aligning with the consolidated type system and improving efficiency for this small value type.
45-58: Test refactored for packed Float consistency verificationThe test function (renamed from
testToFixedDecimalLossyMemtotestToFixedDecimalLossyPacked) properly:
- Unpacks the Float parameter to call the implementation with raw values
- Compares results between packed and unpacked implementations
- Handles error propagation consistently
This ensures both implementations behave identically across all inputs.
test/src/lib/LibDecimalFloat.multiply.t.sol (3)
4-5: Imports updated for consolidated Float typeThe imports have been simplified to focus on the core Float type and constants, aligning with the codebase's consolidated approach to floating-point representation.
24-26: Function signature updated for value-based parameter passingThe function now takes
Floatparameters by value instead of by memory reference, which is more efficient for this packed type and aligns with the consolidated approach.
28-43: Test refactored for packed Float verificationThe test function (renamed from
testMultiplyMemtotestMultiplyPacked) properly:
- Unpacks Float inputs to get raw component values
- Tests multiplication using direct component values
- Compares against multiplication using packed Floats
- Handles error cases consistently
This ensures both implementation approaches produce identical results, validating the refactoring strategy.
test/src/lib/LibDecimalFloat.inv.t.sol (4)
5-6: LGTM! Import changes aligned with the consolidated packed Float approachThe imports have been correctly updated to reflect the new architecture with packed
Floattype and delegation to implementation library.
11-16: LGTM! Function correctly adapts to the packed Float architectureThe function now properly:
- Delegates core logic to
LibDecimalFloatImplementation.inv- Uses
packLossyto convert the returned values to a packedFloat- Returns the packed
Floattype directlyThis aligns well with the consolidated packed approach.
18-20: LGTM! Updated function signature to use packed FloatCorrectly updated to use the packed
Floattype by value rather than by memory reference.
23-29: LGTM! Test logic properly updated for packed FloatThe test logic has been correctly updated to:
- Unpack the input
Floatto get components- Compare unpacked components instead of struct fields
- Handle both the component-based and packed Float paths consistently
This ensures equivalence between the two paths is properly tested.
test/src/lib/LibDecimalFloat.log10.t.sol (4)
5-5: LGTM! Import updated to include implementation libraryCorrect addition of the implementation library import that contains the core logarithm logic.
11-17: LGTM! Function correctly updated to use packed FloatThe function has been properly updated to:
- Call the implementation library's log10 function
- Pack the result using packLossy
- Return a packed Float value
This aligns with the library's shift to packed representation.
19-21: LGTM! Updated function signature to use packed Float directlyThe function now correctly accepts a packed
Floatby value rather than by memory reference and returns a packedFloat.
23-30: LGTM! Test renamed and updated for packed FloatThe test function has been correctly:
- Renamed from
testLog10MemtotestLog10Packedto reflect the type change- Updated to work with packed
Floatvalues- Modified to unpack and compare components instead of comparing struct fields
This ensures consistency with the new packed Float approach.
test/src/lib/LibDecimalFloat.decimalLossless.t.sol (6)
21-23: LGTM! Function renamed and updated to use packed FloatThe function has been correctly renamed from
fromFixedDecimalLosslessMemExternaltofromFixedDecimalLosslessPackedExternaland updated to return a packedFloat.
33-35: LGTM! Updated function signature to use packed FloatThe function now correctly accepts a packed
Floatby value rather than by memory reference.
37-38: LGTM! Value bounded to prevent coefficient overflowThe test now properly bounds the input value to stay within
int224range, preventing coefficient overflow errors.
45-49: LGTM! Test updated to use packed Float representationThe test has been properly updated to:
- Use the packed Float implementation
fromFixedDecimalLosslessPacked- Unpack the Float to get components
- Compare unpacked components instead of struct fields
This ensures consistency with the new packed Float approach.
52-54: LGTM! Test renamed and updated for packed FloatThe test function has been correctly renamed from
testToFixedDecimalLosslessMemtotestToFixedDecimalLosslessPackedand updated to work with packedFloatvalues.
59-59: LGTM! Updated external call to use packed FloatCorrectly updated to use the external function with packed
Floatparameter.test/src/lib/LibDecimalFloat.add.t.sol (4)
4-8: LGTM! Imports updated for packed Float and implementationThe imports have been correctly updated to:
- Import the packed
Floattype- Import the implementation library and relevant constants
- Remove unused constants
This properly reflects the architectural shift to packed Float representation.
15-25: LGTM! Function updated to return packed FloatThe function has been properly updated to:
- Return a packed
Floatinstead of component tuple- Call the implementation library's add function
- Pack the result using packLossy
This aligns with the library's packed Float approach.
27-29: LGTM! Updated function signature to use packed FloatThe function now correctly accepts packed
Floatvalues by value rather than by memory reference and returns a packedFloat.
31-43:Details
❌ Incorrect review comment
Consider implementing coefficient overflow handling in testAddPacked
While the test logic has been updated to use packed
Floatvalues, it doesn't explicitly handle cases where the result of addition causes coefficient overflow.From past review comments, it appears there might be issues with coefficient overflow errors. Consider adding bounds checking or pre-filtering input values to avoid
CoefficientOverflowerrors.function testAddPacked(Float a, Float b) external { + // Filter out Float values that could cause overflow + vm.assume(Float.unwrap(a) != 0 && Float.unwrap(b) != 0); + { + try LibDecimalFloat.unpack(a) returns (int256 sc, int256 e) { + vm.assume(sc >= type(int224).min && sc <= type(int224).max); + } catch { + return; + } + } + { + try LibDecimalFloat.unpack(b) returns (int256 sc, int256 e) { + vm.assume(sc >= type(int224).min && sc <= type(int224).max); + } catch { + return; + } + } (int256 signedCoefficientA, int256 exponentA) = LibDecimalFloat.unpack(a); (int256 signedCoefficientB, int256 exponentB) = LibDecimalFloat.unpack(b);
🏁 Script executed:
#!/bin/bash # Check for coefficient overflow tests or handling in the tests or implementation echo "Searching for coefficient overflow handling in implementation..." rg -A 5 "CoefficientOverflow" src/ echo "Searching for coefficient bounds in tests..." rg -A 5 "bound.*coefficient" test/Length of output: 1446
Remove Unnecessary Overflow Filters in testAddPacked
The existing
testAddPackedalready covers both successful additions and overflow reverts by using thetry/catchblock and asserting that reverts match the original error. Introducing pre‑filters to exclude overflow‑causing inputs would hide those error cases and reduce test coverage. No additional bounds checks orvm.assumecalls for coefficient ranges are needed here.Likely an incorrect or invalid review comment.
test/src/lib/LibDecimalFloat.floor.t.sol (1)
11-13:Details
❌ Incorrect review comment
Same fuzz issue as in
fractest
testFloorNotRevertsassumes every randomFloatreturned by the fuzzer is well‑formed. Please guard against invalid encodings or switch the property to “does not revert iff valid”.
🏁 Script executed:
#!/bin/bash # Show the beginning of the floor test to confirm if any guards are applied sed -n '1,50p' test/src/lib/LibDecimalFloat.floor.t.sol # Locate and inspect the frac test for comparison rg -n 'testFracNotReverts' -n test/src/lib sed -n '1,50p' test/src/lib/LibDecimalFloat.frac.t.solLength of output: 4088
🏁 Script executed:
#!/bin/bash # Locate the definition of Float and related constants in the library rg -n "type Float" -n src/lib/LibDecimalFloat.sol || rg -n "struct Float" -n src/lib/LibDecimalFloat.sol # Show surrounding lines to understand validity constraints rg -C3 "packLossless" -n src/lib/LibDecimalFloat.solLength of output: 845
🏁 Script executed:
#!/bin/bash # Search and display the implementation of `floor` in the library rg -n "function floor" -n src/lib/LibDecimalFloat.sol # If found, print its body for inspection rg -C5 "function floor" -n src/lib/LibDecimalFloat.solLength of output: 734
🏁 Script executed:
#!/bin/bash # Find the unpack function implementation for Float rg -n "function unpack" -n src/lib/LibDecimalFloat.sol # Display its body for analysis rg -C5 "function unpack" -n src/lib/LibDecimalFloat.solLength of output: 832
🏁 Script executed:
#!/bin/bash # Search for the packLossy function definition rg -n "function packLossy" -n src/lib/LibDecimalFloat.sol # Display its implementation for analysis rg -C5 "function packLossy" -n src/lib/LibDecimalFloat.solLength of output: 932
🏁 Script executed:
#!/bin/bash # Locate the file(s) defining `characteristicMantissa` rg -l "characteristicMantissa" -n src # Display the implementation for inspection for file in $(rg -l "characteristicMantissa" -n src); do echo "=== $file ===" rg -C8 "function characteristicMantissa" -n "$file" doneLength of output: 1325
Incorrect suggestion:
floornever reverts on anyFloatencodingThe
floorimplementation unpacks an arbitrary 32‑byte value, computes the characteristic/mantissa in an unchecked block, and repacks viapackLossy(which does not revert). There is no code path infloorthat can throw, even for “invalid” bit patterns. As such,testFloorNotRevertscorrectly asserts that callingx.floor()never reverts, and no additional guards or “iff valid” checks are required.Likely an incorrect or invalid review comment.
.gas-snapshot (1)
121-128:Details
❓ Verification inconclusive
power10fuzz tests now > 1.23 M gas – confirm no accidental regression
LibDecimalFloatPower10Test:testPower10Packed,testExactLookups, andtestInterpolatedLookupsPowerhave all crossed the 1.6 M / 1.23 M gas mark.
Because these values changed together with the packed‑float refactor, please sanity‑check:
- The packing/unpacking logic is
uncheckedwhere safe;- No redundant
abi.decode/abi.encodePackedcalls were introduced;- Re‑normalisation is skipped when the exponent is already in range.
If the spike is unavoidable, consider adding a “budget” comment in the test explaining the rationale so reviewers do not flag it again.
🏁 Script executed:
#!/bin/bash # Locate Power10Test entries in the gas snapshot grep -n 'LibDecimalFloat.*Power10Test' .gas-snapshot grep -n 'testPower10Packed' .gas-snapshot grep -n 'testExactLookups' .gas-snapshot grep -n 'testInterpolatedLookupsPower' .gas-snapshotLength of output: 984
🏁 Script executed:
#!/bin/bash # Search for abi.encodePacked and abi.decode occurrences rg -n "abi\.encodePacked" . rg -n "abi\.decode" . # Search for unchecked usage in Solidity files rg -n "unchecked" --glob "*.sol" # Search for normalization-related functions or calls rg -n "normalize" --glob "*.sol" rg -n "renormalize" --glob "*.sol"Length of output: 10716
Please verify packing logic and re‑normalization in Power10 implementation
Quick findings from the codebase search:
- No
abi.decodecalls and only twoabi.encodePackedusages (in parsing tests and deployment), so it’s unlikely redundant encode/decode was introduced.
- Numerousunchecked { … }blocks exist inLibDecimalFloatImplementation.sol, but it’s not obvious which cover the new pack/unpack paths in your Power10 refactor.
- The early‐exit “exponent already in range” check around normalization in the Power10 path needs to be confirmed present.Next steps:
• Open the new Power10 routines in
src/lib/implementation/LibDecimalFloatImplementation.soland ensure:
– Packing/unpacking calls are wrapped inuncheckedonly where safe.
– No extraabi.encodePacked/abi.decodeare invoked in that code path.
– A guard skips any re‑normalization when the exponent is within the acceptable range.If the gas increase is unavoidable, please add a “budget” comment in the Power10 test explaining the spike.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/src/lib/LibDecimalFloat.power.t.sol (1)
81-96: Emptycatchblocks still swallow unexpected revertsPrevious reviews highlighted that silent catches hide assertion failures.
Lines 82-96 still ignore all errors other than an implicitDivisionByZero, so a genuine bug insidepowercould pass unnoticed.Repurpose the handler to allow only the specific, documented revert selectors (e.g.
ExponentOverflow,DivisionByZero) and fail otherwise.This repeats earlier feedback verbatim.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/src/lib/LibDecimalFloat.power.t.sol(2 hunks)
🔇 Additional comments (1)
test/src/lib/LibDecimalFloat.power.t.sol (1)
48-61: Potential division-by-zero incheckRoundTrip
roundTripmay legitimately evaluate to0(e.g. whena.power(b)underflows).
Unlike the fuzz helper, this path isn’t guarded here, soa.divide(roundTrip)will revert withDivisionByZero.Add the same zero-check you already use in
testRoundTripFuzz:- Float c = a.power(b, tables); - - Float roundTrip = c.power(b.inv(), tables); + Float c = a.power(b, tables); + + Float roundTrip = c.power(b.inv(), tables); + if (roundTrip.isZero()) return; // avoid divide-by-zero
1 participant
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests