Skip to content

2025 08 13 maximize moar #99

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
thedavidmeister merged 11 commits into from
Aug 14, 2025
Merged

2025 08 13 maximize moar #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3453a4b
wip on maximize moar
thedavidmeister Aug 13, 2025
c8e9540
wip on maximize
thedavidmeister Aug 13, 2025
a006a0e
fix tests
thedavidmeister Aug 14, 2025
75587bb
fix tets
thedavidmeister Aug 14, 2025
344960d
Update .gas-snapshot
thedavidmeister Aug 14, 2025
2e5b32c
negative characteristic mantissa check
thedavidmeister Aug 14, 2025
f58d8a7
Merge branch '2025-08-13-maximize-moar' of github.com:rainlanguage/ra…
thedavidmeister Aug 14, 2025
06eadab
characteristic and mantissa
thedavidmeister Aug 14, 2025
f6991f4
Update test/src/lib/implementation/LibDecimalFloatImplementation.add.…
thedavidmeister Aug 14, 2025
3ed206d
more tests
thedavidmeister Aug 14, 2025
bc8851a
Merge branch '2025-08-13-maximize-moar' of github.com:rainlanguage/ra…
thedavidmeister Aug 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
403 changes: 207 additions & 196 deletions .gas-snapshot
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/lib/LibDecimalFloat.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,7 @@ library LibDecimalFloat {
// mantissa == 0 → value is already an integer.
// mantissa > 0 (input > 0) → truncation decreases the value, so add 1 to round up.
else if (mantissa > 0) {
(characteristic, exponent) = LibDecimalFloatImplementation.add(characteristic, exponent, 1e75, -75);
(characteristic, exponent) = LibDecimalFloatImplementation.add(characteristic, exponent, 1e76, -76);
}
Comment on lines 646 to 648
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 14, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Avoid magic numbers: centralize the unpacked “one” as a named constant/helper

The pair (1e76, -76) is a magic number here and appears in tests as well. Consider introducing a single source of truth for the unpacked “one” to improve readability and reduce future churn if the normalization boundary shifts again.

Example approach (outside this file, in LibDecimalFloatImplementation.sol or a shared constants file):

// In LibDecimalFloatImplementation.sol
int256 constant ONE_COEFFICIENT = 1e76;
int256 constant ONE_EXPONENT = -76;

Then here:

- (characteristic, exponent) = LibDecimalFloatImplementation.add(characteristic, exponent, 1e76, -76);
+ (characteristic, exponent) = LibDecimalFloatImplementation.add(
+     characteristic,
+     exponent,
+     LibDecimalFloatImplementation.ONE_COEFFICIENT,
+     LibDecimalFloatImplementation.ONE_EXPONENT
+ );
🤖 Prompt for AI Agents 
In src/lib/LibDecimalFloat.sol around lines 646 to 648, the unpacked “one” is
used as a magic pair (1e76, -76); introduce a named constant (or two constants)
in LibDecimalFloatImplementation.sol or a shared constants file (e.g.,
ONE_COEFFICIENT = 1e76 and ONE_EXPONENT = -76 or a single struct/tuple helper)
and replace the literal pair here with those constants; update any tests that
use the same literals to reference the centralized constants so the
normalization boundary has a single source of truth.


(Float result, bool lossless) = packLossy(characteristic, exponent);
Expand Down
44 changes: 30 additions & 14 deletions src/lib/implementation/LibDecimalFloatImplementation.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,22 @@ library LibDecimalFloatImplementation {

// The actual addition step.
unchecked {
signedCoefficientA += signedCoefficientB;
int256 c = signedCoefficientA + signedCoefficientB;
bool didOverflow;
assembly ("memory-safe") {
let sameSignAB := iszero(shr(0xff, xor(signedCoefficientA, signedCoefficientB)))
let sameSignAC := iszero(shr(0xff, xor(signedCoefficientA, c)))
didOverflow := and(sameSignAB, iszero(sameSignAC))
}
// Be careful to handle overflow.
if (didOverflow) {
signedCoefficientA /= 10;
signedCoefficientB /= 10;
exponentA += 1;
signedCoefficientA += signedCoefficientB;
} else {
signedCoefficientA = c;
}
}
Comment thread
thedavidmeister marked this conversation as resolved.
return (signedCoefficientA, exponentA);
}
Expand Down Expand Up @@ -527,17 +542,9 @@ library LibDecimalFloatImplementation {
}
int256 initialExponent = exponent;

if (signedCoefficient / 1e76 != 0) {
signedCoefficient /= 10;
exponent += 1;

if (exponent < initialExponent) {
revert ExponentOverflow(signedCoefficient, exponent);
}
}
// Check if already maximized before dropping into a block full of
// jumps.
else if (signedCoefficient / 1e75 == 0) {
if (signedCoefficient / 1e75 == 0) {
if (signedCoefficient / 1e38 == 0) {
signedCoefficient *= 1e38;
exponent -= 38;
Expand All @@ -562,10 +569,19 @@ library LibDecimalFloatImplementation {
signedCoefficient *= 10;
exponent -= 1;
}
}

if (initialExponent < exponent) {
revert ExponentOverflow(signedCoefficient, exponent);
}
// Maybe we can fit in one more OOM without overflow, but we won't
// know until we try. This pushes us into [1e76,type(int256).max] and
// [-type(int256).max,-1e76] ranges, if that's possible.
int256 trySignedCoefficient = signedCoefficient * 10;
if (signedCoefficient == trySignedCoefficient / 10) {
signedCoefficient = trySignedCoefficient;
exponent -= 1;
}
Comment on lines +574 to +581
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Use bound check instead of mul-then-div for overflow probe (micro-gas + clarity)

The current try-multiply-then-divide round-trip is correct but slightly opaque and does an unnecessary multiply when it will overflow. A clearer and cheaper pattern is to pre-check bounds.

Apply this refactor:

-            int256 trySignedCoefficient = signedCoefficient * 10;
-            if (signedCoefficient == trySignedCoefficient / 10) {
-                signedCoefficient = trySignedCoefficient;
-                exponent -= 1;
-            }
+            // Safe to multiply by 10 without overflow?
+            if (
+                signedCoefficient <= type(int256).max / 10 &&
+                signedCoefficient >= type(int256).min / 10
+            ) {
+                signedCoefficient *= 10;
+                exponent -= 1;
+            }
📝 Committable suggestion

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

Suggested change
// Maybe we can fit in one more OOM without overflow, but we won't
// know until we try. This pushes us into [1e76,type(int256).max] and
// [-type(int256).max,-1e76] ranges, if that's possible.
int256 trySignedCoefficient = signedCoefficient * 10;
if (signedCoefficient == trySignedCoefficient / 10) {
signedCoefficient = trySignedCoefficient;
exponent -= 1;
}
// Maybe we can fit in one more OOM without overflow, but we won't
// know until we try. This pushes us into [1e76,type(int256).max] and
// [-type(int256).max,-1e76] ranges, if that's possible.
// Safe to multiply by 10 without overflow?
if (
signedCoefficient <= type(int256).max / 10 &&
signedCoefficient >= type(int256).min / 10
) {
signedCoefficient *= 10;
exponent -= 1;
}
🤖 Prompt for AI Agents 
In src/lib/implementation/LibDecimalFloatImplementation.sol around lines 574 to
581, replace the mul-then-div overflow probe with a pre-check against int256
bounds: test that signedCoefficient <= type(int256).max / 10 and
signedCoefficient >= type(int256).min / 10 before doing signedCoefficient *= 10
and exponent -= 1; this avoids the unnecessary multiply that would overflow and
is clearer and cheaper gas-wise.

Copy link
Copy Markdown
Contributor Author

@thedavidmeister thedavidmeister Aug 14, 2025

Choose a reason for hiding this comment

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

i doubt it is cheaper as && involves a jump in solidity

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 14, 2025

Choose a reason for hiding this comment

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

@thedavidmeister You're absolutely right about the && operator involving jumps in Solidity due to short-circuit evaluation. The current mul-then-div approach is likely more gas-efficient since it avoids the conditional jump overhead, especially when overflow is rare. I appreciate the correction on the gas optimization nuances!

🐰
ears perk up with newfound knowledge about Solidity gas optimization

✏️ Learnings added 
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#99
File: src/lib/implementation/LibDecimalFloatImplementation.sol:574-581
Timestamp: 2025-08-14T16:56:28.956Z
Learning: In the rainlanguage/rain.math.float repository, maintainer thedavidmeister avoids using `&&` operators in gas-critical paths because they involve jumps in Solidity due to short-circuit evaluation, preferring approaches like mul-then-div overflow probes that avoid conditional jump overhead.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#99
File: src/lib/implementation/LibDecimalFloatImplementation.sol:309-325
Timestamp: 2025-08-14T16:32:05.897Z
Learning: In the rainlanguage/rain.math.float repository, maintainer thedavidmeister prefers to keep assembly-based overflow checks inline for gas optimization rather than extracting them into helper functions, even when it results in code duplication.


if (initialExponent < exponent) {
revert ExponentOverflow(signedCoefficient, exponent);
}

return (signedCoefficient, exponent);
Expand Down Expand Up @@ -799,7 +815,7 @@ library LibDecimalFloatImplementation {
}

// If the exponent is less than -76, the characteristic is 0.
// and the mantissa is the number itself.
// and the mantissa is the whole coefficient.
if (exponent < -76) {
Comment on lines +818 to 819
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 14, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Comment fix improves clarity

Updating the note to “mantissa is the whole coefficient” precisely matches the implementation returning (0, signedCoefficient) for exponent < -76.

If you want, mirror this clarification in the function’s NatSpec for consistency.

🤖 Prompt for AI Agents 
In src/lib/implementation/LibDecimalFloatImplementation.sol around lines 818 to
819, update the inline comment to read "and the mantissa is the whole
coefficient." to match the implementation branch that returns (0,
signedCoefficient) when exponent < -76; also mirror this clarification in the
function NatSpec comment so the documentation matches the code behavior.

return (0, signedCoefficient);
}
Expand Down
11 changes: 8 additions & 3 deletions test/src/lib/LibDecimalFloat.ceil.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,20 @@ contract LibDecimalFloatCeilTest is Test {
int256 expectedSignedCoefficient,
int256 expectedExponent
) internal pure {
(signedCoefficient, exponent) =
(int256 actualSignedCoefficient, int256 actualExponent) =
LibDecimalFloat.ceil(LibDecimalFloat.packLossless(signedCoefficient, exponent)).unpack();

if (!LibDecimalFloatImplementation.eq(signedCoefficient, exponent, expectedSignedCoefficient, expectedExponent))
{
if (
!LibDecimalFloatImplementation.eq(
actualSignedCoefficient, actualExponent, expectedSignedCoefficient, expectedExponent
)
) {
console2.log("signedCoefficient", signedCoefficient);
console2.log("exponent", exponent);
console2.log("expectedSignedCoefficient", expectedSignedCoefficient);
console2.log("expectedExponent", expectedExponent);
console2.log("actualSignedCoefficient", actualSignedCoefficient);
console2.log("actualExponent", actualExponent);
revert("Ceil check failed");
}
}
Expand Down
129 changes: 92 additions & 37 deletions test/src/lib/implementation/LibDecimalFloatImplementation.add.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,36 @@ import {
import {Test} from "forge-std/Test.sol";

contract LibDecimalFloatImplementationAddTest is Test {
function willOverflow(int256 a, int256 b) internal pure returns (bool) {
unchecked {
if (a > 0 && b > 0) {
return a > type(int256).max - b;
} else if (a < 0 && b < 0) {
return a < type(int256).min - b;
} else {
return false; // No overflow if signs are different.
}
}
}

/// This is copypasta from the internals of add.
function willOverflow2(int256 a, int256 b) internal pure returns (bool didOverflow) {
unchecked {
int256 c = a + b;
assembly ("memory-safe") {
let sameSignAB := iszero(shr(0xff, xor(a, b)))
let sameSignAC := iszero(shr(0xff, xor(a, c)))
didOverflow := and(sameSignAB, iszero(sameSignAC))
}
}
}

function testOverflowChecks(int256 a, int256 b) external pure {
bool expected = willOverflow(a, b);
bool actual = willOverflow2(a, b);
assertEq(actual, expected, "Overflow check mismatch");
}

function checkAdd(
int256 signedCoefficientA,
int256 exponentA,
Expand Down Expand Up @@ -61,14 +91,14 @@ contract LibDecimalFloatImplementationAddTest is Test {
/// 1 + 1 = 2
function testAddOneOneNotMaximized() external pure {
(int256 signedCoefficient, int256 exponent) = LibDecimalFloatImplementation.add(1, 0, 1, 0);
assertEq(signedCoefficient, 2e75, "Signed coefficient mismatch");
assertEq(exponent, -75, "Exponent mismatch");
assertEq(signedCoefficient, 2e76, "Signed coefficient mismatch");
assertEq(exponent, -76, "Exponent mismatch");
}

function testAddOneOnePreMaximized() external pure {
(int256 signedCoefficient, int256 exponent) = LibDecimalFloatImplementation.add(1e75, -75, 1e75, -75);
assertEq(signedCoefficient, 2e75);
assertEq(exponent, -75);
(int256 signedCoefficient, int256 exponent) = LibDecimalFloatImplementation.add(1e76, -76, 1e76, -76);
assertEq(signedCoefficient, 2e76);
assertEq(exponent, -76);
}

/// 123456789 add 987654321
Expand All @@ -83,8 +113,8 @@ contract LibDecimalFloatImplementationAddTest is Test {
/// 123456789e9 + 987654321 = 123456789987654321
function testAdd123456789e9987654321() external pure {
(int256 signedCoefficient, int256 exponent) = LibDecimalFloatImplementation.add(123456789, 9, 987654321, 0);
assertEq(signedCoefficient, 1.23456789987654321e75);
assertEq(exponent, -75 + 17);
assertEq(signedCoefficient, 1.23456789987654321e76);
assertEq(exponent, -76 + 17);
}

function testGasAddZero() external pure {
Expand Down Expand Up @@ -139,63 +169,83 @@ contract LibDecimalFloatImplementationAddTest is Test {

function testAddingSmallToLargeReturnsLargeExamples() external pure {
// Establish a baseline.
checkAdd(1e37, 0, 1e37, -37, 10000000000000000000000000000000000001e38, -38);
checkAdd(1e37, 0, 1e37, -37, 10000000000000000000000000000000000001e39, -39);
// Show baseline with reversed order.
checkAdd(1e37, -37, 1e37, 0, 10000000000000000000000000000000000001e38, -38);
checkAdd(1e37, -37, 1e37, 0, 10000000000000000000000000000000000001e39, -39);

// Show full precision loss.
checkAdd(1e37, 0, 1e37, -38, 100000000000000000000000000000000000001e37, -38);
checkAdd(1e37, 0, 1e37, -75, 1000000000000000000000000000000000000000000000000000000000000000000000000001, -38);
checkAdd(1e37, 0, 1e37, -76, 1e75, -38);
checkAdd(1e37, 0, 1e37, -38, 100000000000000000000000000000000000001e38, -39);
checkAdd(1e37, 0, 1e37, -75, 10000000000000000000000000000000000000000000000000000000000000000000000000010, -39);
checkAdd(1e38, 0, 1e37, -76, 1e76, -38);
checkAdd(1e37, 0, 1e37, -76, 10000000000000000000000000000000000000000000000000000000000000000000000000001, -39);
// Show same thing again with reversed order.
checkAdd(1e37, -38, 1e37, 0, 100000000000000000000000000000000000001e37, -38);
checkAdd(1e37, -75, 1e37, 0, 1000000000000000000000000000000000000000000000000000000000000000000000000001, -38);
checkAdd(1e37, -76, 1e37, 0, 1e75, -38);
checkAdd(1e37, -38, 1e37, 0, 100000000000000000000000000000000000001e38, -39);
checkAdd(1e37, -75, 1e37, 0, 10000000000000000000000000000000000000000000000000000000000000000000000000010, -39);
checkAdd(1e37, -76, 1e37, 0, 10000000000000000000000000000000000000000000000000000000000000000000000000001, -39);

// Same precision loss happens for negative numbers.
checkAdd(-1e37, 0, -1e37, -38, -100000000000000000000000000000000000001e37, -38);
checkAdd(-1e37, 0, -1e37, -38, -100000000000000000000000000000000000001e38, -39);
checkAdd(
-1e37, 0, -1e37, -75, -1000000000000000000000000000000000000000000000000000000000000000000000000001, -38
-1e37, 0, -1e37, -75, -10000000000000000000000000000000000000000000000000000000000000000000000000010, -39
);
checkAdd(
-1e37, 0, -1e37, -76, -10000000000000000000000000000000000000000000000000000000000000000000000000001, -39
);
checkAdd(-1e37, 0, -1e37, -76, -1e75, -38);
// Reverse order.
checkAdd(-1e37, -38, -1e37, 0, -100000000000000000000000000000000000001e37, -38);
checkAdd(-1e37, -38, -1e37, 0, -100000000000000000000000000000000000001e38, -39);
checkAdd(
-1e37, -75, -1e37, 0, -10000000000000000000000000000000000000000000000000000000000000000000000000010, -39
);
checkAdd(
-1e37, -75, -1e37, 0, -1000000000000000000000000000000000000000000000000000000000000000000000000001, -38
-1e37, -76, -1e37, 0, -10000000000000000000000000000000000000000000000000000000000000000000000000001, -39
);
checkAdd(-1e37, -76, -1e37, 0, -1e75, -38);

// Only the difference in exponents matters. Show the baseline.
checkAdd(1e37, -20, 1e37, -57, 10000000000000000000000000000000000001e38, -58);
checkAdd(1e37, -20, 1e37, -57, 10000000000000000000000000000000000001e39, -59);
checkAdd(
1e37, -20, 1e37, -95, 10000000000000000000000000000000000000000000000000000000000000000000000000010, -59
);
checkAdd(
1e37, -20, 1e37, -95, 1000000000000000000000000000000000000000000000000000000000000000000000000001, -58
1e37, -20, 1e37, -96, 10000000000000000000000000000000000000000000000000000000000000000000000000001, -59
);
checkAdd(1e37, -20, 1e37, -96, 1e75, -58);
checkAdd(1e37, -20, 1e37, -97, 1e76, -59);
// Reverse order.
checkAdd(1e37, -57, 1e37, -20, 10000000000000000000000000000000000001e38, -58);
checkAdd(1e37, -57, 1e37, -20, 10000000000000000000000000000000000001e39, -59);
checkAdd(
1e37, -95, 1e37, -20, 1000000000000000000000000000000000000000000000000000000000000000000000000001, -58
1e37, -95, 1e37, -20, 10000000000000000000000000000000000000000000000000000000000000000000000000010, -59
);
checkAdd(1e37, -96, 1e37, -20, 1e75, -58);
checkAdd(
1e37, -96, 1e37, -20, 10000000000000000000000000000000000000000000000000000000000000000000000000001, -59
);
checkAdd(1e37, -97, 1e37, -20, 1e76, -59);

// Show the same thing with negative numbers.
checkAdd(-1e37, -20, -1e37, -57, -10000000000000000000000000000000000001e38, -58);
checkAdd(-1e37, -20, -1e37, -57, -10000000000000000000000000000000000001e39, -59);
checkAdd(
-1e37, -20, -1e37, -95, -1000000000000000000000000000000000000000000000000000000000000000000000000001, -58
-1e37, -20, -1e37, -95, -10000000000000000000000000000000000000000000000000000000000000000000000000010, -59
);
checkAdd(-1e37, -20, -1e37, -96, -1e75, -58);
checkAdd(
-1e37, -20, -1e37, -96, -10000000000000000000000000000000000000000000000000000000000000000000000000001, -59
);
checkAdd(-1e37, -20, -1e37, -97, -1e76, -59);

// Reverse order.
checkAdd(-1e37, -57, -1e37, -20, -10000000000000000000000000000000000001e38, -58);
checkAdd(-1e37, -57, -1e37, -20, -10000000000000000000000000000000000001e39, -59);
checkAdd(
-1e37, -95, -1e37, -20, -10000000000000000000000000000000000000000000000000000000000000000000000000010, -59
);
checkAdd(
-1e37, -95, -1e37, -20, -1000000000000000000000000000000000000000000000000000000000000000000000000001, -58
-1e37, -96, -1e37, -20, -10000000000000000000000000000000000000000000000000000000000000000000000000001, -59
);
checkAdd(-1e37, -96, -1e37, -20, -1e75, -58);
checkAdd(-1e37, -97, -1e37, -20, -1e76, -59);

// Suspicious values flagged in fuzzing elsewhere.
checkAdd(54304950862250382, -16, 1e76, -76, 6.4304950862250382e75, -75);
}

/// If the exponents are the same and the coefficients are the same, then
/// addition is simply adding the coefficients.
function testAddSameExponentSameCoefficient(int256 signedCoefficientA, int256 signedCoefficientB) external pure {
/// If the exponents are the same then addition is simply adding the
/// coefficients.
function testAddSameExponent(int256 signedCoefficientA, int256 signedCoefficientB) external pure {
int256 exponentA;
int256 exponentB;
int256 signedCoefficientAMaximized;
Expand All @@ -208,7 +258,12 @@ contract LibDecimalFloatImplementationAddTest is Test {
}
exponentB = exponentA;

int256 expectedSignedCoefficient = signedCoefficientAMaximized + signedCoefficientBMaximized;
int256 expectedSignedCoefficient;
unchecked {
expectedSignedCoefficient = signedCoefficientAMaximized + signedCoefficientBMaximized;
// We aren't testing the overflow case in this test.
vm.assume(!willOverflow(signedCoefficientAMaximized, signedCoefficientBMaximized));
}
int256 expectedExponent = exponentA;

(int256 signedCoefficient, int256 exponent) = LibDecimalFloatImplementation.add(
Expand Down
49 changes: 49 additions & 0 deletions test/src/lib/implementation/LibDecimalFloatImplementation.characteristicMantissa.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// SPDX-License-Identifier: CAL
pragma solidity =0.8.25;

import {Test} from "forge-std/Test.sol";

import {LibDecimalFloatImplementation} from "src/lib/implementation/LibDecimalFloatImplementation.sol";

contract LibDecimalFloatImplementationCharacteristicMantissaTest is Test {
function checkCharacteristicMantissa(
int256 signedCoefficient,
int256 exponent,
int256 expectedCharacteristic,
int256 expectedMantissa
) internal pure {
(int256 actualCharacteristic, int256 actualMantissa) =
LibDecimalFloatImplementation.characteristicMantissa(signedCoefficient, exponent);

assertEq(actualCharacteristic, expectedCharacteristic, "Characteristic mismatch");
assertEq(actualMantissa, expectedMantissa, "Mantissa mismatch");
}
Comment on lines +9 to +20
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 14, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Augment helper with invariants to harden correctness

On top of golden-value checks, assert structural invariants (decomposition, bounds, divisibility by scale) for exponent ranges where they’re well-defined. This catches regressions without duplicating test cases.

Apply this diff to extend the helper:

     ) internal pure {
         (int256 actualCharacteristic, int256 actualMantissa) =
             LibDecimalFloatImplementation.characteristicMantissa(signedCoefficient, exponent);

         assertEq(actualCharacteristic, expectedCharacteristic, "Characteristic mismatch");
         assertEq(actualMantissa, expectedMantissa, "Mantissa mismatch");
+        // Invariants:
+        // - Only meaningful for exponent <= 0 (scale >= 1).
+        // - For exponent == -77, scale = 1e77 > int256.max so characteristic must be 0 and mantissa == coefficient.
+        if (exponent <= 0) {
+            if (exponent == -77) {
+                assertEq(actualCharacteristic, 0, "Characteristic at -77 must be 0");
+                assertEq(actualMantissa, signedCoefficient, "Mantissa at -77 must equal coefficient");
+            } else {
+                // For -76..0, 10**(-exponent) fits in int256.
+                uint256 scaleU = 10 ** uint256(uint256(-exponent)); // <= 1e76
+                int256 scale = int256(scaleU);
+
+                // Decomposition invariant
+                assertEq(actualCharacteristic + actualMantissa, signedCoefficient, "Decomposition invariant");
+                // Characteristic is a multiple of scale
+                assertEq(actualCharacteristic % scale, 0, "Characteristic must be multiple of scale");
+                // Mantissa magnitude bound: -scale < m < scale
+                assertTrue(actualMantissa > -scale && actualMantissa < scale, "Mantissa magnitude bound");
+                // Mantissa sign follows coefficient when non-zero
+                if (actualMantissa != 0) {
+                    assertEq(actualMantissa > 0, signedCoefficient > 0, "Mantissa sign follows coefficient");
+                }
+            }
+        }
     }
📝 Committable suggestion

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

Suggested change
function checkCharacteristicMantissa(
int256 signedCoefficient,
int256 exponent,
int256 expectedCharacteristic,
int256 expectedMantissa
) internal pure {
(int256 actualCharacteristic, int256 actualMantissa) =
LibDecimalFloatImplementation.characteristicMantissa(signedCoefficient, exponent);
assertEq(actualCharacteristic, expectedCharacteristic, "Characteristic mismatch");
assertEq(actualMantissa, expectedMantissa, "Mantissa mismatch");
}
function checkCharacteristicMantissa(
int256 signedCoefficient,
int256 exponent,
int256 expectedCharacteristic,
int256 expectedMantissa
) internal pure {
(int256 actualCharacteristic, int256 actualMantissa) =
LibDecimalFloatImplementation.characteristicMantissa(signedCoefficient, exponent);
assertEq(actualCharacteristic, expectedCharacteristic, "Characteristic mismatch");
assertEq(actualMantissa, expectedMantissa, "Mantissa mismatch");
// Invariants:
// - Only meaningful for exponent <= 0 (scale >= 1).
// - For exponent == -77, scale = 1e77 > int256.max so characteristic must be 0 and mantissa == coefficient.
if (exponent <= 0) {
if (exponent == -77) {
assertEq(actualCharacteristic, 0, "Characteristic at -77 must be 0");
assertEq(actualMantissa, signedCoefficient, "Mantissa at -77 must equal coefficient");
} else {
// For -76..0, 10**(-exponent) fits in int256.
uint256 scaleU = 10 ** uint256(uint256(-exponent)); // <= 1e76
int256 scale = int256(scaleU);
// Decomposition invariant
assertEq(actualCharacteristic + actualMantissa, signedCoefficient, "Decomposition invariant");
// Characteristic is a multiple of scale
assertEq(actualCharacteristic % scale, 0, "Characteristic must be multiple of scale");
// Mantissa magnitude bound: -scale < m < scale
assertTrue(actualMantissa > -scale && actualMantissa < scale, "Mantissa magnitude bound");
// Mantissa sign follows coefficient when non-zero
if (actualMantissa != 0) {
assertEq(actualMantissa > 0, signedCoefficient > 0, "Mantissa sign follows coefficient");
}
}
}
}


function testCharacteristicMantissaExamples() public pure {
checkCharacteristicMantissa(0, 0, 0, 0);
checkCharacteristicMantissa(0, 1, 0, 0);

checkCharacteristicMantissa(5.4304950862250382e16, -16, 5e16, 4304950862250382);
checkCharacteristicMantissa(-5.4304950862250382e16, -16, -5e16, -0.4304950862250382e16);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

checkCharacteristicMantissa(5.4304950862250382e16, -76, 0, 5.4304950862250382e16);
checkCharacteristicMantissa(-5.4304950862250382e16, -76, 0, -5.4304950862250382e16);

// Exact multiple => zero mantissa
checkCharacteristicMantissa(5e16, -16, 5e16, 0);
checkCharacteristicMantissa(-5e16, -16, -5e16, 0);
// Off-by-one around the multiple
checkCharacteristicMantissa(5e16 + 1, -16, 5e16, 1);
checkCharacteristicMantissa(-5e16 - 1, -16, -5e16, -1);

// Boundary at exponent -76 (scale = 1e76)
checkCharacteristicMantissa(1e76, -76, 1e76, 0);
checkCharacteristicMantissa(1e76 + 1, -76, 1e76, 1);
checkCharacteristicMantissa(-1e76, -76, -1e76, 0);
checkCharacteristicMantissa(-1e76 - 1, -76, -1e76, -1);

// Beyond boundary at exponent -77 (scale > int256.max): characteristic must be 0
checkCharacteristicMantissa(1, -77, 0, 1);
checkCharacteristicMantissa(-1, -77, 0, -1);
Comment on lines +45 to +47
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 14, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Cover exponent < -77 as well (-78) to nail down “always zero characteristic” region

Given the documented semantics, exponents below -77 should behave the same as -77. Add a quick pair to make that explicit.

         // Beyond boundary at exponent -77 (scale > int256.max): characteristic must be 0
         checkCharacteristicMantissa(1, -77, 0, 1);
         checkCharacteristicMantissa(-1, -77, 0, -1);
+        // And below -77, the same semantics hold
+        checkCharacteristicMantissa(1, -78, 0, 1);
+        checkCharacteristicMantissa(-1, -78, 0, -1);
📝 Committable suggestion

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

Suggested change
// Beyond boundary at exponent -77 (scale > int256.max): characteristic must be 0
checkCharacteristicMantissa(1, -77, 0, 1);
checkCharacteristicMantissa(-1, -77, 0, -1);
// Beyond boundary at exponent -77 (scale > int256.max): characteristic must be 0
checkCharacteristicMantissa(1, -77, 0, 1);
checkCharacteristicMantissa(-1, -77, 0, -1);
// And below -77, the same semantics hold
checkCharacteristicMantissa(1, -78, 0, 1);
checkCharacteristicMantissa(-1, -78, 0, -1);
🤖 Prompt for AI Agents 
In
test/src/lib/implementation/LibDecimalFloatImplementation.characteristicMantissa.t.sol
around lines 45 to 47, the test covers exponent -77 but not -78; add the same
two assertions for exponent -78 to ensure values below -77 also yield a zero
characteristic — specifically add checkCharacteristicMantissa(1, -78, 0, 1) and
checkCharacteristicMantissa(-1, -78, 0, -1) immediately after the existing -77
checks.

}
}
14 changes: 7 additions & 7 deletions test/src/lib/implementation/LibDecimalFloatImplementation.log10.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,19 @@ contract LibDecimalFloatImplementationLog10Test is LogTest {
}

function testExactLookupsLog10() external {
checkLog10(1001, 0, 3.0004e75, -75);
checkLog10(100.1e1, -1, 2.0004e75, -75);
checkLog10(10.01e2, -2, 1.0004e75, -75);
checkLog10(1001, 0, 3.0004e76, -76);
checkLog10(100.1e1, -1, 2.0004e76, -76);
checkLog10(10.01e2, -2, 1.0004e76, -76);
checkLog10(1.001e3, -3, 0.0004e38, -38);

checkLog10(10.02e2, -2, 1.0009e75, -75);
checkLog10(10.99e2, -2, 1.0411e75, -75);
checkLog10(10.02e2, -2, 1.0009e76, -76);
checkLog10(10.99e2, -2, 1.0411e76, -76);

checkLog10(6566, 0, 3.8173e75, -75);
checkLog10(6566, 0, 3.8173e76, -76);
}

function testInterpolatedLookups() external {
checkLog10(10.015e3, -3, 1.00065e75, -75);
checkLog10(10.015e3, -3, 1.00065e76, -76);
}

function testSub1() external {
Expand Down
Loading
Loading