full precision div#105
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughAdds a public error Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LibDecimalFloatImpl
participant mulDiv
Caller->>LibDecimalFloatImpl: div(Float A, Float B)
LibDecimalFloatImpl->>LibDecimalFloatImpl: compute signs, abs vals, maximize coefficients (≈1e75–1e76), pick scale and adjustExponent
LibDecimalFloatImpl->>mulDiv: mulDiv(absCoeffA, scale, absCoeffB)
alt mulDiv overflows
mulDiv-->>LibDecimalFloatImpl: revert MulDivOverflow
else success
mulDiv-->>LibDecimalFloatImpl: quotient
LibDecimalFloatImpl-->>Caller: return Float(coefficient=quotient, exponent=expA - expB - adjustExponent, sign=XOR)
end
sequenceDiagram
participant Caller
participant LibDecimalFloatImpl
Caller->>LibDecimalFloatImpl: inv(Float B)
LibDecimalFloatImpl->>LibDecimalFloatImpl: call div(1e76, -76, signedCoefficient(B), exponent(B))
LibDecimalFloatImpl-->>Caller: return Float(result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. ✨ 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/Issue comments)Type 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: 9
🔭 Outside diff range comments (1)
test/src/lib/implementation/LibDecimalFloatImplementation.div.t.sol (1)
111-130: Replace magic -76 with a named constant to aid readability.Using a well-named constant avoids scattering -76 across tests and documents intent. For example, add and use a local constant:
- int256 j = -76 - di; + // Adjust for the scaling exponent used by division normalization. + int256 constant ADJUST = -76; + int256 j = ADJUST - di;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/error/ErrDecimalFloat.sol(1 hunks)src/lib/implementation/LibDecimalFloatImplementation.sol(6 hunks)test/lib/LibCommonResults.sol(1 hunks)test/src/lib/LibDecimalFloat.mixed.t.sol(1 hunks)test/src/lib/LibDecimalFloat.pow.t.sol(1 hunks)test/src/lib/LibDecimalFloat.pow10.t.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.div.t.sol(3 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.pow10.t.sol(2 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-17T10:02:01.394Z
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#64
File: src/concrete/DecimalFloat.sol:0-0
Timestamp: 2025-06-17T10:02:01.394Z
Learning: In the rain.math.float project, the Float type is designed with a specific bit layout: 224-bit signed coefficient and 32-bit signed exponent, represented as 32 bytes total. The LibDecimalFloat.unpack function returns values that are guaranteed to fit within int224 and int32 ranges due to this internal representation, making explicit range checks before casting unnecessary.
Applied to files:
test/src/lib/LibDecimalFloat.pow10.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/lib/LibDecimalFloat.pow10.t.soltest/src/lib/LibDecimalFloat.mixed.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). (7)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (10)
test/lib/LibCommonResults.sol (1)
4-6: ✅ int256 bounds verified for constants• ONES: 76 digits
• THREES_PACKED: 67 digits
• THREES: 76 digits
(All are below the 77-digit safe threshold for int256.)Recommendations
- Add concise NatSpec comments above each constant to clarify the difference between THREES and THREES_PACKED and when to use each.
- (Optional) Improve readability/maintenance by expressing these large literals via formulas (e.g.,
10**76 / 9) or, once supported in your toolchain, using underscores in numeric literals.Location: test/lib/LibCommonResults.sol (lines 4–6)
[optional_refactors_recommended]test/src/lib/implementation/LibDecimalFloatImplementation.pow10.t.sol (1)
35-35: Renaming to testExactLookupsPow10 improves discoverabilityGood rename; the test name now clearly ties to pow10. No functional concerns.
test/src/lib/implementation/LibDecimalFloatImplementation.div.t.sol (2)
43-49: Good normalization to exponent -76 for 1/3 and -1/3.The expectations line up with the new 1e76 scaling and sign handling. Tests are precise and deterministic.
59-75: Expectations for scaled divisions look consistent.
- 1e18/3 → THREES at exponent -58 checks out given maximize() and adjustExponent=76.
- 10/(1e38,-37) → 1 → 1e76,-76 is correct.
- (1e38,-37)/2 → 5 → 5e75,-75 is consistent.
- (5e37,-37)/(2e37,-37) → 2.5 → 2.5e76,-76 is consistent.
All cases match the new normalization strategy.
test/src/lib/LibDecimalFloat.mixed.t.sol (2)
5-5: Switch to THREES_PACKED is appropriate for mixed/API-level tests.Aligns expectations with packed representation used by the public API. Good move.
13-20: Renamed test and updated expectations look correct.
- testDiv1Over3Mixed naming reads clearer.
- Asserting THREES_PACKED and exponent -67 matches the mixed packing/unpacking semantics under the new scaling.
src/lib/implementation/LibDecimalFloatImplementation.sol (4)
4-4: Importing MulDivOverflow is appropriate and clarifies the revert surface.Explicit revert type for mulDiv overflow paths improves diagnosability.
236-321: mulDiv helper is implemented correctly and follows the well-known pattern.Good use of the 512-bit product and inverse modulo 2^256 with appropriate overflow checks and a dedicated revert type.
Note: The AI summary mentions that mul also uses mulDiv, but mul() above still uses coefficient multiply + normalize recursion. If aligning mul with the new 512-bit path is desired for symmetry and headroom, that can be a follow-up refactor.
199-233: Fallback branch is reachable for non-zero denominatorsThe
maximize()routine only guarantees |B| ≥ 1e75—not ≥ 1e76—for all non-zero inputs. Coefficients in the range [1e75, 1e76) (e.g. 9×10⁷⁵) will:
- skip the “< 1e75” scaling block (since B/1e75 ≠ 0),
- overflow if you try to multiply by 10 (B > floor(MAX_INT256/10)),
- remain unchanged with |B| < 1e76,
- and therefore hit the
scale = 1e75fallback.Action items:
• Retain (or document) the 1e75 fallback—it's needed for non-zero B in [1e75, 1e76).
• Add regression tests for coefficients just above 5.789e75 and below 1e76 to cover this path.Likely an incorrect or invalid review comment.
491-494: Manual inversion in log10 negative branch is intentionally tied to the log‐table scaleThe
div(1e37, -37, …)in the negative branch oflog10is deliberate: by using 1e37/–37 you guarantee the inverted coefficient stays within [1e37…1e38) so that, after dividing by the table scale (1e34), your lookup index (x1Coefficient – 1000) falls in [0…9000). Switching toinv()(1e76/–76) would push that coefficient out of range and break the table lookup.Please add an inline comment above the negative branch to document that this constant choice is tied to the log tables and must not be replaced with a generic
inv()call:// This is a negative log. i.e. log(x) where 0 < x < 1. // log(x) = -log(1/x) - else { - (signedCoefficient, exponent) = div(1e37, -37, signedCoefficient, exponent); + else { + // Invert using 1e37/–37 so the result stays in [1e37…1e38) for the log10 tables (scale=1e34). + (signedCoefficient, exponent) = div(1e37, -37, signedCoefficient, exponent); (signedCoefficient, exponent) = log10(tablesDataContract, signedCoefficient, exponent); return minus(signedCoefficient, exponent); }Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…language/rain.math.float into 2025-08-15-minimize-denominator-div
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/lib/implementation/LibDecimalFloatImplementation.sol (1)
202-223: Duplicate abs-conversion blocks; factor into a helper for clarity and reuse.This exact logic appears twice; extract to a single internal helper for maintainability and fewer footguns around int256.min.
Apply this diff to the duplicated blocks:
- uint256 signedCoefficientAAbs; - if (signedCoefficientA < 0) { - if (signedCoefficientA == type(int256).min) { - signedCoefficientAAbs = uint256(type(int256).max) + 1; - } else { - signedCoefficientAAbs = uint256(-signedCoefficientA); - } - } else { - signedCoefficientAAbs = uint256(signedCoefficientA); - } - uint256 signedCoefficientBAbs; - if (signedCoefficientB < 0) { - if (signedCoefficientB == type(int256).min) { - signedCoefficientBAbs = uint256(type(int256).max) + 1; - } else { - signedCoefficientBAbs = uint256(-signedCoefficientB); - } - } else { - signedCoefficientBAbs = uint256(signedCoefficientB); - } + uint256 signedCoefficientAAbs = _absToUint256(signedCoefficientA); + uint256 signedCoefficientBAbs = _absToUint256(signedCoefficientB);And add this helper inside the same library (outside the selected range):
function _absToUint256(int256 x) internal pure returns (uint256) { unchecked { if (x >= 0) return uint256(x); return x == type(int256).min ? uint256(type(int256).max) + 1 : uint256(-x); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/implementation/LibDecimalFloatImplementation.sol(4 hunks)
⏰ 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-static)
- 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-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: git-clean
🔇 Additional comments (3)
src/lib/implementation/LibDecimalFloatImplementation.sol (3)
4-4: Importing MulDivOverflow is appropriate and scoped correctly.The new error is wired for mulDiv overflow/invalid-denominator signaling. No issues here.
246-331: mulDiv implementation otherwise matches the well-known 512-bit pattern.Beyond the denominator-0 fast-path issue above, the rest of the implementation (remainder subtraction, power-of-two factoring, modular inverse via Newton-Raphson) looks correct and gas-competitive.
501-504: inv now reuses div at the 1e76 scale — good simplification.Centralizing reciprocal behavior through div is cleaner and keeps invariant logic in one place.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/lib/implementation/LibDecimalFloatImplementation.sol (4)
64-68: MINIMIZED_ZERO_ are currently unused; either remove or wire into zero fast-paths.*
These constants aren’t referenced in div/maximize/normalize. Prefer wiring them into div’s 0/B fast-path (see next comment) to canonicalize zero and avoid drift, or remove them to keep the surface clean.
197-201: Add a fast-path for zero dividend to preserve canonical zero and avoid unnecessary work.
As-is, 0/B flows through maximize(scale) and returns 0 with a shifted exponent, deviating from the spec text (“adjust remains 0 when dividend is 0”), and does unnecessary mulDiv. Short-circuit right after maximizing A.Apply this diff:
// Move both coefficients into the e75/e76 range, so that the result // of division will not cause a mulDiv overflow. (signedCoefficientA, exponentA) = maximize(signedCoefficientA, exponentA); + // Fast-path: 0 / B = 0 (canonical minimized zero). + if (signedCoefficientA == 0) { + return (MINIMIZED_ZERO_SIGNED_COEFFICIENT, MINIMIZED_ZERO_EXPONENT); + } (signedCoefficientB, exponentB) = maximize(signedCoefficientB, exponentB);
202-223: DRY up the absolute-value conversion for A/B into a tiny helper.
Duplicate blocks for int256→uint256 absolute conversion (with int256.min handling). Factor to a helper for clarity and smaller bytecode.Apply this diff:
- // mulDiv only works with unsigned integers, so get the absolute - // values of the coefficients. - uint256 signedCoefficientAAbs; - if (signedCoefficientA < 0) { - if (signedCoefficientA == type(int256).min) { - signedCoefficientAAbs = uint256(type(int256).max) + 1; - } else { - signedCoefficientAAbs = uint256(-signedCoefficientA); - } - } else { - signedCoefficientAAbs = uint256(signedCoefficientA); - } - uint256 signedCoefficientBAbs; - if (signedCoefficientB < 0) { - if (signedCoefficientB == type(int256).min) { - signedCoefficientBAbs = uint256(type(int256).max) + 1; - } else { - signedCoefficientBAbs = uint256(-signedCoefficientB); - } - } else { - signedCoefficientBAbs = uint256(signedCoefficientB); - } + // mulDiv only works with unsigned integers, so get the absolute values of the coefficients. + uint256 signedCoefficientAAbs = _absToUint256(signedCoefficientA); + uint256 signedCoefficientBAbs = _absToUint256(signedCoefficientB);Add this helper inside the same library (outside the selected range):
function _absToUint256(int256 x) internal pure returns (uint256) { unchecked { if (x >= 0) return uint256(x); // Handle int256.min explicitly. return x == type(int256).min ? uint256(type(int256).max) + 1 : uint256(-x); } }
260-266: Fast-path can divide by zero; add explicit denominator==0 guard before returning.
When prod1==0, current code performs prod0/denominator without checking denominator. If x*y fits in 256 bits and denominator==0, this takes a bare div-by-zero instead of reverting with MulDivOverflow as intended.Apply this diff:
- // Handle non-overflow cases, 256 by 256 division. - if (prod1 == 0) { - unchecked { - return prod0 / denominator; - } - } + // Denominator must be non-zero to avoid div-by-zero in the fast path. + if (denominator == 0) { + revert MulDivOverflow(x, y, denominator); + } + // Handle non-overflow cases, 256 by 256 division. + if (prod1 == 0) { + unchecked { + return prod0 / denominator; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/implementation/LibDecimalFloatImplementation.sol(4 hunks)
⏰ 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 (ubuntu-latest, rainix-rs-test)
🔇 Additional comments (3)
src/lib/implementation/LibDecimalFloatImplementation.sol (3)
4-4: Importing MulDivOverflow looks correct for signaling mulDiv errors.
Using a dedicated error here is appropriate and keeps div overflow/denominator issues well-scoped.
246-330: mulDiv core implementation otherwise looks standard and solid.
With the explicit denominator==0 check (see prior comment), the implementation aligns with established patterns and error signaling.
501-504: LGTM: inv delegates to div with 1e76 scaling.
This keeps behavior consistent with the updated div scaling. Ensure tests cover extremal ranges and inv(0) reverting semantics.Would you like me to add tests for:
- div(0, eA, B, eB) returns canonical zero (0e0),
- inv(0) reverts with MulDivOverflow,
- large/small operand pairs exercising both 1e76 and 1e75 branches?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/lib/implementation/LibDecimalFloatImplementation.sol (4)
192-196: Add 0-dividend fast-path after maximizing A to avoid unnecessary work and exponent drift.
Returning immediately for 0 / B avoids scaling B and the mulDiv call. This was previously requested and still applicable.Apply this diff:
(signedCoefficientA, exponentA) = maximize(signedCoefficientA, exponentA); + // Fast-path: 0 / B = 0 (return the canonical zero form). + if (signedCoefficientA == 0) { + return (MAXIMIZED_ZERO_SIGNED_COEFFICIENT, MAXIMIZED_ZERO_EXPONENT); + } (signedCoefficientB, exponentB) = maximize(signedCoefficientB, exponentB);
197-218: Duplicate abs-conversion blocks; factor into a small helper for clarity and gas.
The two blocks are identical and handle int256.min edge case. Factor them out to reduce bytecode and improve readability.Apply this diff:
- uint256 signedCoefficientAAbs; - if (signedCoefficientA < 0) { - if (signedCoefficientA == type(int256).min) { - signedCoefficientAAbs = uint256(type(int256).max) + 1; - } else { - signedCoefficientAAbs = uint256(-signedCoefficientA); - } - } else { - signedCoefficientAAbs = uint256(signedCoefficientA); - } - uint256 signedCoefficientBAbs; - if (signedCoefficientB < 0) { - if (signedCoefficientB == type(int256).min) { - signedCoefficientBAbs = uint256(type(int256).max) + 1; - } else { - signedCoefficientBAbs = uint256(-signedCoefficientB); - } - } else { - signedCoefficientBAbs = uint256(signedCoefficientB); - } + uint256 signedCoefficientAAbs = _absToUint256(signedCoefficientA); + uint256 signedCoefficientBAbs = _absToUint256(signedCoefficientB);Add this helper inside the same library (outside the selected range):
function _absToUint256(int256 x) internal pure returns (uint256) { unchecked { if (x >= 0) return uint256(x); return x == type(int256).min ? uint256(type(int256).max) + 1 : uint256(-x); } }
220-236: Use unsigned magnitude for scale decision; fix comment; drop signed division.
Basing the choice on signed division is unnecessary and makes the intent murkier. Compare the unsigned magnitude directly; also the comment should say “smaller than or equal to”.Apply this diff:
- // We are going to scale the numerator up by the largest power of ten - // that is smaller than the denominator. This will always overflow - // internally to the mulDiv during the initial multiplication, in - // 512 bits, but will subsequently always be reduced back down to - // fit in 256 bits by the division of a denominator that is larger - // than the scale up. - int256 scale = 1e76; - int256 adjustExponent = 76; - if (signedCoefficientB / scale == 0) { - scale = 1e75; - adjustExponent = 75; - } - uint256 signedCoefficientAbs = mulDiv(signedCoefficientAAbs, uint256(scale), signedCoefficientBAbs); + // Scale the numerator by the largest power of ten that is smaller than + // or equal to the denominator's magnitude. The initial 256-bit product + // may overflow, but the 512-bit mulDiv path handles it and the subsequent + // division reduces the result back to 256 bits. + uint256 scale = 1e76; + int256 adjustExponent = 76; + if (signedCoefficientBAbs < scale) { + scale = 1e75; + adjustExponent = 75; + } + uint256 signedCoefficientAbs = mulDiv(signedCoefficientAAbs, scale, signedCoefficientBAbs);
255-260: mulDiv fast-path can divide by zero; add explicit denominator check before returning.
If x*y fits in 256 bits (prod1 == 0) and denominator == 0, this branch reverts with a bare div-by-zero instead of the intended MulDivOverflow. Guard it explicitly.Apply this diff:
- // Handle non-overflow cases, 256 by 256 division. - if (prod1 == 0) { - unchecked { - return prod0 / denominator; - } - } + // Denominator must be non-zero to avoid div-by-zero in the fast path. + if (denominator == 0) { + revert MulDivOverflow(x, y, denominator); + } + // Handle non-overflow cases, 256 by 256 division. + if (prod1 == 0) { + unchecked { + return prod0 / denominator; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/implementation/LibDecimalFloatImplementation.sol(3 hunks)
⏰ 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-static)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-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)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (2)
src/lib/implementation/LibDecimalFloatImplementation.sol (2)
4-4: Importing MulDivOverflow looks correct and is used below.
The new error is properly referenced in mulDiv; no issues here.
241-243: mulDiv attribution and shape LGTM.
The implementation matches the well-known 512-bit division pattern; attribution and license note are in place.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/lib/implementation/LibDecimalFloatImplementation.sol (4)
199-218: DRY the duplicate int256→uint256 absolute-value conversions.The two blocks computing abs with int256.min handling are duplicated and hurt readability. Factor into a small helper.
- uint256 signedCoefficientAAbs; - if (signedCoefficientA < 0) { - if (signedCoefficientA == type(int256).min) { - signedCoefficientAAbs = uint256(type(int256).max) + 1; - } else { - signedCoefficientAAbs = uint256(-signedCoefficientA); - } - } else { - signedCoefficientAAbs = uint256(signedCoefficientA); - } - uint256 signedCoefficientBAbs; - if (signedCoefficientB < 0) { - if (signedCoefficientB == type(int256).min) { - signedCoefficientBAbs = uint256(type(int256).max) + 1; - } else { - signedCoefficientBAbs = uint256(-signedCoefficientB); - } - } else { - signedCoefficientBAbs = uint256(signedCoefficientB); - } + uint256 signedCoefficientAAbs = _absToUint256(signedCoefficientA); + uint256 signedCoefficientBAbs = _absToUint256(signedCoefficientB);Add this helper inside the same library (outside the selected range):
function _absToUint256(int256 x) internal pure returns (uint256) { unchecked { if (x >= 0) return uint256(x); // Handle int256.min explicitly. return x == type(int256).min ? uint256(type(int256).max) + 1 : uint256(-x); } }
255-266: Critical: mulDiv fast path can divide by zero; add explicit denominator check before returning.When prod1 == 0, the function returns prod0 / denominator without checking denominator. If x*y fits in 256 bits and denominator == 0, this triggers a raw division-by-zero revert instead of your intended MulDivOverflow error, and also breaks inv(0) semantics.
Apply this diff:
- // Handle non-overflow cases, 256 by 256 division. - if (prod1 == 0) { - unchecked { - return prod0 / denominator; - } - } + // Denominator must be non-zero to avoid a raw div-by-zero in the fast path. + if (denominator == 0) { + revert MulDivOverflow(x, y, denominator); + } + // Micro-optimization: if either multiplicand is zero, the result is zero. + if (x == 0 || y == 0) { + return 0; + } + // Handle non-overflow cases, 256 by 256 division. + if (prod1 == 0) { + unchecked { + return prod0 / denominator; + } + }
192-199: Add zero-dividend fast path in div to preserve spec (adjust=0) and avoid unnecessary work.Currently, 0/B flows through abs/scale/mulDiv and returns a zero coefficient with a shifted exponent (exponentA - exponentB - adjustExponent), which contradicts the referenced algorithm (“adjust remains 0 when dividend is 0”) and does extra work.
Apply this diff right after maximizing A:
(signedCoefficientA, exponentA) = maximize(signedCoefficientA, exponentA); + // Fast-path: 0 / B = 0. Preserve canonical zero and avoid unnecessary work. + if (signedCoefficientA == 0) { + return (NORMALIZED_ZERO_SIGNED_COEFFICIENT, NORMALIZED_ZERO_EXPONENT); + } (signedCoefficientB, exponentB) = maximize(signedCoefficientB, exponentB);
220-226: Clarify the scale-selection comment; match behavior and avoid “always overflow” claim.Wording nits:
- It’s “smaller than or equal to” the denominator’s magnitude (since scale can equal |B| when |B|≈1e75).
- The initial 512-bit multiplication may overflow, not “will always”.
- // We are going to scale the numerator up by the largest power of ten - // that is smaller than the denominator. This will always overflow - // internally to the mulDiv during the initial multiplication, in + // We are going to scale the numerator up by the largest power of ten + // that is smaller than or equal to the denominator's magnitude. This may overflow + // internally in mulDiv during the initial multiplication, in
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.gas-snapshot(2 hunks)src/lib/implementation/LibDecimalFloatImplementation.sol(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T14:30:48.562Z
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.562Z
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:
src/lib/implementation/LibDecimalFloatImplementation.sol
🔇 Additional comments (2)
src/lib/implementation/LibDecimalFloatImplementation.sol (1)
4-4: Importing MulDivOverflow is appropriate and consistent with the new mulDiv helper.Good preparatory change for surfacing a consistent revert reason from mulDiv.
.gas-snapshot (1)
1-305: Gas snapshot updates look consistent with the div/inv refactor; watch the outliers.Notable increases around division-heavy tests (e.g., testUnnormalizedThreesDiv0 μ: ~24.5M) align with the new mulDiv path and scale/maximize work. If any of these deltas are unexpected, consider:
- Adding a zero-dividend short-circuit in div (suggested above) to shave gas in common 0/B cases.
- Early-return micro-opts in mulDiv for x==0||y==0 (also suggested) to reduce overhead in degenerate paths.
Otherwise, snapshot changes LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/lib/implementation/LibDecimalFloatImplementation.sol (4)
192-196: Add zero-dividend fast-path before maximizing B to avoid unnecessary work.Currently B is maximized even when A becomes 0 after maximizing, which is wasted gas. Short-circuit right after maximizing A, then only maximize B if needed.
Apply this reordering:
- (signedCoefficientA, exponentA) = maximize(signedCoefficientA, exponentA); - (signedCoefficientB, exponentB) = maximize(signedCoefficientB, exponentB); + (signedCoefficientA, exponentA) = maximize(signedCoefficientA, exponentA); + // Fast-path: 0 / B = 0 (return the canonical zero form) before doing any more work. + if (signedCoefficientA == 0) { + return (MAXIMIZED_ZERO_SIGNED_COEFFICIENT, MAXIMIZED_ZERO_EXPONENT); + } + (signedCoefficientB, exponentB) = maximize(signedCoefficientB, exponentB);
197-211: Duplicate abs conversion logic; consider a tiny helper (optional).Two near-identical blocks convert int256 to absolute uint256 with int256.min handling. A small internal helper improves readability with negligible overhead.
Minimal in-place refactor:
- uint256 signedCoefficientAAbs; - if (signedCoefficientA > 0) { - signedCoefficientAAbs = uint256(signedCoefficientA); - } else if (signedCoefficientA < 0) { - if (signedCoefficientA == type(int256).min) { - signedCoefficientAAbs = uint256(type(int256).max) + 1; - } else { - signedCoefficientAAbs = uint256(-signedCoefficientA); - } - } else { - return (MAXIMIZED_ZERO_SIGNED_COEFFICIENT, MAXIMIZED_ZERO_EXPONENT); - } + uint256 signedCoefficientAAbs; + if (signedCoefficientA > 0) { + signedCoefficientAAbs = uint256(signedCoefficientA); + } else if (signedCoefficientA < 0) { + signedCoefficientAAbs = signedCoefficientA == type(int256).min + ? uint256(type(int256).max) + 1 + : uint256(-signedCoefficientA); + } else { + return (MAXIMIZED_ZERO_SIGNED_COEFFICIENT, MAXIMIZED_ZERO_EXPONENT); + }Add this helper outside the selected range (optional):
// Place anywhere inside the library. function _absToUint256(int256 x) internal pure returns (uint256) { unchecked { if (x >= 0) return uint256(x); return x == type(int256).min ? uint256(type(int256).max) + 1 : uint256(-x); } }Note: A prior learning indicates a preference to keep such logic inline for gas; treat this as optional.
234-239: Guard exponent arithmetic from wrapping.exponent = exponentA - exponentB - adjustExponent can wrap unchecked at extreme inputs. Add a cheap round-trip validation and revert with ExponentOverflow on failure, mirroring mul()’s robustness.
int256 exponent = exponentA - exponentB - adjustExponent; + // Validate two's-complement round-trip to catch wraparound. + unchecked { + if (exponent + exponentB + adjustExponent != exponentA) { + revert ExponentOverflow(signedCoefficient, exponent); + } + }
243-330: mulDiv fast-path can divide by zero; add explicit denominator check before early return.If prod1 == 0 and denominator == 0, current code hits a raw div-by-zero instead of reverting with MulDivOverflow, breaking error consistency.
Apply this fix:
// Handle non-overflow cases, 256 by 256 division. - if (prod1 == 0) { + // Ensure consistent revert semantics for denominator == 0 as well. + if (denominator == 0) { + revert MulDivOverflow(x, y, denominator); + } + if (prod1 == 0) { unchecked { return prod0 / denominator; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.gas-snapshot(2 hunks)src/lib/implementation/LibDecimalFloatImplementation.sol(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-11T14:30:48.562Z
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.562Z
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:
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
⏰ 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: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: git-clean
🔇 Additional comments (5)
src/lib/implementation/LibDecimalFloatImplementation.sol (3)
4-4: Importing MulDivOverflow is correct and consistent with usage.Matches the new error signaling in mulDiv/div paths. No concerns.
211-220: LGTM: Symmetric absolute-magnitude handling for divisor.The int256.min case is correctly handled; unsigned magnitude is ready for the scaling logic.
501-504: inv delegation to div(1e76,-76,…) is sound; please ensure revert-type coverage for inv(0).The new path will bubble up MulDivOverflow on zero input via mulDiv; add/confirm a test asserting the exact revert type for inv(0).
I can draft a focused Forge test that asserts inv(0) reverts with MulDivOverflow and exercises both scale branches around the 1e75/1e76 threshold. Want me to open a PR with those tests?
.gas-snapshot (2)
147-155: Division gas shifts look consistent with full-precision mulDiv scaling.The updated μ/~ across div tests align with the new scaling and 512/256 division path; snapshot changes are reasonable.
156-156: Runs reduced to 100 for testUnnormalizedThreesDiv0; confirm this is intentional.Significant gas per-run is understandable, but please confirm the reduced fuzz count is deliberate and that both 1e75 and 1e76 scale branches remain covered elsewhere.
If helpful, I can add narrowly scoped tests that deterministically hit each branch and assert revert types for zero-divisor cases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/lib/implementation/LibDecimalFloatImplementation.sol (2)
201-224: Duplicate abs-conversion logic; consider a tiny helper (acknowledging prior preference to inline)Both A and B convert int256 to uint256 with the int256.min edge case. A small internal helper would DRY this up. Not blocking given your gas/inline preferences noted previously.
If you ever decide to factor it, here’s a drop-in helper (outside the selected range):
function _absToUint256(int256 x) internal pure returns (uint256) { unchecked { if (x >= 0) return uint256(x); return x == type(int256).min ? uint256(type(int256).max) + 1 : uint256(-x); } }Then:
- signedCoefficientAAbs = _absToUint256(signedCoefficientA);
- signedCoefficientBAbs = _absToUint256(signedCoefficientB);
226-235: Comment nit: clarify “smaller than” → “smaller than or equal to”The code keeps scale at 1e76 when |B| >= 1e76 and uses 1e75 otherwise, so the scale is ≤ |B|. Update the comment to reflect this precisely.
Apply this wording tweak:
- // 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 scale the numerator by the largest power of ten that is smaller than + // or equal to the denominator's magnitude. This will always overflow // internally to the mulDiv during the initial multiplication, in // 512 bits, but will subsequently always be reduced back down to // fit in 256 bits by the division of a denominator that is larger // than the scale up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/lib/implementation/LibDecimalFloatImplementation.sol(3 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.inv.t.sol(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-11T14:30:48.562Z
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.562Z
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:
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
⏰ 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 (8)
src/lib/implementation/LibDecimalFloatImplementation.sol (5)
4-4: Importing MulDivOverflow here is appropriateThis aligns the implementation with the new error type used by mulDiv/div and keeps selectors centralized.
191-200: Solid setup to keep division within mulDiv-safe rangesInitializing scale/adjustExponent and maximizing both operands up-front reads well and matches the new approach.
236-240: Sign computation via XOR is correct and gas-friendlyGood use of XOR on signs and a single cast of the magnitude.
242-245: Good: exponent arithmetic is checked (outside unchecked)Keeping exponentA - exponentB - adjustExponent in a checked context prevents silent wraparound.
506-509: inv simplified via div(1e76, -76, ...) — clearer and ensures consistent scalingThis cleanly routes through the new div path; inv(0) semantics are now testable and consistent with MulDivOverflow.
Consider adding/confirming tests around the 1e75/1e76 threshold to exercise both scale branches in div/inv (|B| just below 1e76 and exactly 1e76), including negative coefficient cases, to cement edge behaviors.
test/src/lib/implementation/LibDecimalFloatImplementation.inv.t.sol (3)
9-11: Importing MulDivOverflow alongside EXPONENT bounds keeps the test self-containedThis makes the revert-assert explicit and stable against selector drift.
14-17: Wrapper is a minimal and effective harness for testing the internal invUsing an external method to surface the revert is appropriate here.
43-46: inv(0) revert assertion matches the intended mulDiv pathAsserting MulDivOverflow(1e76, 1e75, 0) aligns with the div scaling logic when the divisor is zero.
To round out coverage, consider adding:
- A direct div(..., 0) test to confirm the same revert pattern via div, and
- Boundary cases around |B| = 1e76 and |B| = 1e76 - 1 (and sign variants) to exercise both scale branches.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/float/src/lib.rs(1 hunks)
⏰ 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-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: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
|
@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:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=L |
2 participants
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation