multivariadic sub#408
Conversation
WalkthroughRefactors LibOpSub to perform decimal float subtraction by unpacking operands into (signedCoefficient, exponent), using LibDecimalFloatImplementation.sub for arithmetic, and repacking the result with LibDecimalFloat.packLossy. Applies this pattern in both run() and referenceFn(), adds using LibDecimalFloat for Float, and handles lossless flags and slither suppressions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant OpSub as LibOpSub.run/referenceFn
participant Float as LibDecimalFloat (Float)
participant Impl as LibDecimalFloatImplementation
Caller->>OpSub: invoke with inputs (Float a, Float b, [...])
OpSub->>Float: a.unpack()
Float-->>OpSub: (aSignedCoeff, aExp)
OpSub->>Float: b.unpack()
Float-->>OpSub: (bSignedCoeff, bExp)
OpSub->>Impl: sub(aSignedCoeff,aExp,bSignedCoeff,bExp)
Impl-->>OpSub: (accSignedCoeff, accExp)
loop for each additional operand
OpSub->>Float: xi.unpack()
Float-->>OpSub: (xiSignedCoeff, xiExp)
OpSub->>Impl: sub(accSignedCoeff,accExp,xiSignedCoeff,xiExp)
Impl-->>OpSub: (accSignedCoeff, accExp)
end
OpSub->>Float: packLossy(accSignedCoeff, accExp)
Float-->>OpSub: (resultFloat, lossless)
OpSub-->>Caller: resultFloat
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
src/generated/Rainterpreter.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (1)
src/lib/op/math/LibOpSub.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). (10)
- GitHub Check: git-clean
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
🔇 Additional comments (1)
src/lib/op/math/LibOpSub.sol (1)
33-36: Cannot verify Float.unpack ↔ LibDecimalFloatImplementation.sub compatibility — library sources missingImports reference rain.math.float/lib/LibDecimalFloat.sol and rain.math.float/lib/implementation/LibDecimalFloatImplementation.sol (e.g., src/lib/op/math/LibOpSub.sol), but the referenced library sources are not present in the working tree; cannot confirm that Float.unpack()'s (signedCoefficient:int256, exponent:int256) ordering/sign/scale semantics match LibDecimalFloatImplementation.sub(...). Add the missing library sources or explicitly confirm unpack() returns (signedCoefficient, exponent) with the same semantics expected by the implementation.
| import {InterpreterState} from "../../state/LibInterpreterState.sol"; | ||
| import {Float, LibDecimalFloat} from "rain.math.float/lib/LibDecimalFloat.sol"; | ||
| import {StackItem} from "rain.interpreter.interface/interface/unstable/IInterpreterV4.sol"; | ||
| import {LibDecimalFloatImplementation} from "rain.math.float/lib/implementation/LibDecimalFloatImplementation.sol"; |
There was a problem hiding this comment.
Refactor introduces unnecessary complexity for decimal float subtraction
The refactoring from direct LibDecimalFloat.sub(a, b) calls to unpacking/repacking with LibDecimalFloatImplementation.sub() adds significant complexity without clear benefits. This pattern:
- Increases gas consumption due to multiple unpack/repack operations
- Reduces code readability with verbose coefficient/exponent handling
- Potentially loses precision through the
packLossyoperation
Unless there's a specific requirement for coefficient/exponent manipulation (e.g., custom rounding, overflow handling), the simpler LibDecimalFloat.sub() approach should be preferred.
Consider reverting to the simpler approach:
-import {LibDecimalFloatImplementation} from "rain.math.float/lib/implementation/LibDecimalFloatImplementation.sol";
library LibOpSub {
- using LibDecimalFloat for Float;
-
function run(InterpreterState memory, OperandV2 operand, Pointer stackTop) internal pure returns (Pointer) {
Float a;
Float b;
assembly ("memory-safe") {
a := mload(stackTop)
b := mload(add(stackTop, 0x20))
stackTop := add(stackTop, 0x40)
}
- (int256 signedCoefficient, int256 exponent) = a.unpack();
- (int256 signedCoefficientB, int256 exponentB) = b.unpack();
- (signedCoefficient, exponent) =
- LibDecimalFloatImplementation.sub(signedCoefficient, exponent, signedCoefficientB, exponentB);
+ a = LibDecimalFloat.sub(a, b);
{
uint256 inputs = uint256(OperandV2.unwrap(operand) >> 0x10) & 0x0F;
uint256 i = 2;
while (i < inputs) {
assembly ("memory-safe") {
b := mload(stackTop)
stackTop := add(stackTop, 0x20)
}
- (signedCoefficientB, exponentB) = b.unpack();
- (signedCoefficient, exponent) =
- LibDecimalFloatImplementation.sub(signedCoefficient, exponent, signedCoefficientB, exponentB);
+ a = LibDecimalFloat.sub(a, b);
unchecked {
i++;
}
}
}
- //slither-disable-next-line unused-return
- (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent);Also applies to: 15-15, 33-36, 46-48, 54-55, 74-85
There was a problem hiding this comment.
unpacking and lossy conversion happens inside LibDecimalFloat.sub but not LibDecimalFloatImplementation.sub
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for LibDecimalFloat.sub implementation to understand the internal unpacking/packing
ast-grep --pattern 'function sub($_, $_) {
$$$
}'
# Also search for LibDecimalFloatImplementation.sub to compare
rg -n -A 10 -B 2 "function sub" --type=sol
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for LibDecimalFloat.sub implementation
rg -n -A 10 -B 2 "function sub" --type-add 'sol:*.sol' --type=sol
# If that doesn't work, search more broadly
rg -n -A 10 -B 2 "function sub" -g "*.sol"
Length of output: 43045
🏁 Script executed:
#!/bin/bash
# Search specifically for mathematical subtraction functions in the codebase
rg -n -A 15 -B 5 "function sub" -g "*.sol" | grep -A 15 -B 5 -E "(Float|decimal|math)"
# Search for LibDecimalFloat and LibDecimalFloatImplementation in external dependencies
find . -name "*.sol" -exec grep -l "LibDecimalFloat" {} \;
# Let's look for the actual implementation differences by searching for specific functions
rg -n -A 10 -B 5 "LibDecimalFloat\.sub\|LibDecimalFloatImplementation\.sub" -g "*.sol"
Length of output: 3666
🏁 Script executed:
#!/bin/bash
# Let's check other math operations to see if they use similar patterns
cat src/lib/op/math/LibOpAdd.sol | head -50
# Check if other operations use LibDecimalFloatImplementation
rg -n "LibDecimalFloatImplementation" -g "*.sol"
# Look at the imports and usage in LibOpSub to understand the change better
cat src/lib/op/math/LibOpSub.sol | grep -A 5 -B 5 "Implementation\|unpack\|packLossy"
Length of output: 8529
@thedavidmeister Thank you for the clarification! You're absolutely right, and I understand now why this refactoring was necessary.
Looking at the other math operations (LibOpAdd, LibOpMul, LibOpDiv), I can see they all follow the same pattern: they use LibDecimalFloatImplementation functions which operate directly on unpacked coefficients and exponents, avoiding the internal unpacking/packing that happens inside LibDecimalFloat.sub().
This approach is more efficient for multi-operand operations because:
-
Avoids redundant packing/unpacking:
LibDecimalFloat.sub(a, b)would internally unpack both floats, perform the operation, then pack the result back to a Float. In a loop, this creates unnecessary intermediate packing. -
Better precision control: By using
LibDecimalFloatImplementation.sub()directly on unpacked values and only callingpackLossy()once at the end, you maintain maximum precision throughout the computation chain. -
Consistency: All the math operations (add, sub, mul, div) now follow the same pattern for multivariadic operations.
Your implementation correctly keeps the computation in the unpacked coefficient/exponent form throughout the loop and only packs the final result once, which is indeed more efficient than the previous approach that would have caused multiple pack/unpack cycles.
✏️ Learnings added
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#408
File: src/lib/op/math/LibOpSub.sol:10-10
Timestamp: 2025-09-19T17:18:14.810Z
Learning: LibDecimalFloat.sub() internally performs unpacking and packing with potential precision loss, while LibDecimalFloatImplementation.sub() works directly on unpacked coefficients and exponents without intermediate packing. For multivariadic operations, using LibDecimalFloatImplementation functions is more efficient as it avoids redundant pack/unpack cycles and maintains better precision control.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| //slither-disable-next-line unused-return | ||
| (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); |
There was a problem hiding this comment.
🧹 Nitpick
Potential precision loss from packLossy without warning
The packLossy function returns a lossless flag indicating whether precision was lost during packing, but this information is discarded. This could lead to silent precision loss in calculations.
Consider either:
- Using a lossless packing method if available
- Documenting that precision loss is expected/acceptable
- Reverting if precision loss occurs (if exactness is required)
- //slither-disable-next-line unused-return
- (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent);
+ bool lossless;
+ (a, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent);
+ // Consider: require(lossless, "Precision loss in subtraction");📝 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.
| //slither-disable-next-line unused-return | |
| (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | |
| bool lossless; | |
| (a, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | |
| // Consider: require(lossless, "Precision loss in subtraction"); |
🤖 Prompt for AI Agents
In src/lib/op/math/LibOpSub.sol around lines 54-55, the call to
LibDecimalFloat.packLossy ignores the returned lossless flag which can hide
precision loss; update the call to capture the second return value (bool
lossless) and then either (a) call a lossless packing alternative if one exists,
(b) require(lossless, "precision loss") to revert when exactness is required, or
(c) explicitly document/emit an event when lossless is false if silent loss is
acceptable—implement the chosen handling and add a short code comment explaining
the decision.
| bool lossless; | ||
| (acc, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | ||
| (lossless); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary variable assignment and parentheses
The lossless variable is captured but immediately discarded with (lossless);. This adds no value and creates confusion.
- bool lossless;
- (acc, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent);
- (lossless);
+ (acc,) = LibDecimalFloat.packLossy(signedCoefficient, exponent);📝 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.
| bool lossless; | |
| (acc, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | |
| (lossless); | |
| (acc,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); |
🤖 Prompt for AI Agents
In src/lib/op/math/LibOpSub.sol around lines 83 to 85, the function assigns the
boolean variable `lossless` from LibDecimalFloat.packLossy and then immediately
discards it with `(lossless);`. Remove the unused `lossless` variable and any
leftover discard expression, calling LibDecimalFloat.packLossy only into the
needed `acc` variable (or otherwise handle the returned boolean where it's
meaningful), so there is no redundant assignment or stray parentheses.
|
@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:
|
1 participant
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit