variadic add lint - #406
Conversation
WalkthroughUnpacks decimal-float operands to signedCoefficient/exponent pairs, accumulates sums via LibDecimalFloatImplementation.add, then produces a lossy Float using LibDecimalFloat.packLossy; mirrors this in the reference function and updates one test expected value for a 3-input overflow case. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Interpreter
participant LibOpAdd
participant LibDecimalFloat as LibDecimalFloat (unpack/pack)
participant Impl as LibDecimalFloatImplementation (add)
Caller->>Interpreter: eval(op_add, inputs[])
Interpreter->>LibOpAdd: run(inputs)
Note over LibOpAdd: unpack first input -> running (signedCoeff, exp)
loop remaining inputs
LibOpAdd->>LibDecimalFloat: unpack(input) -> (coeffB, expB, signB)
LibDecimalFloat-->>LibOpAdd: (coeffB, expB, signB)
LibOpAdd->>Impl: add(runningCoeff/exp, coeffB/expB/signB)
Impl-->>LibOpAdd: updated runningCoeff, runningExp
end
LibOpAdd->>LibDecimalFloat: packLossy(runningCoeff, runningExp)
LibDecimalFloat-->>LibOpAdd: Float (lossy)
LibOpAdd-->>Interpreter: push Float
Interpreter-->>Caller: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
📜 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 (2)
src/lib/op/math/LibOpAdd.sol(4 hunks)test/src/lib/op/math/LibOpAdd.t.sol(1 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, rainix-sol-artifacts)
- 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-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
🔇 Additional comments (5)
src/lib/op/math/LibOpAdd.sol (5)
11-11: Good use of lower-level add for accumulation.Importing LibDecimalFloatImplementation to accumulate on coeff/exp reduces pack/unpack churn and should be cheaper.
32-36: Single unpack/add upfront is correct and gas-friendlier than Float-level adds.This preserves exactness during the fold and avoids intermediate packing until the end.
45-48: Loop accumulation looks correct; no off‑by‑one on inputs.Starts at i=2 after consuming a,b and processes remaining operands; stack pointer math matches the pop/pop/push pattern.
If operand inputs could ever be <2 at runtime (despite integrity guarantees), this would read past the stack. Please confirm VM invariants ensure min‑2 here.
54-55: Confirm packLossy semantics vs. “Errors on overflow.”Using packLossy aligns with delaying rounding until the end, but ensure true overflow conditions still revert (e.g., coeff > int224 or exp out of int32 range) and that lossy rounding is acceptable for this opcode.
If overflow must revert and lossy rounding must not silently pass, consider guarding the flag:
- (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); + bool lossless; + (a, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); + // If rounding is not allowed for `add`, enforce: + // require(lossless, "ADD_LOSSY");
73-79: Reference function mirrors runtime accumulation correctly.Unpack/fold over inputs using Implementation.add is consistent with run() and maintains exactness until final pack.
| bool lossless; | ||
| (acc, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | ||
| (lossless); | ||
| outputs = new StackItem[](1); |
There was a problem hiding this comment.
🧹 Nitpick
Explicitly handle or document lossless flag in reference.
You’re silencing the flag; that’s fine if rounding is allowed. If not, assert it so tests fail fast.
- bool lossless;
- (acc, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent);
- (lossless);
+ bool lossless;
+ (acc, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent);
+ // assert(lossless); // uncomment if add must be lossless barring overflow📝 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); | |
| outputs = new StackItem[](1); | |
| bool lossless; | |
| (acc, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | |
| // assert(lossless); // uncomment if add must be lossless barring overflow | |
| outputs = new StackItem[](1); |
🤖 Prompt for AI Agents
In src/lib/op/math/LibOpAdd.sol around lines 80 to 83, the returned lossless
flag from LibDecimalFloat.packLossy is currently ignored; either assert it to
fail fast when rounding would occur or explicitly document/handle that lossy
packing is acceptable. Update the code to check the lossless boolean and call
require(lossless, "LibOpAdd: lossy conversion") if rounding must be disallowed,
or add a clear comment and/or telemetry/logging explaining why lossy conversions
are permitted and retaining the current behavior.
| checkUnhappyOverflow( | ||
| "_: add(max-positive-value() max-positive-value() max-positive-value());", | ||
| 26959946667150639794667015087019630673637144422540572481103610249214000000000, | ||
| 40439920000725959692000522630529446010455716633810858721655415373821000000000, |
There was a problem hiding this comment.
🧹 Nitpick
Updated overflow expectation: please justify with a brief note and add a permutation check.
The new coeff literal reflects the changed accumulation path. Add a short comment explaining the derivation and consider adding the two other permutations of three max‑positive inputs to ensure order‑independence in the overflow path.
Example:
// Derived from Implementation.add folding three max-positive-value() before final packing.🤖 Prompt for AI Agents
In test/src/lib/op/math/LibOpAdd.t.sol around line 157, the updated large
coefficient literal lacks a justification comment and the test only covers one
input ordering; add a one-line comment above the literal explaining how the
value was derived (e.g., "Derived from Implementation.add folding three
max-positive-value() before final packing.") and extend the test by adding the
two other permutations of the same three max-positive inputs to assert the
overflow behavior is order-independent; ensure the added permutations use the
same expected literal and run in the same test block.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/op/math/LibOpAdd.sol (1)
81-84: Re: lossless flag in reference — handle or assert.
Same concern as previously noted: either assert lossless or clearly document that lossy rounding is acceptable in tests.Option if you choose to enforce:
- 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/LibOpAdd.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, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
🔇 Additional comments (5)
src/lib/op/math/LibOpAdd.sol (5)
11-11: LGTM: Correct import for Implementation.add.
Matches the new accumulation strategy.
45-47: LGTM: Gas-savvy inner-loop accumulation.
Approach is tight and avoids repeated (re)packing.
74-74: LGTM: Reference initializes accumulator via unpack.
Matches the run path.
76-79: LGTM: Reference loop mirrors implementation add semantics.
Keeps parity with run.
54-56: Don't silently drop lossless; either enforce or document rounding.packLossy returns (Float,bool); run() discards the lossless flag in src/lib/op/math/LibOpAdd.sol:55 — either require lossless here or explicitly allow/document lossy rounding. Similar pattern appears in src/lib/op/math/LibOpDiv.sol:56 and other ops; ensure repo-wide consistency.
Apply this diff to require lossless packing (and drop the Slither suppression):
- //slither-disable-next-line unused-return - (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); + bool lossless; + (a, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); + require(lossless, "LibOpAdd: lossy rounding");If lossy rounding is intentional, update the file header/docs and tests to explicitly allow lossy results.
| (int256 signedCoefficient, int256 exponent) = LibDecimalFloat.unpack(a); | ||
| (int256 signedCoefficientB, int256 exponentB) = LibDecimalFloat.unpack(b); | ||
| (signedCoefficient, exponent) = | ||
| LibDecimalFloatImplementation.add(signedCoefficient, exponent, signedCoefficientB, exponentB); |
There was a problem hiding this comment.
🧹 Nitpick
LGTM: Unpack-then-add reduces per-step pack/unpack. Minor runtime guard suggestion.
Consider mirroring integrity’s min-2 fallback in run to harden against malformed operands.
Apply in the run() block that computes inputs:
- uint256 inputs = uint256((OperandV2.unwrap(operand) >> 0x10) & bytes32(uint256(0x0F)));
+ uint256 inputs = uint256((OperandV2.unwrap(operand) >> 0x10) & bytes32(uint256(0x0F)));
+ if (inputs < 2) inputs = 2;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/op/math/LibOpAdd.sol around lines 32-35, after unpacking a and b and
before calling LibDecimalFloatImplementation.add, add the same runtime guard
used in integrity that enforces a min-2 fallback for operands: in the run()
block that computes inputs, detect if the inputs list is malformed or has fewer
than two operands and supply a default zero operand (coefficient 0, exponent 0)
or otherwise normalize missing/invalid operands to a zero DecimalFloat before
unpacking/adding, so the add call always receives two well-formed operands.
There was a problem hiding this comment.
the whole point of the integrity check is to remove the need for runtime gas to do the same
There was a problem hiding this comment.
@thedavidmeister You're absolutely right. The integrity check system is designed to handle validation at compile/deploy time to avoid runtime gas costs for the same checks. Adding runtime guards would be redundant and wasteful. I understand now that the separation between integrity validation and run execution is an important architectural principle.
✏️ Learnings added
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#406
File: src/lib/op/math/LibOpAdd.sol:32-35
Timestamp: 2025-09-18T19:08:29.941Z
Learning: In Rain interpreter opcodes, integrity checks handle operand validation at compile/deploy time to eliminate the need for runtime validation in the run function. Adding runtime guards that duplicate integrity check logic wastes gas and goes against the architectural separation of concerns.
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.
|
@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