ceil#96
Conversation
WalkthroughAdded ceil(Float) and a public DecimalFloat.ceil wrapper; optimized floor(Float). Introduced maximize(...) and increased ADD_MAX_EXPONENT_DIFF, switching the add path to maximize operands. Added maximize and ceil tests, updated many tests to expect maximized-scale outputs, and refreshed gas-snapshot data. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LibDecimalFloat
participant LibDecimalFloatImplementation
Caller->>LibDecimalFloat: ceil(Float f)
alt exponent >= 0
LibDecimalFloat-->>Caller: return f
else
LibDecimalFloat->>LibDecimalFloat: unpack characteristic & mantissa
alt mantissa == 0
LibDecimalFloat-->>Caller: return f
else
LibDecimalFloat->>LibDecimalFloatImplementation: add(char, exp, 1e75, -75) if mantissa>0
LibDecimalFloatImplementation-->>LibDecimalFloat: adjusted packed result
LibDecimalFloat-->>Caller: return ceiled Float
end
end
Caller->>LibDecimalFloat: floor(Float f)
alt exponent >= 0
LibDecimalFloat-->>Caller: return f
else
LibDecimalFloat->>LibDecimalFloat: truncate mantissa, packLossy -> return floored Float
end
Caller->>LibDecimalFloatImplementation: add(a, b)
LibDecimalFloatImplementation->>LibDecimalFloatImplementation: maximize(a)
LibDecimalFloatImplementation->>LibDecimalFloatImplementation: maximize(b)
alt exponents differ
LibDecimalFloatImplementation->>LibDecimalFloatImplementation: align by dividing smaller coeff (10^diff)
end
LibDecimalFloatImplementation->>LibDecimalFloatImplementation: sum coefficients (use larger exponent)
LibDecimalFloatImplementation-->>Caller: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (5)
src/lib/LibDecimalFloat.sol (1)
613-625: floor is incorrect for negative values; truncates toward zero instead of −∞With negative inputs and non-zero mantissa,
characteristicis the truncation toward zero (e.g., floor(-1.2) should be -2, but returns -1). Fix by subtracting 1 when mantissa < 0.function floor(Float float) internal pure returns (Float) { (int256 signedCoefficient, int256 exponent) = float.unpack(); // If the exponent is 0 or greater then the float is already an integer. if (exponent >= 0) { return float; } (int256 characteristic, int256 mantissa) = LibDecimalFloatImplementation.characteristicMantissa(signedCoefficient, exponent); - (Float result, bool lossless) = packLossy(characteristic, exponent); + (Float result, bool lossless) = packLossy(characteristic, exponent); + // Adjust for negatives: true floor moves to the next more-negative integer. + if (mantissa < 0) { + result = sub(result, FLOAT_ONE); + } // Flooring is lossy by definition. (lossless, mantissa); return result; }src/lib/implementation/LibDecimalFloatImplementation.sol (3)
709-721: Use ADD_MAX_EXPONENT_DIFF instead of literal 76 for rescale boundscompareRescale uses a hard-coded 76. Replace with ADD_MAX_EXPONENT_DIFF (and ensure overflow paths remain correct).
747-770: Same constant leakage here: replace 76 with ADD_MAX_EXPONENT_DIFF in withTargetExponent()Keeps the exponent-difference policy consistent from a single source of truth.
785-791: Characteristic/mantissa cutoff (-76) should also reference ADD_MAX_EXPONENT_DIFFHelps avoid drift if the limit changes again.
test/src/lib/implementation/LibDecimalFloatImplementation.add.t.sol (1)
119-121: Fuzz test cannot be pure: uses vm.assume/bound/maximize/assertMark as non-pure (or view) to compile.
- ) public pure { + ) public {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.gas-snapshot(3 hunks)src/lib/LibDecimalFloat.sol(2 hunks)src/lib/implementation/LibDecimalFloatImplementation.sol(5 hunks)test/src/lib/LibDecimalFloat.ceil.t.sol(1 hunks)test/src/lib/LibDecimalFloat.pow.t.sol(1 hunks)test/src/lib/LibDecimalFloat.sqrt.t.sol(2 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.add.t.sol(4 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.log10.t.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.pow10.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#59
File: crates/float/src/lib.rs:233-242
Timestamp: 2025-06-17T10:17:56.205Z
Learning: In the rainlanguage/rain.math.float repository, the maintainer 0xgleb prefers to handle documentation additions and improvements in separate issues rather than inline with feature PRs.
📚 Learning: 2025-06-16T13:17:28.513Z
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
Applied to files:
src/lib/LibDecimalFloat.soltest/src/lib/LibDecimalFloat.ceil.t.sol
📚 Learning: 2025-07-24T04:32:14.171Z
Learnt from: rouzwelt
PR: rainlanguage/rain.math.float#83
File: src/concrete/DecimalFloat.sol:248-251
Timestamp: 2025-07-24T04:32:14.171Z
Learning: In the rainlanguage/rain.math.float project, functions in DecimalFloat.sol that return tuples from LibDecimalFloat calls must unpack the tuple into local variables before returning them (rather than returning directly) to maintain compatibility with Slither static analysis checks.
Applied to files:
src/lib/LibDecimalFloat.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (16)
src/lib/LibDecimalFloat.sol (1)
615-618: Good micro-optimization: early return when already an integerEarly-exiting floor for exponent >= 0 avoids unnecessary work. No functional risk.
src/lib/implementation/LibDecimalFloatImplementation.sol (3)
59-63: Maximized zero constants mirror normalized ones — OKValues mirror normalized zero; safe and consistent.
278-283: Using maximize before alignment in add() improves stability across OOMsMaximizing both operands prior to alignment is a sound change and matches the increased exponent-diff tolerance. LGTM.
16-16: Widening ADD_MAX_EXPONENT_DIFF to 76 is reasonable; ensure consistency across codeadd() now respects this limit; however other places still use the literal 76. Recommend centralizing to the constant for maintainability.
test/src/lib/implementation/LibDecimalFloatImplementation.log10.t.sol (1)
46-47: Interpolated lookup expectation aligns with new scaleThe updated expected (≈1.00065e75, -75) is consistent. No issues.
test/src/lib/LibDecimalFloat.pow.t.sol (1)
128-144: Rename only; behavior unchangedThe fuzz test logic remains intact under the new name. All good.
test/src/lib/LibDecimalFloat.sqrt.t.sol (3)
87-91: Rename LGTMRenaming testRoundTripFuzz to testRoundTripFuzzSqrt matches conventions in other test contracts. Looks good.
56-57: Inconsistent scaling in sqrt test expectationsThe test at test/src/lib/LibDecimalFloat.sqrt.t.sol mixes normalized and maximized representations:
• Lines 54–55 use small-scale outputs (2e3 @ –3, 4e3 @ –3)
• Line 56 expects a maximized coefficient (…0000 @ –66)Please confirm which scaling convention the
sqrtimplementation now uses, then make them consistent by either:
- Converting the first two cases to maximized form, or
- Reverting the
sqrt(16)expectation back to normalized form (e.g., 4e3 @ –3).
70-74: Verify normalize usage forsqrterror pathI wasn’t able to locate the
sqrtimplementation in the repo, so please confirm whether it still callsnormalize(...)before reverting on negative inputs. If it now usesmaximize(...)to prepare its revert arguments, update the test accordingly; otherwise, no change is needed.
- test/src/lib/LibDecimalFloat.sqrt.t.sol (lines 70–74): currently uses
– Switch to(int256 signedCoefficientNormalized, int256 exponentNormalized) = LibDecimalFloatImplementation.normalize(signedCoefficient, exponent); vm.expectRevert( abi.encodeWithSelector(Log10Negative.selector, signedCoefficientNormalized, exponentNormalized) ); this.sqrtExternal(a, tables);LibDecimalFloatImplementation.maximize(...)here if thesqrtimplementation encodes the maximized values.
– Otherwise, keep usingnormalize(...)to match the current revert behavior..gas-snapshot (2)
257-263: Fuzz test rename reflectedRenamed fuzz tests (Pow/Sqrt) are represented here. No action needed.
35-40: Action Required: Verify Updated Gas Snapshot for Ceil TestsThe
.gas-snapshotfile now includes the newLibDecimalFloatCeilTest:testCeil*entries on lines 35–40. Since the sandbox cannot runforge, please confirm locally that these entries are correct and up to date:
- File:
.gas-snapshot
Lines: 35–40 — verify thetestCeil*gas valuesTo refresh the snapshot, run locally:
forge test --gas-report forge snapshotOnce confirmed, commit the regenerated
.gas-snapshotto keep tests and benchmarks in sync.test/src/lib/implementation/LibDecimalFloatImplementation.add.t.sol (5)
62-66: Expectation update to maximized result is correctAdding two ones returns 2e75 @ -75 under maximize. LGTM.
68-72: Pre-maximized add case LGTMInputs and expected outputs are consistent with maximize.
77-80: Scaled expectation LGTM1.11111111e76 @ (-76 + 9) matches maximize semantics for 123456789 + 987654321.
124-128: Switched to maximize in fuzz pre-processingGood alignment with the implementation shift from normalize to maximize.
201-220: Minor message inconsistencySubject casing differs between messages ("Signed coefficient mismatch" vs "signed coefficient mismatch"). Consider unifying for consistency.
- assertEq(signedCoefficient, expectedSignedCoefficient, "signed coefficient mismatch"); + assertEq(signedCoefficient, expectedSignedCoefficient, "Signed coefficient mismatch");Likely an incorrect or invalid review comment.
| /// For exponents [-76,-1] the ceil is the + 1. | ||
| function testCeilInRange(int224 x, int256 exponent) external pure { | ||
| exponent = bound(exponent, -76, -1); | ||
| int256 scale = int256(10 ** uint256(-exponent)); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify docstring for the in-range behavior
Doc is truncated. Suggest explicit statement of the rule used below.
- /// For exponents [-76,-1] the ceil is the + 1.
+ /// For exponents in [-76, -1], ceil(x) = characteristic + 1 iff mantissa > 0; otherwise ceil(x) = characteristic.📝 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.
| /// For exponents [-76,-1] the ceil is the + 1. | |
| function testCeilInRange(int224 x, int256 exponent) external pure { | |
| exponent = bound(exponent, -76, -1); | |
| int256 scale = int256(10 ** uint256(-exponent)); | |
| /// For exponents in [-76, -1], ceil(x) = characteristic + 1 iff mantissa > 0; otherwise ceil(x) = characteristic. | |
| function testCeilInRange(int224 x, int256 exponent) external pure { | |
| exponent = bound(exponent, -76, -1); | |
| int256 scale = int256(10 ** uint256(-exponent)); |
🤖 Prompt for AI Agents
In test/src/lib/LibDecimalFloat.ceil.t.sol around lines 48 to 51, the docstring
for the testCeilInRange function is incomplete and unclear about the behavior
for exponents in the range [-76, -1]. Update the docstring to explicitly state
the rule applied in this range, clarifying that for these exponents, the ceiling
operation results in adding 1 to the value. Make sure the explanation is concise
and fully describes the expected behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/implementation/LibDecimalFloatImplementation.sol (1)
296-317: Update function documentation to reflect new maximization behaviorThe add() function now returns the sum aligned to the larger exponent using maximize() instead of normalize(). The function header and docstring comments should be updated to clearly state that the result uses maximization (aligned to ~1e75-1e76 range) rather than normalization (1e37-1e38 range), and that the result maintains the larger input's exponent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/lib/implementation/LibDecimalFloatImplementation.sol(5 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.maximize.t.sol(1 hunks)
🔇 Additional comments (1)
src/lib/implementation/LibDecimalFloatImplementation.sol (1)
128-130: Verify overflow handling consistency inmul
Theaddimplementation now usesmaximizeto scale operands before combining, but inmul(src/lib/implementation/LibDecimalFloatImplementation.sol lines 127–131) the overflow path still callsnormalize. If the intent is to standardize onmaximizefor both operations, consider replacing those twonormalizecalls withmaximize; otherwise please confirm that falling back tonormalizehere is intentional.Locations to review:
- src/lib/implementation/LibDecimalFloatImplementation.sol:127–131
Proposed change (optional):
if (didOverflow) { - (signedCoefficientA, exponentA) = normalize(signedCoefficientA, exponentA); - (signedCoefficientB, exponentB) = normalize(signedCoefficientB, exponentB); + (signedCoefficientA, exponentA) = maximize(signedCoefficientA, exponentA); + (signedCoefficientB, exponentB) = maximize(signedCoefficientB, exponentB); return mul(signedCoefficientA, exponentA, signedCoefficientB, exponentB); }
| function testMaximizedExamples() external pure { | ||
| checkMaximized(0, 0, 0, 0); | ||
| checkMaximized(0, 1, 0, 0); | ||
| checkMaximized(1e37, 0, 1e75, -38); | ||
| checkMaximized(1e75, 0, 1e75, 0); | ||
| checkMaximized(type(int256).max, 0, type(int256).max / 10, 1); | ||
| checkMaximized(type(int256).min, 0, type(int256).min / 10, 1); | ||
| checkMaximized(42, 0, 42e74, -74); | ||
| checkMaximized(42e74, -74, 42e74, -74); | ||
|
|
||
| for (int256 i = 76; i >= 0; i--) { | ||
| checkMaximized(int256(10 ** uint256(i)), 0, 1e75, i - 75); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding tests for ExponentOverflow conditions
The test suite covers normal operation well but doesn't test the ExponentOverflow revert conditions in the maximize function. Consider adding tests that verify the function reverts when exponent overflows occur during scaling.
Example test case:
function testMaximizeExponentOverflow() external {
// Test overflow when scaling down
vm.expectRevert();
LibDecimalFloatImplementation.maximize(1e77, type(int256).max);
// Test underflow when scaling up
vm.expectRevert();
LibDecimalFloatImplementation.maximize(1, type(int256).min);
}🤖 Prompt for AI Agents
In test/src/lib/implementation/LibDecimalFloatImplementation.maximize.t.sol
around lines 51 to 64, add tests that assert the maximize function reverts on
exponent overflow/underflow; create a new test (e.g.,
testMaximizeExponentOverflow) that calls LibDecimalFloatImplementation.maximize
with values that force an exponent overflow when scaling down (for example a
mantissa >= 1e77 with exponent = type(int256).max) and another call that forces
exponent underflow when scaling up (for example mantissa = 1 with exponent =
type(int256).min), wrapping each call with vm.expectRevert() so the test
verifies the ExponentOverflow revert path.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/lib/LibDecimalFloat.sol (1)
619-624: Add a mantissa==0 fast path in floor() to avoid unnecessary packingWhen exponent < 0 and mantissa == 0 the value is already integral; you can return the original float and skip packLossy.
(int256 characteristic, int256 mantissa) = LibDecimalFloatImplementation.characteristicMantissa(signedCoefficient, exponent); +(mantissa); // touch for clarity below +// If there is no fractional component, return the original float. +if (mantissa == 0) { + return float; +} (Float result, bool lossless) = packLossy(characteristic, exponent); // Flooring is lossy by definition. (lossless, mantissa); return result;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/LibDecimalFloat.sol(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-16T13:17:28.513Z
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
Applied to files:
src/lib/LibDecimalFloat.sol
📚 Learning: 2025-07-24T04:32:14.171Z
Learnt from: rouzwelt
PR: rainlanguage/rain.math.float#83
File: src/concrete/DecimalFloat.sol:248-251
Timestamp: 2025-07-24T04:32:14.171Z
Learning: In the rainlanguage/rain.math.float project, functions in DecimalFloat.sol that return tuples from LibDecimalFloat calls must unpack the tuple into local variables before returning them (rather than returning directly) to maintain compatibility with Slither static analysis checks.
Applied to files:
src/lib/LibDecimalFloat.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: git-clean
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
🔇 Additional comments (2)
src/lib/LibDecimalFloat.sol (2)
615-618: Good short-circuit for integer inputs in floor()Returning the original float when exponent >= 0 is correct and saves gas.
631-642: ceil() early exits look correct and consistent
- exponent >= 0: already integer → return.
- mantissa == 0: already integer at negative exponent → return.
Nice gas win and symmetry with floor().
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/lib/LibDecimalFloat.sol (1)
619-624: Optional: skip pack when mantissa is zero in floorWhen mantissa is 0 the value is already integral; you can mirror the ceil optimization and return the original float to save a pack.
(int256 characteristic, int256 mantissa) = LibDecimalFloatImplementation.characteristicMantissa(signedCoefficient, exponent); +// If there's no fractional part, return the original float (already an integer). +if (mantissa == 0) { + return float; +} (Float result, bool lossless) = packLossy(characteristic, exponent);
♻️ Duplicate comments (2)
test/src/lib/LibDecimalFloat.ceil.t.sol (2)
52-53: Clarify in-range rule in docstringMake the rule explicit for exponents in [-76, -1]: add 1 iff mantissa > 0.
-/// For exponents [-76,-1] the ceil is the + 1. +/// For exponents in [-76, -1], ceil(x) = characteristic + 1 iff mantissa > 0; otherwise ceil(x) = characteristic.
80-98: Remove duplicate example lineThe max example appears twice; the second is redundant.
checkCeil(type(int224).max, 0, type(int224).max, 0); checkCeil(type(int224).min, 0, type(int224).min, 0); checkCeil(2.5e37, -37, 3e66, -66); - checkCeil(type(int224).max, 0, type(int224).max, 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.gas-snapshot(3 hunks)src/lib/LibDecimalFloat.sol(2 hunks)test/src/lib/LibDecimalFloat.ceil.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-11T14:30:48.527Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#96
File: test/src/lib/LibDecimalFloat.ceil.t.sol:43-50
Timestamp: 2025-08-11T14:30:48.527Z
Learning: When reviewing Solidity test files using Forge, verify the actual mutability of helper functions like `bound()` before suggesting changes to function mutability specifiers, as Forge implements many test utilities as pure functions.
Applied to files:
test/src/lib/LibDecimalFloat.ceil.t.sol
📚 Learning: 2025-06-16T13:17:28.513Z
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
Applied to files:
test/src/lib/LibDecimalFloat.ceil.t.solsrc/lib/LibDecimalFloat.sol
📚 Learning: 2025-08-11T14:30:35.222Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#96
File: test/src/lib/LibDecimalFloat.ceil.t.sol:35-39
Timestamp: 2025-08-11T14:30:35.222Z
Learning: In Foundry/Forge test framework, the `bound()` function is pure - it performs mathematical clamping operations without reading contract state, so test functions using `bound()` can be marked as pure if all other operations in the function are also pure.
Applied to files:
test/src/lib/LibDecimalFloat.ceil.t.sol
📚 Learning: 2025-08-11T14:30:48.527Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#96
File: test/src/lib/LibDecimalFloat.ceil.t.sol:43-50
Timestamp: 2025-08-11T14:30:48.527Z
Learning: In Forge test contracts, the `bound()` function from the Test contract is a pure function that performs mathematical bounds checking without reading state, so test functions using `bound()` can remain `pure`.
Applied to files:
test/src/lib/LibDecimalFloat.ceil.t.sol
📚 Learning: 2025-08-11T14:32:50.404Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#96
File: test/src/lib/implementation/LibDecimalFloatImplementation.maximize.t.sol:15-29
Timestamp: 2025-08-11T14:32:50.404Z
Learning: In test code for the rain.math.float repository, redundant checks may be intentionally kept for clarity and documentation purposes, even when they could be simplified. The maintainer (thedavidmeister) prefers explicit assertions in test code to make the test's intent clear to future readers, prioritizing readability over conciseness.
Applied to files:
test/src/lib/LibDecimalFloat.ceil.t.sol
📚 Learning: 2025-06-16T13:19:32.009Z
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: crates/float/src/lib.rs:382-401
Timestamp: 2025-06-16T13:19:32.009Z
Learning: In the rainlanguage/rain.math.float codebase, variable shadowing in property tests like test_lt_eq_gt_with_add is acceptable when it represents a logical progression of the same conceptual value being tested through different states.
Applied to files:
test/src/lib/LibDecimalFloat.ceil.t.sol
📚 Learning: 2025-07-24T04:32:14.171Z
Learnt from: rouzwelt
PR: rainlanguage/rain.math.float#83
File: src/concrete/DecimalFloat.sol:248-251
Timestamp: 2025-07-24T04:32:14.171Z
Learning: In the rainlanguage/rain.math.float project, functions in DecimalFloat.sol that return tuples from LibDecimalFloat calls must unpack the tuple into local variables before returning them (rather than returning directly) to maintain compatibility with Slither static analysis checks.
Applied to files:
src/lib/LibDecimalFloat.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: git-clean
🔇 Additional comments (6)
src/lib/LibDecimalFloat.sol (2)
615-618: Good early return for integer values in floorReturning early when exponent >= 0 avoids unnecessary characteristic/mantissa work and packing for integers. Nice gas win.
631-657: Ceil logic looks correct and handles signs and extremes
- Early-return for exponent >= 0 and mantissa == 0 are correct and gas-friendly.
- For mantissa > 0, adding FLOAT_ONE is the right adjustment; with maximize and ADD_MAX_EXPONENT_DIFF=76, tiny→huge addition behaves as expected for exponents < -76.
- Parameter name float follows repo convention. All good.
test/src/lib/LibDecimalFloat.ceil.t.sol (3)
12-14: LGTM: follows Float parameter naming conventionParameter named float adheres to established repo convention. No issues.
16-33: Helper correctness: pack → ceil → unpack with explicit failure loggingThe flow is solid and compares via implementation eq. Keeping explicit logs before revert aids debugging.
100-108: Zero behavior checks are precise and comprehensiveCovers different zero encodings (packed with any exponent) and equality; good coverage.
.gas-snapshot (1)
35-40: Gas snapshot entries for ceil look consistentNew ceil tests are captured with sensible gas/μ metrics. Snapshot update looks consistent with added functionality.
…loat into 2025-08-09-ceil
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.gas-snapshot(3 hunks)src/concrete/DecimalFloat.sol(1 hunks)test/src/concrete/DecimalFloat.ceil.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-16T13:17:28.513Z
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
Applied to files:
src/concrete/DecimalFloat.soltest/src/concrete/DecimalFloat.ceil.t.sol
📚 Learning: 2025-07-24T04:32:14.171Z
Learnt from: rouzwelt
PR: rainlanguage/rain.math.float#83
File: src/concrete/DecimalFloat.sol:248-251
Timestamp: 2025-07-24T04:32:14.171Z
Learning: In the rainlanguage/rain.math.float project, functions in DecimalFloat.sol that return tuples from LibDecimalFloat calls must unpack the tuple into local variables before returning them (rather than returning directly) to maintain compatibility with Slither static analysis checks.
Applied to files:
test/src/concrete/DecimalFloat.ceil.t.sol
🪛 GitHub Actions: Git is clean
test/src/concrete/DecimalFloat.ceil.t.sol
[error] git diff --exit-code failed (exit code 1). Detected end-of-file newline issue: 'No newline at end of file' in test/src/concrete/DecimalFloat.ceil.t.sol.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
🔇 Additional comments (3)
.gas-snapshot (1)
3-3: Manual Verification of Gas Snapshot UpdateThe sandbox environment cannot execute
forge, so please confirm locally that the snapshot was regenerated cleanly with no unintended changes:• Run the full suite and regenerate the snapshot from a clean state:
forge test -vvv forge snapshot git diff -- .gas-snapshot• Verify that no unexpected diffs appear in
.gas-snapshot.src/concrete/DecimalFloat.sol (1)
169-174: LGTM: ceil wrapper mirrors floor and follows existing patterns.
- external/pure is correct.
- Uses library extension method consistently.
- NatSpec mirrors existing wrappers.
No changes requested.
test/src/concrete/DecimalFloat.ceil.t.sol (1)
18-25: Parity test pattern is solid.
- External call via this.ceilExternal captures revert data; vm.expectRevert(err) ensures deployed path reverts identically.
- unwrap comparison checks exact value parity.
No changes requested.
| function testCeilDeployed(Float a) external { | ||
| DecimalFloat deployed = new DecimalFloat(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: reuse a single DecimalFloat instance to reduce per-test deployment cost.
Repeated new DecimalFloat() per fuzz case inflates gas/time. Consider a shared instance via setUp() unless test isolation requires fresh deployments.
- DecimalFloat deployed = new DecimalFloat();
+ // Use a shared instance (initialized in setUp()).
+ DecimalFloat deployed = deployedInstance;Add once to the contract:
DecimalFloat internal deployedInstance;
function setUp() public {
deployedInstance = new DecimalFloat();
}🧰 Tools
🪛 GitHub Actions: Git is clean
[error] git diff --exit-code failed (exit code 1). Detected end-of-file newline issue: 'No newline at end of file' in test/src/concrete/DecimalFloat.ceil.t.sol.
🤖 Prompt for AI Agents
In test/src/concrete/DecimalFloat.ceil.t.sol around lines 15 to 17, avoid
creating a new DecimalFloat() inside each fuzz test; instead add a
contract-level DecimalFloat variable and initialize it once in a setUp()
function, then update testCeilDeployed to use that shared deployedInstance;
ensure visibility is internal or private as needed and keep tests that require
fresh state as exceptions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.gas-snapshot(3 hunks)src/concrete/DecimalFloat.sol(1 hunks)src/lib/LibDecimalFloat.sol(2 hunks)test/src/concrete/DecimalFloat.ceil.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-16T13:17:28.513Z
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
Applied to files:
src/concrete/DecimalFloat.soltest/src/concrete/DecimalFloat.ceil.t.solsrc/lib/LibDecimalFloat.sol
📚 Learning: 2025-07-24T04:32:14.171Z
Learnt from: rouzwelt
PR: rainlanguage/rain.math.float#83
File: src/concrete/DecimalFloat.sol:248-251
Timestamp: 2025-07-24T04:32:14.171Z
Learning: In the rainlanguage/rain.math.float project, functions in DecimalFloat.sol that return tuples from LibDecimalFloat calls must unpack the tuple into local variables before returning them (rather than returning directly) to maintain compatibility with Slither static analysis checks.
Applied to files:
test/src/concrete/DecimalFloat.ceil.t.solsrc/lib/LibDecimalFloat.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: git-clean
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
🔇 Additional comments (4)
.gas-snapshot (1)
3-3: Snapshot updates reflect new ceil coverage; OKNew DecimalFloatCeilTest and LibDecimalFloatCeilTest entries look consistent with adding ceil and related library coverage. No action needed.
Also applies to: 36-41
src/concrete/DecimalFloat.sol (1)
169-174: New ceil wrapper matches existing wrapper patterns
- External + pure is correct.
- Using-for delegation aligns with the rest of the contract.
- Docstring consistent with floor/frac style.
LGTM.
src/lib/LibDecimalFloat.sol (2)
615-618: floor early-return is a solid micro-optimizationReturning early for exponent >= 0 avoids unnecessary characteristic/mantissa/pack work. Safe and gas-friendly.
627-653: Verify exponent-diff headroom forceiladd pathThe headroom check in
LibDecimalFloatImplementation.addusesif (alignmentExponentDiff > ADD_MAX_EXPONENT_DIFF) { return (signedCoefficientA, exponentA); }with
ADD_MAX_EXPONENT_DIFF = 76. In theceilpath you always add the literal1viaLibDecimalFloatImplementation.add( characteristic, exponent, 1e75, -75 );so the maximum possible exponent-gap is
|exponent – (–75)| ≤ 76 ⇔ exponent ∈ [–151, +1]However:
- The float’s exponent domain is confined by
packLossy/packto roughly [–75, +76] (any value outside that is clamped or underflows to zero).- In practice, a
Floatnever carries an exponent < –75, so|exponent + 75| ≤ 150, and in all actual cases|exponent + 75| ≤ 75: well below the76threshold.- Test coverage in
test/src/lib/LibDecimalFloat.ceil.t.solexercises exponents down to < –76 and confirmsceil(x)>0for positivex.Conclusion: the
add(…,1e75,–75)call inceilcan never hit thealignmentExponentDiff > 76branch given the float’s allowed exponent range.Minor doc polish for consistency:
@@ src/lib/LibDecimalFloat.sol - (Float result, bool lossless) = packLossy(characteristic, exponent); - (lossless); + // Ceil is lossy by design: sub-unit precision is discarded. + (Float result, bool lossless) = packLossy(characteristic, exponent); + (lossless);
| function testCeilDeployed(Float a) external { | ||
| DecimalFloat deployed = new DecimalFloat(); | ||
|
|
||
| try this.ceilExternal(a) returns (Float b) { | ||
| Float deployedB = deployed.ceil(a); | ||
|
|
||
| assertEq(Float.unwrap(b), Float.unwrap(deployedB)); | ||
| } catch (bytes memory err) { | ||
| vm.expectRevert(err); | ||
| deployed.ceil(a); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Good parity test between library and deployed wrapper
- try/catch with vm.expectRevert(err) correctly asserts revert parity.
- Success path unwraps and compares raw values, avoiding equality pitfalls.
Consider renaming ceilExternal to internal helper in a separate contract if you later expand tests, but current approach is fine.
🤖 Prompt for AI Agents
test/src/concrete/DecimalFloat.ceil.t.sol lines 15-26: the test is correct and
requires no functional changes; keep the try/catch + vm.expectRevert pattern and
the unwrap comparison as-is for parity checks, but if you plan to expand tests
later consider moving ceilExternal into a small helper contract (rename
ceilExternal to a more descriptive internal helper or create a dedicated test
helper contract) to improve code organization and reuse.
1 participant
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests