multivariadic mul - #407
Conversation
WalkthroughRefactors LibOpMul to unpack Float operands to (signedCoefficient, exponent), multiply using LibDecimalFloatImplementation.mul across inputs, then repack with LibDecimalFloat.packLossy. Adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant LibOpMul
participant FloatLib as LibDecimalFloat
participant Impl as LibDecimalFloatImplementation
Caller->>LibOpMul: run(inputs: Float[])
LibOpMul->>FloatLib: unpack(a)
FloatLib-->>LibOpMul: (coeffA, expA)
LibOpMul->>FloatLib: unpack(b)
FloatLib-->>LibOpMul: (coeffB, expB)
LibOpMul->>Impl: mul(coeffA, expA, coeffB, expB)
Impl-->>LibOpMul: (coeff, exp)
loop accumulate remaining operands
LibOpMul->>FloatLib: unpack(next)
FloatLib-->>LibOpMul: (coeffN, expN)
LibOpMul->>Impl: mul(coeff, exp, coeffN, expN)
Impl-->>LibOpMul: (coeff, exp)
end
LibOpMul->>FloatLib: packLossy(coeff, exp)
FloatLib-->>LibOpMul: (Float result, lossFlag)
LibOpMul-->>Caller: Float result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 1
📜 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/LibOpMul.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: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: rainix (macos-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-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (7)
src/lib/op/math/LibOpMul.sol (7)
15-15: LGTM! Good use of the using directive.Adding the using directive for
LibDecimalFloatonFloattype improves code readability by enabling method-style calls likea.unpack()instead ofLibDecimalFloat.unpack(a).
33-36: The refactoring to unpacked arithmetic looks correct.The change from direct multiplication to unpacking operands, multiplying coefficients/exponents via
LibDecimalFloatImplementation.mul, and then repacking is a valid approach that provides more control over the multiplication process.
46-48: Consistent implementation in the loop.The loop correctly unpacks each additional operand and accumulates the result using the same multiplication approach, maintaining consistency with the initial multiplication.
73-79: Reference implementation correctly mirrors the main logic.The reference implementation properly follows the same unpacked arithmetic pattern as the main
runfunction, ensuring consistency for testing purposes.
33-55: Precision-loss handling consistent across math opsLibOpAdd and LibOpDiv follow the same unpack → LibDecimalFloatImplementation.add/div → LibDecimalFloat.packLossy accumulation pattern as LibOpMul, so the refactor is consistent.
55-55: Consider handling the loss flag frompackLossy.The
packLossyfunction returns a tuple(Float, bool)where the boolean indicates whether the packing was lossless. Currently, the loss flag is being discarded. Consider whether this information should be used for debugging, logging, or error handling.
17-21: Approve — integrity supports variadic mulIntegrity enforces a minimum of 2 inputs; run and reference implementations iterate over N≥2 and the test suite covers integrity edge cases (0/1) and runtime for 2+ (including 3) inputs. Files: src/lib/op/math/LibOpMul.sol, test/src/lib/op/math/LibOpMul.t.sol, test/src/lib/op/math/uint256/LibOpUint256Mul.t.sol.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/op/math/LibOpMul.sol (1)
82-85: Unusedlosslessin reference; remove or assert.Same nit as before: captured then no‑op. Either discard the second return or use it for a check.
Apply this diff to discard cleanly:
- bool lossless; - (acc, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); - (lossless); + (acc,) = LibDecimalFloat.packLossy(signedCoefficient, exponent);Alternatively, if you want reference to fail on quantization during tests:
- bool lossless; - (acc, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); - (lossless); + bool lossless; + (acc, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); + assert(lossless);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/op/math/LibOpMul.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: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: git-clean
🔇 Additional comments (4)
src/lib/op/math/LibOpMul.sol (4)
15-16: Using directive is correct and necessary.Enables
.unpack()onFloat. No issues.
46-48: Loop multiply per-operand is sound.Unpack each operand and accumulate via implementation mul;
unchecked { i++; }is appropriate.
74-74: Reference accumulator init LGTM.Starting from
inputs[0]asaccis consistent with the main path.
76-80: Reference multiply mirrors main logic correctly.No discrepancies spotted between reference and main paths for the core mul.
| (int256 signedCoefficient, int256 exponent) = a.unpack(); | ||
| (int256 signedCoefficientB, int256 exponentB) = b.unpack(); | ||
| (signedCoefficient, exponent) = | ||
| LibDecimalFloatImplementation.mul(signedCoefficient, exponent, signedCoefficientB, exponentB); |
There was a problem hiding this comment.
🧹 Nitpick
Unpack → impl.mul path looks correct.
Clean handoff to LibDecimalFloatImplementation.mul. Consider optional zero short‑circuit if result coefficient becomes 0 (still consuming remaining inputs but skipping extra unpacks/muls) for minor gas wins on sparse products.
| //slither-disable-next-line unused-return | ||
| (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); |
There was a problem hiding this comment.
🧹 Nitpick
Remove unnecessary Slither suppression.
You already capture the first return; the “unused-return” suppression is likely redundant here.
Apply this diff:
- //slither-disable-next-line unused-return
(a,) = 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.
| //slither-disable-next-line unused-return | |
| (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | |
| (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); |
🤖 Prompt for AI Agents
In src/lib/op/math/LibOpMul.sol around lines 55 to 56, the Slither suppression
comment "//slither-disable-next-line unused-return" is unnecessary because the
code already captures the first return value with "(a,)" from
LibDecimalFloat.packLossy; remove the suppression line so the file simply
performs the packed assignment without the redundant Slither directive.
|
@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