tests for target exponent #141
Merged
Conversation
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
Contributor
WalkthroughTighten withTargetExponent’s overflow guard to treat exponentDiff <= 0 as an overflow/zero-return case for the target-greater branch. Add unit tests covering withTargetExponent edge cases and update gas-snapshot metrics. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Lib as LibDecimalFloatImplementation
Note right of Lib: withTargetExponent(signedCoefficient, exponent, targetExponent)
Caller->>Lib: call withTargetExponent(...)
Lib->>Lib: exponentDiff = targetExponent - exponent
alt exponentDiff == 0
Lib-->>Caller: return signedCoefficient
else targetExponent > exponent
alt exponentDiff <= 0 or exponentDiff > 76
Note right of Lib #lightblue: tightened overflow guard
Lib-->>Caller: return 0
else exponentDiff in [1..76]
Lib->>Lib: divide coefficient by 10^exponentDiff
Lib-->>Caller: return scaled coefficient
end
else targetExponent < exponent
Lib->>Lib: multiply coefficient by 10^(exponent - targetExponent) (existing path)
Lib-->>Caller: return adjusted coefficient
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/implementation/LibDecimalFloatImplementation.sol (3)
548-556: Overflow risk when bumping exponent on add overflowIf
exponentAistype(int256).max,exponentA += 1wraps in the unchecked block. Guard and revert before incrementing.if (didOverflow) { signedCoefficientA /= 10; signedCoefficientB /= 10; + if (exponentA == type(int256).max) { + revert ExponentOverflow(signedCoefficientA, exponentA); + } - exponentA += 1; + exponentA += 1; signedCoefficientA += signedCoefficientB; } else {
972-979: withTargetExponent: widened guard to<= 0is correct for unchecked underflow; prefer shared constantChange is good—catches wrapped zero diffs. Use the shared constant to avoid magic number drift.
- if (exponentDiff > 76 || exponentDiff <= 0) { + if (exponentDiff > ADD_MAX_EXPONENT_DIFF || exponentDiff <= 0) { return (MAXIMIZED_ZERO_SIGNED_COEFFICIENT); }
980-989: withTargetExponent (grow path): keep numeric limit consistentMirror the constant here too for consistency with the shrink path and
compareRescale.- if (exponentDiff > 76 || exponentDiff < 0) { + if (exponentDiff > ADD_MAX_EXPONENT_DIFF || exponentDiff < 0) { revert WithTargetExponentOverflow(signedCoefficient, exponent, targetExponent); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/lib/implementation/LibDecimalFloatImplementation.sol(4 hunks)test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 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.
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.
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#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.
📚 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.withTargetExponent.t.solsrc/lib/implementation/LibDecimalFloatImplementation.sol
📚 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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.solsrc/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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.solsrc/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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.solsrc/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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.solsrc/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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.solsrc/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-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-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (macos-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (5)
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol (4)
113-117: Good: equality no-op case coveredCovers the
exponent == targetExponentfast path.
119-130: Good: >76 larger target returns zeroValidates the widened
<= 0/large-diff zeroing behavior for shrink path.
132-136: Good: extremal underflow case
(min, max)pair is a strong regression check for the unchecked diff computation.
138-150: Good: scale-down fuzz band [1, 76]Exercises precise division for all valid shrink diffs. Consider mirroring with a “grow within 76” test that asserts revert only when multiplication would overflow (you already have one for the revert path, but a positive case with explicit equality assertion would complete the matrix).
src/lib/implementation/LibDecimalFloatImplementation.sol (1)
622-641: Narrow unchecked scope in log10Function is entirely wrapped in unchecked, which suppresses overflow checks on control/return-path math; restrict unchecked to only arithmetic hot spots (table/scale ops) and keep boundary-sensitive sums and final arithmetic checked. Confirm intent to keep log10 largely unchecked; otherwise I will provide a scoped diff that closes the unchecked block before the final arithmetic and reopens it where needed.
Contributor
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 (1)
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
📚 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.withTargetExponent.t.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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol
📚 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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.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: 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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.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-static)
- GitHub Check: rainix (macos-latest, rainix-sol-legal)
- 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 (ubuntu-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: git-clean
🔇 Additional comments (6)
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol (6)
119-131: Underflow-to-zero path (diff ≥ 77) asserted — good.Validates the non-error underflow behavior when scaling down by ≥77 decimal places.
132-137: Extreme diff (min → max) returns zero — good guard coverage.Covers the wrap/overflow edge in
targetExponent - exponentand asserts graceful zero.
138-151: Scale down by 1..76 with exact division — good property test.Confirms precise division for safe diffs.
152-165: Scale up with diff ≥ 77 reverts — correct overflow expectation.Uses the external wrapper to capture revert; bounds are correct.
166-187: Scale up by 1..76 without overflow — correct result asserted.Good use of
vm.assumeto filter overflow cases.
188-206: Scale up overflow by 1..76 — revert path validated.Complements the non-overflow case; bounds and assumptions look right.
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol
Outdated
Show resolved
Hide resolved
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol
Show resolved
Hide resolved
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)
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.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: 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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol
📚 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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.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:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol
📚 Learning: 2025-09-10T19:20:35.927Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#135
File: test/src/lib/LibDecimalFloat.pow.t.sol:160-161
Timestamp: 2025-09-10T19:20:35.927Z
Learning: In test/src/lib/LibDecimalFloat.pow.t.sol, the comment "we do not try" in the context of "The round trip should not error so we do not try" refers to not using try/catch constructs around the powExternal call, contrasting with previous versions that used nested try/catch blocks.
Applied to files:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol
📚 Learning: 2025-08-29T15:26:25.984Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: test/src/lib/LibDecimalFloat.decimal.t.sol:56-56
Timestamp: 2025-08-29T15:26:25.984Z
Learning: In Forge tests, the bound() function is overloaded and requires explicit type casting for overload resolution. When using bound(value, min, max), if the max parameter involves type conversions, an outer cast on the entire bound() call may be necessary for the compiler to resolve which overload to use. Additionally, Solidity cannot change both signedness and bit width in a single cast, so complex casts like uint256(int256(type(int224).max)) require the staged approach rather than a direct cast.
Applied to files:
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.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-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (macos-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (7)
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol (7)
113-124: LGTM: scale-down diff ≥ 77 returns zero.Good property coverage for the non-error underflow path.
126-130: LGTM: max wraparound case covered.Covers the exponentDiff wrap (min→max) path returning zero.
132-144: LGTM: precise scale-down property (diff ∈ [1,76]).Checks truncation toward zero via integer division as expected.
146-158: LGTM: large scale-up (diff ≥ 77) revert path.Correctly uses external call to validate custom error selector.
182-200: LGTM: scale-up overflow property (diff ∈ [1,76]) with assume() guard.Covers the multiplicative overflow revert path cleanly.
118-119: Replace 76/77 “magic numbers” with named constants.Improves readability and reduces mistakes. This was requested earlier and still applies here.
Apply within this file (changed lines shown):
- targetExponent = bound(targetExponent, type(int256).min + 77, type(int256).max); - exponent = bound(exponent, type(int256).min, targetExponent - 77); + targetExponent = bound(targetExponent, type(int256).min + OVERFLOW_EXP_DIFF, type(int256).max); + exponent = bound(exponent, type(int256).min, targetExponent - OVERFLOW_EXP_DIFF);- targetExponentDiff = bound(targetExponentDiff, int256(1), int256(76)); + targetExponentDiff = bound(targetExponentDiff, int256(1), MAX_SAFE_EXP_DIFF);- exponent = bound(exponent, type(int256).min + 77, type(int256).max); - targetExponent = bound(targetExponent, type(int256).min, exponent - 77); + exponent = bound(exponent, type(int256).min + OVERFLOW_EXP_DIFF, type(int256).max); + targetExponent = bound(targetExponent, type(int256).min, exponent - OVERFLOW_EXP_DIFF);- targetExponentDiff = bound(targetExponentDiff, int256(1), int256(76)); + targetExponentDiff = bound(targetExponentDiff, int256(1), MAX_SAFE_EXP_DIFF);- targetExponentDiff = bound(targetExponentDiff, int256(1), int256(76)); + targetExponentDiff = bound(targetExponentDiff, int256(1), MAX_SAFE_EXP_DIFF);Add constants near the top of the contract:
int256 constant MAX_SAFE_EXP_DIFF = 76; int256 constant OVERFLOW_EXP_DIFF = MAX_SAFE_EXP_DIFF + 1;Also applies to: 136-139, 151-153, 165-168, 185-188
113-200: Verified: withTargetExponent semantics match tests.withTargetExponent returns MAXIMIZED_ZERO_SIGNED_COEFFICIENT (zero) when targetExponent - exponent > 76 or when the computed exponentDiff ≤ 0 (wrapped), and it reverts WithTargetExponentOverflow when exponent - targetExponent > 76 — no changes required.
test/src/lib/implementation/LibDecimalFloatImplementation.withTargetExponent.t.sol
Show resolved
Hide resolved
|
@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:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Performance