binary search on unhappy div path#142
Merged
thedavidmeister merged 3 commits intomainfrom Sep 22, 2025
Merged
Conversation
Contributor
WalkthroughUpdates a gas-snapshot data file with many revised timing entries and refactors LibDecimalFloatImplementation.div by replacing the prior small-B slow-path with an explicit, magnitude-based multi-branch that selects scale and adjustExponent before finalizing scale and reducing it. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Lib as LibDecimalFloatImplementation.div
Caller->>Lib: div(a, b)
alt Early exits / validations
Lib-->>Caller: return (zero/NaN/inf handling)
else Small-|b| path (new branching)
Note right of Lib: If |b.coeff| < scale && fullB == false
rect rgb(240,248,255)
Note right of Lib: Select initial `scale` and `adjustExponent`<br/>by checking multiple magnitude thresholds of |b.coeff|
end
else General division path
Note over Lib: Compute initial scale/exponent via existing logic
end
Lib->>Lib: Finalize scale after binary-search step
loop Reduce scale to fit constraints
Lib->>Lib: decrement scale and adjust until fits
end
Lib-->>Caller: return quotient
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (13)📓 Common learnings📚 Learning: 2025-09-09T15:11:31.003ZApplied to files:
📚 Learning: 2025-08-21T18:03:40.347ZApplied to files:
📚 Learning: 2025-08-29T14:54:24.240ZApplied to files:
📚 Learning: 2025-08-29T10:38:26.353ZApplied to files:
📚 Learning: 2025-08-29T10:38:26.353ZApplied to files:
📚 Learning: 2025-09-22T09:29:22.086ZApplied to files:
📚 Learning: 2025-08-29T14:58:50.500ZApplied to files:
📚 Learning: 2025-08-14T16:32:05.932ZApplied to files:
📚 Learning: 2025-08-14T16:56:28.978ZApplied to files:
📚 Learning: 2025-08-26T12:49:02.313ZApplied to files:
📚 Learning: 2025-08-11T14:32:50.439ZApplied to files:
📚 Learning: 2025-08-27T13:37:22.601ZApplied to files:
🔇 Additional comments (4)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.gas-snapshot(7 hunks)src/lib/implementation/LibDecimalFloatImplementation.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-09T15:11:31.003Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#130
File: src/lib/implementation/LibDecimalFloatImplementation.sol:320-330
Timestamp: 2025-09-09T15:11:31.003Z
Learning: In LibDecimalFloatImplementation.sol div function, returning zero when underflowExponentBy > 76 is intentional "non-error underflow" behavior rather than reverting. This graceful handling is the main goal of the changes.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol.gas-snapshot
📚 Learning: 2025-08-21T18:03:40.347Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#107
File: test/lib/LibDecimalFloatSlow.sol:37-45
Timestamp: 2025-08-21T18:03:40.347Z
Learning: In test/lib/LibDecimalFloatSlow.sol, the "slow" implementation is intentionally different from the production implementation to serve as an independent reference for fuzzing tests. The goal is to have two different approaches (expensive loops vs optimized jumps) that produce equivalent results, not identical implementations.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol.gas-snapshot
📚 Learning: 2025-08-29T14:54:24.240Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: test/src/lib/LibDecimalFloat.decimal.t.sol:122-126
Timestamp: 2025-08-29T14:54:24.240Z
Learning: In test/src/lib/LibDecimalFloat.decimal.t.sol, the fromFixedDecimalLossy function converts uint256(type(int256).max) losslessly because the boundary check is `value > uint256(type(int256).max)`, not `>=`. Values exactly equal to type(int256).max still take the lossless conversion path.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol.gas-snapshot
📚 Learning: 2025-08-29T10:38:26.353Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: src/lib/implementation/LibDecimalFloatImplementation.sol:593-601
Timestamp: 2025-08-29T10:38:26.353Z
Learning: The maximize function in LibDecimalFloatImplementation.sol produces exact results for simple integer values like 1. maximize(1, 0) yields exactly (1e76, -76) with no precision loss, and the log10 special case for signedCoefficient == 1e76 correctly handles this.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol.gas-snapshot
📚 Learning: 2025-08-29T10:38:26.353Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: src/lib/implementation/LibDecimalFloatImplementation.sol:593-601
Timestamp: 2025-08-29T10:38:26.353Z
Learning: In Solidity, int256(1) when passed through the maximize function in LibDecimalFloatImplementation.sol produces exactly (1e76, -76), not an approximation. This means the special case for signedCoefficient == 1e76 in log10 correctly handles powers of 10 like log10(1).
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol.gas-snapshot
📚 Learning: 2025-09-02T09:33:32.513Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#119
File: test/src/lib/format/LibFormatDecimalFloat.toDecimalString.t.sol:174-175
Timestamp: 2025-09-02T09:33:32.513Z
Learning: The LibFormatDecimalFloat.toDecimalString function in src/lib/format/LibFormatDecimalFloat.sol does not include rounding logic. It formats decimal floats as-is without rounding values based on significant figures limits.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.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). (9)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-legal)
- GitHub Check: git-clean
🔇 Additional comments (4)
src/lib/implementation/LibDecimalFloatImplementation.sol (2)
268-359: Binary search implementation for scale determination looks correctThe nested if-else binary search efficiently determines the appropriate scale and adjustExponent values based on signedCoefficientBAbs magnitude. This replaces the previous slow-path comment with an explicit threshold-based mapping that should improve performance for division operations with small divisors.
The thresholds are correctly ordered (1e5, 1e10, 1e14, 1e19, 1e23, 1e28, 1e33, 1e38, 1e43, 1e48, 1e53, 1e58, 1e63, 1e68, 1e73) and the corresponding adjustExponent values match the scale powers.
268-359: Binary-search scale selection verified — no gaps found. Tests exercise the thresholds and boundaries (notably test/src/lib/implementation/LibDecimalFloatImplementation.div.t.sol → testUnnormalizedThreesDiv0, plus maximize.t.sol and withTargetExponent.t.sol); the coarse nested checks plus the final while-loop produce the largest power-of-ten ≤ denominator, so branches are reachable..gas-snapshot (2)
82-85: Gas optimizations show mixed results for division operationsThe gas snapshots show varying impacts on division-related tests:
testDivByNegativeOneFloat: μ decreased from 361760 to 358393 (improvement)testDivByOneFloat: μ decreased from 336347 to 333291 (improvement)testDivPacked: μ increased from 12720 to 12850 (slight regression)The improvements for specific divisor tests (by one, by negative one) suggest the binary search optimization is working as intended for these common cases.
158-159: Significant gas improvements for division edge casesNotable improvements in division tests:
testDivBy1: Shows improvement in gas usagetestDivByNegativeOneFloat: Shows improvementtestUnnormalizedThreesDiv0: Shows improvement (μ: 29911409 → 29825155)These improvements align with the PR's objective to optimize the "unhappy div path" for small divisors.
Also applies to: 166-166
Contributor
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 (1)
src/lib/implementation/LibDecimalFloatImplementation.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#130
File: src/lib/implementation/LibDecimalFloatImplementation.sol:320-330
Timestamp: 2025-09-09T15:11:31.003Z
Learning: In LibDecimalFloatImplementation.sol div function, returning zero when underflowExponentBy > 76 is intentional "non-error underflow" behavior rather than reverting. This graceful handling is the main goal of the changes.
📚 Learning: 2025-09-09T15:11:31.003Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#130
File: src/lib/implementation/LibDecimalFloatImplementation.sol:320-330
Timestamp: 2025-09-09T15:11:31.003Z
Learning: In LibDecimalFloatImplementation.sol div function, returning zero when underflowExponentBy > 76 is intentional "non-error underflow" behavior rather than reverting. This graceful handling is the main goal of the changes.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-08-21T18:03:40.347Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#107
File: test/lib/LibDecimalFloatSlow.sol:37-45
Timestamp: 2025-08-21T18:03:40.347Z
Learning: In test/lib/LibDecimalFloatSlow.sol, the "slow" implementation is intentionally different from the production implementation to serve as an independent reference for fuzzing tests. The goal is to have two different approaches (expensive loops vs optimized jumps) that produce equivalent results, not identical implementations.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-08-29T14:54:24.240Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: test/src/lib/LibDecimalFloat.decimal.t.sol:122-126
Timestamp: 2025-08-29T14:54:24.240Z
Learning: In test/src/lib/LibDecimalFloat.decimal.t.sol, the fromFixedDecimalLossy function converts uint256(type(int256).max) losslessly because the boundary check is `value > uint256(type(int256).max)`, not `>=`. Values exactly equal to type(int256).max still take the lossless conversion path.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-08-29T10:38:26.353Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: src/lib/implementation/LibDecimalFloatImplementation.sol:593-601
Timestamp: 2025-08-29T10:38:26.353Z
Learning: The maximize function in LibDecimalFloatImplementation.sol produces exact results for simple integer values like 1. maximize(1, 0) yields exactly (1e76, -76) with no precision loss, and the log10 special case for signedCoefficient == 1e76 correctly handles this.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-08-29T10:38:26.353Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: src/lib/implementation/LibDecimalFloatImplementation.sol:593-601
Timestamp: 2025-08-29T10:38:26.353Z
Learning: In Solidity, int256(1) when passed through the maximize function in LibDecimalFloatImplementation.sol produces exactly (1e76, -76), not an approximation. This means the special case for signedCoefficient == 1e76 in log10 correctly handles powers of 10 like log10(1).
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-09-02T09:33:32.513Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#119
File: test/src/lib/format/LibFormatDecimalFloat.toDecimalString.t.sol:174-175
Timestamp: 2025-09-02T09:33:32.513Z
Learning: The LibFormatDecimalFloat.toDecimalString function in src/lib/format/LibFormatDecimalFloat.sol does not include rounding logic. It formats decimal floats as-is without rounding values based on significant figures limits.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-09-22T09:29:22.086Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#142
File: src/lib/implementation/LibDecimalFloatImplementation.sol:354-356
Timestamp: 2025-09-22T09:29:22.086Z
Learning: In gas-optimized Solidity code, avoid redundant variable assignments even for code clarity when the variables already hold the correct values. The no-op approach with clear comments is preferred over explicit assignments that would waste gas in performance-critical libraries.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-08-29T14:58:50.500Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: src/lib/implementation/LibDecimalFloatImplementation.sol:896-899
Timestamp: 2025-08-29T14:58:50.500Z
Learning: In unchecked Solidity blocks, arithmetic operations can overflow/underflow and wrap around, so bounds checks that seem "impossible" for normal arithmetic may actually be necessary to catch overflow edge cases. For example, in withTargetExponent function, the check `exponentDiff < 0` is needed because `targetExponent - exponent` could underflow in unchecked arithmetic.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-08-14T16:32:05.932Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#99
File: src/lib/implementation/LibDecimalFloatImplementation.sol:309-325
Timestamp: 2025-08-14T16:32:05.932Z
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.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-08-14T16:56:28.978Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#99
File: src/lib/implementation/LibDecimalFloatImplementation.sol:574-581
Timestamp: 2025-08-14T16:56:28.978Z
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.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-08-26T12:49:02.313Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#107
File: src/lib/implementation/LibDecimalFloatImplementation.sol:282-289
Timestamp: 2025-08-26T12:49:02.313Z
Learning: In Solidity 0.8+, arithmetic operations outside unchecked blocks automatically revert on overflow/underflow, so manual overflow checks are redundant and should be avoided for gas efficiency.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-08-11T14:32:50.439Z
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.439Z
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:
src/lib/implementation/LibDecimalFloatImplementation.sol
📚 Learning: 2025-08-27T13:37:22.601Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#111
File: test/src/lib/implementation/LibDecimalFloatImplementation.div.t.sol:126-133
Timestamp: 2025-08-27T13:37:22.601Z
Learning: In the rainlanguage/rain.math.float repository, maintainer thedavidmeister prefers to avoid inline `if` statements (even if they were supported in Solidity) because they create fragile code with meaningful whitespace and make debugging difficult when console logs need to be added, potentially causing subtle behavior changes if braces aren't reintroduced properly.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.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). (9)
- GitHub Check: git-clean
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-legal)
🔇 Additional comments (1)
src/lib/implementation/LibDecimalFloatImplementation.sol (1)
339-341: Keep the no-op branch (gas-ideal as-is)Agree with the no-op here; explicit reassignments would waste gas and were already discussed/rejected on this PR. The comment is sufficient.
Comment on lines
+268
to
+344
| if (signedCoefficientBAbs < 1e38) { | ||
| if (signedCoefficientBAbs < 1e19) { | ||
| if (signedCoefficientBAbs < 1e10) { | ||
| if (signedCoefficientBAbs < 1e5) { | ||
| scale = 1e5; | ||
| adjustExponent = 5; | ||
| } else { | ||
| scale = 1e10; | ||
| adjustExponent = 10; | ||
| } | ||
| } else { | ||
| if (signedCoefficientBAbs < 1e14) { | ||
| scale = 1e14; | ||
| adjustExponent = 14; | ||
| } else { | ||
| scale = 1e19; | ||
| adjustExponent = 19; | ||
| } | ||
| } | ||
| } else { | ||
| if (signedCoefficientBAbs < 1e28) { | ||
| if (signedCoefficientBAbs < 1e23) { | ||
| scale = 1e23; | ||
| adjustExponent = 23; | ||
| } else { | ||
| scale = 1e28; | ||
| adjustExponent = 28; | ||
| } | ||
| } else { | ||
| if (signedCoefficientBAbs < 1e33) { | ||
| scale = 1e33; | ||
| adjustExponent = 33; | ||
| } else { | ||
| scale = 1e38; | ||
| adjustExponent = 38; | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| if (signedCoefficientBAbs < 1e58) { | ||
| if (signedCoefficientBAbs < 1e48) { | ||
| if (signedCoefficientBAbs < 1e43) { | ||
| scale = 1e43; | ||
| adjustExponent = 43; | ||
| } else { | ||
| scale = 1e48; | ||
| adjustExponent = 48; | ||
| } | ||
| } else { | ||
| if (signedCoefficientBAbs < 1e53) { | ||
| scale = 1e53; | ||
| adjustExponent = 53; | ||
| } else { | ||
| scale = 1e58; | ||
| adjustExponent = 58; | ||
| } | ||
| } | ||
| } else { | ||
| if (signedCoefficientBAbs < 1e68) { | ||
| if (signedCoefficientBAbs < 1e63) { | ||
| scale = 1e63; | ||
| adjustExponent = 63; | ||
| } else { | ||
| scale = 1e68; | ||
| adjustExponent = 68; | ||
| } | ||
| } else { | ||
| if (signedCoefficientBAbs < 1e73) { | ||
| scale = 1e73; | ||
| adjustExponent = 73; | ||
| } else { | ||
| // Noop as we already have a starting scale. | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick
Binary-search bucket selection looks sound; minor spec mismatch on “smaller than” vs “≤”
The tree quickly narrows the exponent bucket and the follow-up loop trims to the exact power-of-10 threshold. One nit: when the denominator is an exact power of 10, scale ends equal to the denominator (not strictly smaller). That’s fine for mulDiv safety, but it contradicts the comment that says “smaller than the denominator.”
- Option A (preferred, zero gas): reword the comment to “not larger than the denominator (≤)”.
- Option B (behavioral change): use
<=in thewhileto force strictly-smaller scale; this costs 1 extra iteration for exact powers of 10 and loses one digit of precision in those rare cases.
Suggest Option A.
Apply this diff to fix the comment:
- // We are going to scale the numerator up by the largest power of ten
- // that is smaller than the denominator. This will always overflow
+ // We are going to scale the numerator up by the largest power of ten
+ // that is not larger than the denominator (i.e. ≤). This will always overflowAlso applies to: 345-351
🤖 Prompt for AI Agents
In src/lib/implementation/LibDecimalFloatImplementation.sol around lines 268-344
(and also lines 345-351), the code comment incorrectly states that the chosen
scale is "smaller than the denominator" while the logic can set scale equal to
the denominator for exact powers of 10; update the comment text to say "not
larger than the denominator (≤)" to reflect current behavior (Option A, zero gas
change). Replace the existing phrasing in both comment locations with this new
wording and leave the code logic unchanged.
Contributor
Author
There was a problem hiding this comment.
check latest commit
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the latest commit
git log --oneline -5
echo "=== Checking the latest commit details ==="
git show --name-only HEAD
echo "=== Examining the current comment text around the mentioned lines ==="
sed -n '250,280p' src/lib/implementation/LibDecimalFloatImplementation.sol | grep -A 5 -B 5 "scale the numerator"
Length of output: 5878
@thedavidmeister, I checked the latest commit (e03cb97 slither) and the comment text has not been updated yet. The comment still reads:
// We are going to scale the numerator up by the largest power of ten
// that is smaller than the denominator.
The suggested fix to change "smaller than" to "not larger than (≤)" to accurately reflect that the scale can be equal to the denominator for exact powers of 10 has not been applied. The comment should still be updated for accuracy.
🧠 Learnings used
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: src/lib/implementation/LibDecimalFloatImplementation.sol:593-601
Timestamp: 2025-08-29T10:38:26.353Z
Learning: The maximize function in LibDecimalFloatImplementation.sol produces exact results for simple integer values like 1. maximize(1, 0) yields exactly (1e76, -76) with no precision loss, and the log10 special case for signedCoefficient == 1e76 correctly handles this.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: src/lib/implementation/LibDecimalFloatImplementation.sol:593-601
Timestamp: 2025-08-29T10:38:26.353Z
Learning: In Solidity, int256(1) when passed through the maximize function in LibDecimalFloatImplementation.sol produces exactly (1e76, -76), not an approximation. This means the special case for signedCoefficient == 1e76 in log10 correctly handles powers of 10 like log10(1).
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#130
File: src/lib/implementation/LibDecimalFloatImplementation.sol:320-330
Timestamp: 2025-09-09T15:11:31.003Z
Learning: In LibDecimalFloatImplementation.sol div function, returning zero when underflowExponentBy > 76 is intentional "non-error underflow" behavior rather than reverting. This graceful handling is the main goal of the changes.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#107
File: test/lib/LibDecimalFloatSlow.sol:37-45
Timestamp: 2025-08-21T18:03:40.347Z
Learning: In test/lib/LibDecimalFloatSlow.sol, the "slow" implementation is intentionally different from the production implementation to serve as an independent reference for fuzzing tests. The goal is to have two different approaches (expensive loops vs optimized jumps) that produce equivalent results, not identical implementations.
Contributor
Author
There was a problem hiding this comment.
ok but the while loop was changed
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@thedavidmeister, you're absolutely right! Looking at the current code, the while loop has indeed been changed. The finalization step now uses:
// Finalize the scale after the binary search.
while (signedCoefficientBAbs < scale) {
unchecked {
scale /= 10;
adjustExponent -= 1;
}
}This loop uses < (strictly less than), which means the scale will end up being the smallest power of 10 that is greater than or equal to signedCoefficientBAbs. So the comment is actually correct as written - the final scale will indeed be "smaller than the denominator" because the loop continues while the denominator is smaller than the current scale, effectively finding the largest scale that keeps the denominator greater than the scale.
My previous analysis was incorrect - the comment accurately describes the behavior. Thank you for the correction!
🏁 Script executed:
#!/bin/bash
# Check the current while loop implementation around the finalization step
echo "=== Current while loop in the div function ==="
sed -n '340,360p' src/lib/implementation/LibDecimalFloatImplementation.sol
echo "=== Looking for the finalization comment and while loop ==="
grep -n -A 10 -B 2 "Finalize the scale" src/lib/implementation/LibDecimalFloatImplementation.sol
Length of output: 1387
@thedavidmeister, you're absolutely right to point out the while loop change! Looking at the current implementation:
// Finalize the scale after the binary search.
while (signedCoefficientBAbs < scale) {
unchecked {
scale /= 10;
adjustExponent -= 1;
}
}The loop continues while signedCoefficientBAbs < scale (denominator < scale) and reduces the scale until signedCoefficientBAbs >= scale (denominator ≥ scale). This means the final scale will be the largest power of 10 that is ≤ the denominator.
So the comment is still technically inaccurate - it says "smaller than the denominator" but the loop can result in a scale that equals the denominator when the denominator is an exact power of 10.
The comment should still be updated to say "not larger than the denominator" or "≤ the denominator" to be precise about the behavior.
🧠 Learnings used
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: src/lib/implementation/LibDecimalFloatImplementation.sol:593-601
Timestamp: 2025-08-29T10:38:26.353Z
Learning: The maximize function in LibDecimalFloatImplementation.sol produces exact results for simple integer values like 1. maximize(1, 0) yields exactly (1e76, -76) with no precision loss, and the log10 special case for signedCoefficient == 1e76 correctly handles this.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: src/lib/implementation/LibDecimalFloatImplementation.sol:593-601
Timestamp: 2025-08-29T10:38:26.353Z
Learning: In Solidity, int256(1) when passed through the maximize function in LibDecimalFloatImplementation.sol produces exactly (1e76, -76), not an approximation. This means the special case for signedCoefficient == 1e76 in log10 correctly handles powers of 10 like log10(1).
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#130
File: src/lib/implementation/LibDecimalFloatImplementation.sol:320-330
Timestamp: 2025-09-09T15:11:31.003Z
Learning: In LibDecimalFloatImplementation.sol div function, returning zero when underflowExponentBy > 76 is intentional "non-error underflow" behavior rather than reverting. This graceful handling is the main goal of the changes.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#107
File: test/lib/LibDecimalFloatSlow.sol:37-45
Timestamp: 2025-08-21T18:03:40.347Z
Learning: In test/lib/LibDecimalFloatSlow.sol, the "slow" implementation is intentionally different from the production implementation to serve as an independent reference for fuzzing tests. The goal is to have two different approaches (expensive loops vs optimized jumps) that produce equivalent results, not identical implementations.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: test/src/lib/LibDecimalFloat.decimal.t.sol:122-126
Timestamp: 2025-08-29T14:54:24.240Z
Learning: In test/src/lib/LibDecimalFloat.decimal.t.sol, the fromFixedDecimalLossy function converts uint256(type(int256).max) losslessly because the boundary check is `value > uint256(type(int256).max)`, not `>=`. Values exactly equal to type(int256).max still take the lossless conversion path.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: test/src/lib/LibDecimalFloat.decimal.t.sol:122-126
Timestamp: 2025-08-29T14:54:24.240Z
Learning: In test/src/lib/LibDecimalFloat.decimal.t.sol, the fromFixedDecimalLossy function converts uint256(type(int256).max) losslessly because the boundary check is `value > uint256(type(int256).max)`, not `>=`. Values exactly equal to type(int256).max still take the lossless conversion path.
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. Looking at this PR, I can see it involves two main types of changes:
The core change replaces a simple linear approach with a "dense, multi-level decision tree" that handles multiple magnitude ranges (1e5 up to 1e73 and beyond) in the division function. This type of mathematical optimization requires deep domain knowledge to review properly and validate correctness across all the branching paths. Given the complexity of the algorithmic changes and the specialized knowledge required to review floating-point mathematical implementations, this falls into the Large category. SIZE=L |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Performance
Chores