2025 09 03 div zero#125
Merged
thedavidmeister merged 2 commits intomainfrom Sep 3, 2025
Merged
Conversation
Contributor
WalkthroughTightens zero-division short-circuit in LibDecimalFloatImplementation.div, causing 0/0 to enter full division path. Adds tests and an external wrapper to validate division-by-zero and MulDivOverflow behavior. Updates .gas-snapshot metrics across many tests; no public API changes to production contracts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as LibDecimalFloatImplementationDivTest
participant L as LibDecimalFloatImplementation.div
Note over T: testDivZero(signedCoefficient, exponent)
T->>T: maximize(signedCoefficient, exponent)
T->>L: divExternal(a= signedCoefficient,<br/>ea= exponent,<br/>b=0, eb=0)
alt a == 0 && b != 0
Note over L: Short-circuit return zero (unchanged path)
L-->>T: (0, adjustedExponent)
else a == 0 && b == 0 (new path)
Note over L: Enters main division flow
opt May compute mulDiv(..., denom=0)
L-->>T: revert stdError.divisionError
end
opt Alternative scaling path
L-->>T: revert MulDivOverflow(...)
end
else a != 0
Note over L: Normal division path
L-->>T: (quotient, exponent)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/implementation/LibDecimalFloatImplementation.sol (2)
228-271: Unify divide-by-zero behavior with an explicit, deterministic revertRight now, divisor==0 surfaces as either a builtin divisionError (0/0) or MulDivOverflow (x≠0/0) via mulDiv, which is observable and brittle. Consider making this explicit and consistent at the top of div:
- If signedCoefficientB == 0:
- If signedCoefficientA == 0: revert DivisionUndefined();
- Else: revert DivisionByZero(signedCoefficientA, exponentA);
- Else if signedCoefficientA == 0: return maximized zero.
This matches the decimal spec’s “Division undefined/by zero” cases and avoids relying on mulDiv side effects.
If you adopt this, tests expecting the current two different errors will need to be updated.
Proposed patch within this function:
- if (signedCoefficientA == 0 && signedCoefficientB != 0) { + if (signedCoefficientB == 0) { + // Deterministic div-by-zero semantics. + if (signedCoefficientA == 0) { + revert DivisionUndefined(); + } + revert DivisionByZero(signedCoefficientA, exponentA); + } else if (signedCoefficientA == 0) { signedCoefficient = MAXIMIZED_ZERO_SIGNED_COEFFICIENT; exponent = MAXIMIZED_ZERO_EXPONENT; } else {Additional (outside this hunk): define the two errors in ErrDecimalFloat.sol and import them here alongside the existing errors.
245-254: Comment nit: reflect actual condition (“≤” instead of “<”)The code keeps scale at 1e76 when signedCoefficientBAbs == 1e76, so the scale can be equal to the denominator. Tweak the comment to avoid confusion.
- // 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 smaller than or equal to the denominator. This will always overflow
📜 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 (3)
.gas-snapshot(2 hunks)src/lib/implementation/LibDecimalFloatImplementation.sol(1 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.div.t.sol(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 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.
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.
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.
📚 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.soltest/src/lib/implementation/LibDecimalFloatImplementation.div.t.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.soltest/src/lib/implementation/LibDecimalFloatImplementation.div.t.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.soltest/src/lib/implementation/LibDecimalFloatImplementation.div.t.sol
📚 Learning: 2025-08-18T13:52:43.369Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#105
File: src/lib/implementation/LibDecimalFloatImplementation.sol:248-335
Timestamp: 2025-08-18T13:52:43.369Z
Learning: The codebase has a policy of not modifying external code, such as mulDiv implementations adopted from standard libraries like OpenZeppelin, PRB Math, and Solady.
Applied to files:
src/lib/implementation/LibDecimalFloatImplementation.soltest/src/lib/implementation/LibDecimalFloatImplementation.div.t.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.soltest/src/lib/implementation/LibDecimalFloatImplementation.div.t.sol.gas-snapshot
📚 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:
test/src/lib/implementation/LibDecimalFloatImplementation.div.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). (9)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-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, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (5)
src/lib/implementation/LibDecimalFloatImplementation.sol (1)
259-263: LGTM: subtraction order reduces transient overflow riskThe branch on exponentA sign to order subtractions is a solid guard against intermediate overflow in unchecked math.
test/src/lib/implementation/LibDecimalFloatImplementation.div.t.sol (3)
15-22: Good wrapper to exercise external-call revert dataUsing an external pass-through simplifies expectRevert assertions for both builtin and custom errors.
37-54: Clear coverage for 0/0 vs x≠0/0 pathsNice assertion split: divisionError for 0/0 and MulDivOverflow for x≠0/0 aligns with current behavior. If you later make div-by-zero explicit (single error), update this test accordingly.
165-168: Bounds look safe for fuzzing extreme exponentsConstraining to EXPONENT_[MIN|MAX]/2 avoids pathological maximize paths while still stressing scaling logic.
.gas-snapshot (1)
1-329: Ack: snapshot drift consistent with new div pathsGas metrics updated as expected; no actionable concerns.
|
@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. 🧩 Analysis chainLooking at PR #125, I need to assess the size classification based on the changes made. SIZE=M 🏁 Script executed: Length of output: 85 🏁 Script executed: Length of output: 4251 SIZE=M 🧠 Learnings used |
7 tasks
This was referenced Sep 22, 2025
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
Bug Fixes
Tests
Chores