sig figs by amount #152
sig figs by amount #152
Conversation
WalkthroughReplaces sig-figs formatting with explicit scientific-mode and range-based formatting: adds default scientific bounds constants, new DecimalFloat.format overloads (bool and range), updates LibFormatDecimalFloat.toDecimalString to accept a boolean scientific flag, updates bindings/tests, and regenerates gas snapshot. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant DecimalFloat
participant LibFormat
rect rgb(250,250,255)
note over Caller,DecimalFloat: Default format()
Caller->>DecimalFloat: format(a)
DecimalFloat->>DecimalFloat: scientific = a.lt(FORMAT_DEFAULT_SCIENTIFIC_MIN) || a.gt(FORMAT_DEFAULT_SCIENTIFIC_MAX)
DecimalFloat->>LibFormat: toDecimalString(a, scientific)
LibFormat-->>DecimalFloat: formattedString
DecimalFloat-->>Caller: formattedString
end
rect rgb(245,255,245)
note over Caller,DecimalFloat: Range-based format
Caller->>DecimalFloat: format(a, scientificMin, scientificMax)
DecimalFloat->>DecimalFloat: require(scientificMin < scientificMax)
DecimalFloat->>LibFormat: toDecimalString(a, a.lt(scientificMin) || a.gt(scientificMax))
LibFormat-->>DecimalFloat: formattedString
DecimalFloat-->>Caller: formattedString
end
rect rgb(255,245,245)
note over Caller,DecimalFloat: Explicit mode
Caller->>DecimalFloat: format(a, scientific: bool)
DecimalFloat->>LibFormat: toDecimalString(a, scientific)
LibFormat-->>DecimalFloat: formattedString
DecimalFloat-->>Caller: formattedString
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/concrete/DecimalFloat.sol(1 hunks)src/lib/format/LibFormatDecimalFloat.sol(1 hunks)test/src/concrete/DecimalFloat.format.t.sol(1 hunks)test/src/lib/format/LibFormatDecimalFloat.toDecimalString.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/format/LibFormatDecimalFloat.soltest/src/lib/format/LibFormatDecimalFloat.toDecimalString.t.soltest/src/concrete/DecimalFloat.format.t.solsrc/concrete/DecimalFloat.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/format/LibFormatDecimalFloat.toDecimalString.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/format/LibFormatDecimalFloat.toDecimalString.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). (2)
- GitHub Check: git-clean
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
🔇 Additional comments (5)
src/lib/format/LibFormatDecimalFloat.sol (1)
73-88: ...test/src/lib/format/LibFormatDecimalFloat.toDecimalString.t.sol (3)
17-23: Helper update LGTMSignature and call site match the new boolean API; assertion is clear.
26-33: Round‑trip helper LGTMParses then canonicalizes with the same scientific flag; consistent with “no rounding” formatting. Based on learnings.
94-104: Fuzz round‑trip coverage LGTMGood bounds, positive/negative symmetry, and canonicalization for both paths.
Also applies to: 107-123
test/src/concrete/DecimalFloat.format.t.sol (1)
13-15: Mirror logic LGTMLocal wrapper faithfully mirrors the production boolean for scientific mode.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/concrete/DecimalFloat.sol (1)
70-78: Address the existing review feedback.A previous review comment already identified improvements for this function:
- Extract the
scientificboolean into a local variable for clarity- Update NatSpec to specify that bounds are inclusive
Please refer to the existing review comment for the suggested changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.gas-snapshot(2 hunks)src/concrete/DecimalFloat.sol(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/concrete/DecimalFloat.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:
.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:
.gas-snapshot
📚 Learning: 2025-09-28T13:28:35.814Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#144
File: test/src/lib/LibDecimalFloat.pow.t.sol:40-61
Timestamp: 2025-09-28T13:28:35.814Z
Learning: In test/src/lib/LibDecimalFloat.pow.t.sol, the diffLimit tolerance (currently 8.6%) represents a conservative worst-case bound to accommodate precision limitations of table lookup interpolation for fractional exponents in the pow function. The exponentiation-by-squaring approach used for integer parts achieves much tighter precision, but the tolerance must account for the less precise table-based calculations used for fractional exponents.
Applied to files:
.gas-snapshot
🔇 Additional comments (3)
.gas-snapshot (1)
1-345: Gas snapshot appropriately updated for new formatting API.The updated metrics reflect the formatting changes introduced in this PR (range-based and boolean scientific notation formatting). The addition of
testFormatConstantsat line 13 aligns with the newFORMAT_DEFAULT_SCIENTIFIC_MINandFORMAT_DEFAULT_SCIENTIFIC_MAXconstants.src/concrete/DecimalFloat.sol (2)
80-87: LGTM!The boolean interface provides a clean, direct way to control scientific notation formatting when custom logic is needed beyond the range-based approach.
89-95: LGTM!The default format function correctly delegates to the range-based format using the newly defined constants, providing a sensible default behavior for most use cases.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/concrete/DecimalFloat.sol (1)
71-79: Extract boolean flag and clarify inclusive bounds (duplicate of past review).The past review's suggestions remain unaddressed:
Readability: Extract
bool scientific = a.lt(scientificMin) || a.gt(scientificMax);before callingtoDecimalStringto improve code clarity.NatSpec clarity: Update parameter documentation to explicitly state that bounds are inclusive:
@param scientificMinThe smallest value formatted in non-scientific notation (inclusive).@param scientificMaxThe largest value formatted in non-scientific notation (inclusive).The logic is correct—values equal to the bounds will use non-scientific formatting.
Apply this diff:
function format(Float a, Float scientificMin, Float scientificMax) public pure returns (string memory) { require(scientificMin.lt(scientificMax), "scientificMin must be less than scientificMax"); - return LibFormatDecimalFloat.toDecimalString(a, a.lt(scientificMin) || a.gt(scientificMax)); + bool scientific = a.lt(scientificMin) || a.gt(scientificMax); + return LibFormatDecimalFloat.toDecimalString(a, scientific); }And update the NatSpec:
- /// @param scientificMin The smallest number that won't be formatted in - /// scientific notation. - /// @param scientificMax The largest number that won't be formatted in - /// scientific notation. + /// @param scientificMin The smallest number that won't be formatted in scientific notation (inclusive). + /// @param scientificMax The largest number that won't be formatted in scientific notation (inclusive).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/concrete/DecimalFloat.sol(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/concrete/DecimalFloat.sol
🔇 Additional comments (1)
src/concrete/DecimalFloat.sol (1)
81-88: LGTM: Clean boolean interface.The boolean overload provides a straightforward raw interface for scientific formatting, appropriately documented and correctly delegating to the underlying library function.
| /// The default minimum value for scientific formatting. 1e-4 | ||
| // slither-disable-next-line too-many-digits | ||
| Float public constant FORMAT_DEFAULT_SCIENTIFIC_MIN = | ||
| Float.wrap(0xfffffffc00000000000000000000000000000000000000000000000000000001); | ||
|
|
||
| /// The default maximum value for scientific formatting. 1e9 | ||
| // slither-disable-next-line too-many-digits | ||
| Float public constant FORMAT_DEFAULT_SCIENTIFIC_MAX = | ||
| Float.wrap(0x0000000900000000000000000000000000000000000000000000000000000001); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Constants have basic NatSpec—consider the optional enhancement from past review.
The constants now include NatSpec comments stating their purpose and numeric values, which is an improvement. However, the past review suggested more detailed documentation explaining the threshold behavior (e.g., "Values below this threshold will use scientific notation"). This enhancement would improve discoverability but is not critical.
🤖 Prompt for AI Agents
In src/concrete/DecimalFloat.sol around lines 12 to 20, the NatSpec for
FORMAT_DEFAULT_SCIENTIFIC_MIN and FORMAT_DEFAULT_SCIENTIFIC_MAX only states
numeric values; update each comment to clearly describe the threshold behavior
(e.g., that values strictly below FORMAT_DEFAULT_SCIENTIFIC_MIN and strictly
above FORMAT_DEFAULT_SCIENTIFIC_MAX will be rendered in scientific notation,
include the intended numeric thresholds like 1e-4 and 1e9 and any
inclusive/exclusive semantics), keep the short summary and numeric example, and
ensure wording is concise and consistent with surrounding documentation.
| /// Exposes `format(Float, Float, Float)` for offchain use. | ||
| /// Provides default scientific formatting. | ||
| /// @param a The float to format. | ||
| /// @return The string representation of the float. | ||
| function format(Float a, uint256 sigFigsLimit) external pure returns (string memory) { | ||
| return LibFormatDecimalFloat.toDecimalString(a, sigFigsLimit); | ||
| function format(Float a) external pure returns (string memory) { | ||
| return format(a, FORMAT_DEFAULT_SCIENTIFIC_MIN, FORMAT_DEFAULT_SCIENTIFIC_MAX); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider clarifying the default bounds in NatSpec.
The implementation correctly delegates to the range-based format with default constants. To improve discoverability, consider mentioning the actual default range in the NatSpec documentation (e.g., "Formats using default scientific bounds: 1e-4 to 1e9").
Optional diff:
- /// Exposes `format(Float, Float, Float)` for offchain use.
- /// Provides default scientific formatting.
+ /// Formats the float using default scientific notation bounds (1e-4 to 1e9).
+ /// Values outside this range use scientific notation.
/// @param a The float to format.
/// @return The string representation of the float.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Exposes `format(Float, Float, Float)` for offchain use. | |
| /// Provides default scientific formatting. | |
| /// @param a The float to format. | |
| /// @return The string representation of the float. | |
| function format(Float a, uint256 sigFigsLimit) external pure returns (string memory) { | |
| return LibFormatDecimalFloat.toDecimalString(a, sigFigsLimit); | |
| function format(Float a) external pure returns (string memory) { | |
| return format(a, FORMAT_DEFAULT_SCIENTIFIC_MIN, FORMAT_DEFAULT_SCIENTIFIC_MAX); | |
| } | |
| /// Formats the float using default scientific notation bounds (1e-4 to 1e9). | |
| /// Values outside this range use scientific notation. | |
| /// @param a The float to format. | |
| /// @return The string representation of the float. | |
| function format(Float a) external pure returns (string memory) { | |
| return format(a, FORMAT_DEFAULT_SCIENTIFIC_MIN, FORMAT_DEFAULT_SCIENTIFIC_MAX); | |
| } |
🤖 Prompt for AI Agents
In src/concrete/DecimalFloat.sol around lines 90 to 96, the NatSpec for the
format(Float) wrapper doesn't state the actual default scientific bounds; update
the function comment to explicitly mention the default range (e.g., "Formats
using default scientific bounds: 1e-4 to 1e9") so callers can discover the
defaults without reading constants, keeping the implementation unchanged.
|
@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 |
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Breaking Changes
Tests
Chores