bump int#405
Conversation
WalkthroughRefactors decimal division to unpack inputs, call LibDecimalFloatImplementation.div, and packLossy the result; updates LibOpDiv tests and overflow/divide-by-zero semantics and expected values; updates expected constants in LibOpAdd and LibOpMul tests; advances the lib/rain.interpreter.interface submodule pointer. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor EVM as "Opcode Runner"
participant LibOpDiv as "LibOpDiv"
participant Unpack as "LibDecimalFloat.unpack"
participant Impl as "LibDecimalFloatImplementation.div"
participant Pack as "LibDecimalFloat.packLossy"
participant Tests as "Test Harness"
EVM->>LibOpDiv: execute DIV opcode with inputs...
LibOpDiv->>Unpack: unpack(inputA) => (coeffA, expA)
LibOpDiv->>Unpack: unpack(inputB) => (coeffB, expB)
LibOpDiv->>Impl: div(coeffA, expA, coeffB, expB)
note right of Impl #e6f2ff: Impl.div invoked repeatedly for multi-input sequences
Impl-->>LibOpDiv: (coeffResult, expResult)
LibOpDiv->>Pack: packLossy(coeffResult, expResult)
Pack-->>EVM: return packed decimal result
LibOpDiv->>Tests: tests assert results / log diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/src/lib/op/math/LibOpDiv.t.sol (2)
78-83: Clarify positive-only exponent overflow check.You only expect-revert when
exponentAoverflows int32 on the positive side; negative-side overflow is allowed (implicit underflow-to-zero semantics). Add a brief comment so future readers don’t “fix” this.- if (int32(exponentA) != exponentA && exponentA > 0) { + // Intentionally only guard positive-side overflow. Very-negative exponents are allowed (underflow to zero). + if (int32(exponentA) != exponentA && exponentA > 0) {
86-90: Initialize the second input explicitly.Make the intent (division by zero) obvious rather than relying on default-initialized memory.
function testDebugOpDivRun() external { StackItem[] memory inputs = new StackItem[](2); inputs[0] = StackItem.wrap(bytes32(uint256(115792089237316195423570985008687907853269984665640564039458))); + inputs[1] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(0, 0))); testOpDivRun(inputs); }src/lib/op/math/LibOpDiv.sol (1)
4-5: Minor import tidy (optional).You import
OperandV2andStackItemfrom the same path separately. Consider combining into one line for cohesion.-import {OperandV2} from "rain.interpreter.interface/interface/unstable/IInterpreterV4.sol"; +import {OperandV2, StackItem} from "rain.interpreter.interface/interface/unstable/IInterpreterV4.sol"; @@ -import {StackItem} from "rain.interpreter.interface/interface/unstable/IInterpreterV4.sol"; +// (combined above)Also applies to: 10-10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
src/generated/Rainterpreter.pointers.solis excluded by!**/generated/**src/generated/RainterpreterParser.pointers.solis excluded by!**/generated/**src/generated/RainterpreterReferenceExtern.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (4)
src/lib/op/math/LibOpDiv.sol(6 hunks)test/src/lib/op/math/LibOpAdd.t.sol(1 hunks)test/src/lib/op/math/LibOpDiv.t.sol(5 hunks)test/src/lib/op/math/LibOpMul.t.sol(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-17T14:15:14.886Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#368
File: test/src/lib/op/math/uint256/LibOpUint256Mul.t.sol:56-69
Timestamp: 2025-07-17T14:15:14.886Z
Learning: In multiplication overflow detection tests like LibOpUint256MulTest, when performing sequential multiplication (a * b * c * d...), encountering a zero value means the final result will always be zero regardless of subsequent values. Since zero multiplied by any value (including MAX_UINT256) cannot overflow, it's safe and correct to break out of the overflow detection loop early when zero is encountered.
Applied to files:
test/src/lib/op/math/LibOpMul.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). (10)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (2)
test/src/lib/op/math/LibOpDiv.t.sol (1)
171-171: Confirm error argument widths for 1e76.
DivisionByZero(selector, 1e76, -76)assumes the error params accept values beyond int224 max; confirm they’re int256 in the signature.src/lib/op/math/LibOpDiv.sol (1)
36-41: Unpack/div/pack refactor looks good.Using
LibDecimalFloatImplementation.divwith explicit unpack/pack aligns the runtime with the reference path;packLossyis appropriate for div rounding.Please confirm
packLossy(notpackLossless) is the intended public behavior for division rounding here.Also applies to: 50-53, 59-59, 97-97
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 (1)
src/lib/op/math/LibOpDiv.sol (1)
84-93: Bug: sentinel overwritten; reference no longer bails as intended.After setting the sentinel on div‑by‑zero (Lines 85–87), you overwrite
aat Line 92 viapackLossy, nullifying the bail‑out. Tests may observe the reference returning a normal value instead of a sentinel, masking failures.- Float a = Float.wrap(StackItem.unwrap(inputs[0])); + Float a = Float.wrap(StackItem.unwrap(inputs[0])); + bool bailed; @@ - if (b.isZero()) { - a = Float.wrap(bytes32(keccak256(abi.encodePacked("overflow sentinel")))); - break; - } + if (b.isZero()) { + a = Float.wrap(bytes32(keccak256(abi.encodePacked("overflow sentinel")))); + bailed = true; + break; + } @@ - (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); + if (!bailed) { + (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); + }Also, the comment mentions “overflow or divide by zero” but only div‑by‑zero is handled. Either implement an overflow pre‑check (hard to do precisely) or update the comment to reflect current behavior.
📜 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/LibOpDiv.sol(6 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, rainix-sol-test)
- GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (6)
src/lib/op/math/LibOpDiv.sol (6)
9-9: Import path OK; confirm submodule pin.Looks good. Please confirm the rain.math.float submodule/commit you bumped contains this implementation path and is pinned to a specific commit to avoid drift.
34-35: Good: explicit unpack → implementation.div for first operation.This matches the new math API and keeps the stack discipline intact.
37-39: Semantics parity check with previous rounding/normalization.Assuming
LibDecimalFloatImplementation.divpreserves prior rounding/normalization guarantees once repacked. Please confirm (esp. edge exponents).
48-51: Looped div path looks correct.Unpack each
b, apply implementation.div, incrementiunchecked. No off‑by‑one.
57-57: Confirm lossy packing is intended.
packLossymay drop precision; if the previousLibDecimalFloat.diveffectively produced a normalized Float with the same rounding, fine—otherwise considerpackif available.
75-75: Reference: unpack start LGTM.Sets up the coefficient/exponent accumulator correctly.
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 (1)
test/src/lib/op/math/LibOpDiv.t.sol (1)
76-81: Prefer divide-by-zero expectation firstIf both conditions could hold, DivisionByZero will be the first real failure. Flip the checks for clarity and intent.
- if (int32(exponentA) != exponentA && exponentA > 0) { - vm.expectRevert(); - } else if (divideByZero > 0) { - vm.expectRevert(); - } + if (divideByZero > 0) { + vm.expectRevert(); + } else if (int32(exponentA) != exponentA && exponentA > 0) { + vm.expectRevert(); + }
♻️ Duplicate comments (4)
test/src/lib/op/math/LibOpDiv.t.sol (2)
138-142: Replace magic numbers with named constants (2-input overflow)Improves readability and maintainability. Same ask as before.
checkUnhappyOverflow( "_: div(max-positive-value() 1e-18);", - 13479973333575319897333507543509815336818572211270286240551805124607000000000, - 2147483656 + DIV2_UNHAPPY_CALC_INPUTS, + DIV2_UNHAPPY_CALC_OUTPUTS );Add near the top of this file (constants section):
uint256 constant DIV2_UNHAPPY_CALC_INPUTS = 13479973333575319897333507543509815336818572211270286240551805124607000000000; int256 constant DIV2_UNHAPPY_CALC_OUTPUTS = 2147483656; // type(int32).max + 9
180-184: Replace magic numbers with named constants (3-input overflow)Same rationale as the 2-input overflow case.
checkUnhappyOverflow( "_: div(max-positive-value() 1e-18 1e-18);", - 13479973333575319897333507543509815336818572211270286240551805124607000000000, - 2147483674 + DIV3_UNHAPPY_CALC_INPUTS, + DIV3_UNHAPPY_CALC_OUTPUTS );Add near the top:
uint256 constant DIV3_UNHAPPY_CALC_INPUTS = 13479973333575319897333507543509815336818572211270286240551805124607000000000; int256 constant DIV3_UNHAPPY_CALC_OUTPUTS = 2147483674; // type(int32).max + 27src/lib/op/math/LibOpDiv.sol (2)
13-13: Tighten NatSpec (wording and behavior)Be explicit and use the verb “divide”.
-/// @notice Opcode to div N decimal float values. Errors on overflow. +/// @notice Opcode to divide N decimal float values. Reverts on overflow or divide-by-zero.
25-25: Clarify the summary lineCapitalize and mention failure modes.
- /// decimal floating point division. + /// Decimal floating point division. Reverts on overflow or if any divisor is zero.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/lib/op/math/LibOpDiv.sol(6 hunks)test/src/lib/op/math/LibOpDiv.t.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, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: git-clean
🔇 Additional comments (2)
src/lib/op/math/LibOpDiv.sol (2)
9-9: LGTM: Import Implementation where the arithmetic livesCorrect dependency; keeps core decimal logic centralized.
34-38: Division flow looks correctUnpack → Implementation.div → repeat for N, then pack. Matches test expectations.
If the intent is to only revert for positive exponent overflow (and allow very negative exponents to lossy-pack to 0), please confirm this is by design so we don’t gate on lossless packing here.
Also applies to: 47-50
| bool lossless; | ||
| (a, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | ||
| (lossless); | ||
| outputs = new StackItem[](1); |
There was a problem hiding this comment.
🧹 Nitpick
Drop temp ‘lossless’ var; mirror run() style
Use tuple discard to avoid the dummy read.
- bool lossless;
- (a, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent);
- (lossless);
+ (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.
| bool lossless; | |
| (a, lossless) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | |
| (lossless); | |
| outputs = new StackItem[](1); | |
| (a,) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | |
| outputs = new StackItem[](1); |
🤖 Prompt for AI Agents
In src/lib/op/math/LibOpDiv.sol around lines 91 to 94, remove the temporary bool
lossless declaration and use tuple discard like the run() style; replace the
two-line assignment with a single tuple-discard assignment: (a, ) =
LibDecimalFloat.packLossy(signedCoefficient, exponent); then proceed with
outputs = new StackItem[](1);
| checkUnhappy("_: div(1 1 0);", abi.encodeWithSelector(DivisionByZero.selector, 1e76, -76)); | ||
| checkUnhappy( |
There was a problem hiding this comment.
🧹 Nitpick
Avoid literals in div-by-zero expected payload (3-input example)
Use named constants for the expected (a) pair to document why 1 maps to (1e76, -76) under the new packing.
- checkUnhappy("_: div(1 1 0);", abi.encodeWithSelector(DivisionByZero.selector, 1e76, -76));
+ checkUnhappy(
+ "_: div(1 1 0);",
+ abi.encodeWithSelector(DivisionByZero.selector, DIV3_DZ_EXPECTED_COEFF, DIV3_DZ_EXPECTED_EXP)
+ );Add near the top:
int256 constant DIV3_DZ_EXPECTED_COEFF = 1e76;
int256 constant DIV3_DZ_EXPECTED_EXP = -76;🤖 Prompt for AI Agents
In test/src/lib/op/math/LibOpDiv.t.sol around lines 169-170, the test uses
hard-coded literals for the expected division-by-zero payload in the 3-input
example; add named constants near the top of the file (e.g., int256 constant
DIV3_DZ_EXPECTED_COEFF = 1e76; int256 constant DIV3_DZ_EXPECTED_EXP = -76;) and
replace the literals in the checkUnhappy call with those constants (use
DIV3_DZ_EXPECTED_COEFF and DIV3_DZ_EXPECTED_EXP) so the expected pair is
documented and self-explanatory.
|
@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 |
1 participant
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit