exp op#393
Merged
Merged
Conversation
Contributor
WalkthroughThe changes re-enable and fully integrate the "exp" (natural exponential) opcode into the standard operations set. This includes updating function pointers, metadata, and tests to use a new decimal floating-point implementation. The test suite and library now utilize updated operand and stack item types, and the exponential function is computed using Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Interpreter
participant LibAllStandardOps
participant LibOpExp
participant LibDecimalFloat
User->>Interpreter: Executes "exp" opcode
Interpreter->>LibAllStandardOps: Lookup function pointers for "exp"
LibAllStandardOps->>LibOpExp: Calls run() with OperandV2 and stackTop
LibOpExp->>LibDecimalFloat: Computes FLOAT_E.pow(input, logTables)
LibDecimalFloat-->>LibOpExp: Returns result
LibOpExp-->>Interpreter: Returns new stackTop with result
Interpreter-->>User: Returns computation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (3)
src/generated/Rainterpreter.pointers.solis excluded by!**/generated/**src/generated/RainterpreterExpressionDeployer.pointers.solis excluded by!**/generated/**src/generated/RainterpreterParser.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (4)
src/lib/op/LibAllStandardOps.sol(6 hunks)src/lib/op/math/LibOpExp.sol(1 hunks)test/src/lib/op/math/LibOpExp.t.sol(1 hunks)test/src/lib/op/math/LibOpSqrt.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#360
File: src/lib/op/erc20/LibOpERC20Allowance.sol:0-0
Timestamp: 2025-07-15T11:31:28.010Z
Learning: In the rainlanguage/rain.interpreter project, forge (Foundry's formatting tool) handles code formatting automatically, so formatting-related suggestions are not actionable.
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#360
File: src/lib/op/erc20/LibOpERC20Allowance.sol:35-36
Timestamp: 2025-07-15T11:31:10.098Z
Learning: In the rain.interpreter codebase, when working with ERC20 tokens that may not implement the optional decimals() function, the preference is to let the call fail explicitly rather than catching errors and defaulting to 18 decimals. This ensures correctness by avoiding potentially incorrect calculations with assumed decimal values.
📚 Learning: in multiplication overflow detection tests like libopuint256multest, when performing sequential mult...
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/LibOpSqrt.t.soltest/src/lib/op/math/LibOpExp.t.sol
📚 Learning: in the rain.interpreter codebase, when working with erc20 tokens that may not implement the optional...
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#360
File: src/lib/op/erc20/LibOpERC20Allowance.sol:35-36
Timestamp: 2025-07-15T11:31:10.098Z
Learning: In the rain.interpreter codebase, when working with ERC20 tokens that may not implement the optional decimals() function, the preference is to let the call fail explicitly rather than catching errors and defaulting to 18 decimals. This ensures correctness by avoiding potentially incorrect calculations with assumed decimal values.
Applied to files:
src/lib/op/math/LibOpExp.sol
📚 Learning: in rain interpreter stack operations like libopevery, when the output position (stacktop) is set to ...
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#381
File: src/lib/op/logic/LibOpEvery.sol:24-46
Timestamp: 2025-07-27T22:56:57.928Z
Learning: In Rain interpreter stack operations like LibOpEvery, when the output position (stackTop) is set to coincide with an input item's position on the stack, explicit writing may not be needed if the desired output value is already at that position. The function can return the pointer to that position directly, leveraging the existing stack layout.
Applied to files:
src/lib/op/math/LibOpExp.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 (macos-latest, rainix-rs-test)
- 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 (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (3)
src/lib/op/math/LibOpExp.sol (2)
23-34: Consider edge case handling for exponential functionThe implementation delegates all computation to
LibDecimalFloat.FLOAT_E.powwithout any input validation. Consider whether edge cases need explicit handling:
- Very large positive inputs that could overflow
- Very large negative inputs that approach zero
- Special float values (NaN, infinity)
The change from
puretoviewis appropriate given the external log tables access.
28-28: Verify log tables address initializationThe
LibDecimalFloat.LOG_TABLES_ADDRESSconstant comes from an external library and must point to a deployed log-tables contract in every environment (e.g., testnets, mainnet). Please confirm that your library linking or deployment/migration scripts correctly supply this address.Usage to double-check:
- src/lib/op/math/LibOpExp.sol at lines 28 & 43
src/lib/op/LibAllStandardOps.sol (1)
79-79: LGTM! Complete integration of exp opcodeThe exp opcode has been properly integrated into the standard operations set:
- Import statement added
- ALL_STANDARD_OPS_LENGTH correctly incremented
- All required function pointers (metadata, operand handler, integrity, run) properly configured
- Alphabetical ordering maintained
Also applies to: 115-115, 276-276, 475-476, 607-607, 718-718
Comment on lines
+24
to
+26
| function testOpExpRun(int224 signedCoefficientA, int32 exponentA, uint16 operandData) public view { | ||
| signedCoefficientA = int224(bound(signedCoefficientA, 0, 10000)); | ||
| exponentA = int32(bound(exponentA, -10, 5)); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider expanding fuzz test bounds and adding negative input tests
The fuzz test bounds are quite restrictive:
- Coefficient is bounded to [0, 10000] - consider testing negative values as e^x is defined for all real x
- Exponent range [-10, 5] might miss edge cases with larger magnitudes
Also consider adding explicit test cases for:
- Negative inputs (e.g.,
exp(-1),exp(-2)) - Edge cases near overflow/underflow boundaries
- signedCoefficientA = int224(bound(signedCoefficientA, 0, 10000));
- exponentA = int32(bound(exponentA, -10, 5));
+ signedCoefficientA = int224(bound(signedCoefficientA, -100000, 100000));
+ exponentA = int32(bound(exponentA, -20, 10));📝 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.
Suggested change
| function testOpExpRun(int224 signedCoefficientA, int32 exponentA, uint16 operandData) public view { | |
| signedCoefficientA = int224(bound(signedCoefficientA, 0, 10000)); | |
| exponentA = int32(bound(exponentA, -10, 5)); | |
| function testOpExpRun(int224 signedCoefficientA, int32 exponentA, uint16 operandData) public view { | |
| - signedCoefficientA = int224(bound(signedCoefficientA, 0, 10000)); | |
| - exponentA = int32(bound(exponentA, -10, 5)); | |
| + signedCoefficientA = int224(bound(signedCoefficientA, -100000, 100000)); | |
| + exponentA = int32(bound(exponentA, -20, 10)); |
🤖 Prompt for AI Agents
In test/src/lib/op/math/LibOpExp.t.sol around lines 24 to 26, the fuzz test
bounds are too narrow and exclude negative coefficients, limiting test coverage.
Expand the bounds for signedCoefficientA to include negative values, for example
from a negative lower bound to a positive upper bound, and increase the
exponentA range to cover larger magnitudes both positive and negative.
Additionally, add explicit test cases for negative inputs such as exp(-1) and
exp(-2), and include tests targeting edge cases near overflow and underflow
boundaries to ensure robustness.
Comment on lines
+40
to
+62
| function testOpExpEval() external view { | ||
| checkHappy("_: exp(0);", Float.unwrap(LibDecimalFloat.packLossless(1, 0)), "e^0"); | ||
| checkHappy( | ||
| "_: exp(1);", | ||
| Float.unwrap(LibDecimalFloat.packLossless(271828182845904523536028747135266249770000, -41)), | ||
| "e^1" | ||
| ); | ||
| checkHappy( | ||
| "_: exp(0.5);", | ||
| Float.unwrap(LibDecimalFloat.packLossless(164864091422952261768014373567633124880000, -41)), | ||
| "e^0.5" | ||
| ); | ||
| checkHappy( | ||
| "_: exp(2);", | ||
| Float.unwrap(LibDecimalFloat.packLossless(73901273138361809414411498854106499908000, -40)), | ||
| "e^2" | ||
| ); | ||
| checkHappy( | ||
| "_: exp(3);", | ||
| Float.unwrap(LibDecimalFloat.packLossless(200884548537713570608086241405798749310000, -40)), | ||
| "e^3" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add test cases for negative inputs
The eval tests only cover positive inputs. Consider adding test cases for negative values to ensure the exponential function handles them correctly:
checkHappy(
"_: exp(3);",
Float.unwrap(LibDecimalFloat.packLossless(200884548537713570608086241405798749310000, -40)),
"e^3"
);
+ checkHappy(
+ "_: exp(-1);",
+ Float.unwrap(LibDecimalFloat.packLossless(367897834377610754067267193921935356200000, -42)),
+ "e^-1"
+ );
+ checkHappy(
+ "_: exp(-2);",
+ Float.unwrap(LibDecimalFloat.packLossless(135263130223700958314976198650936531970000, -42)),
+ "e^-2"
+ );📝 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.
Suggested change
| function testOpExpEval() external view { | |
| checkHappy("_: exp(0);", Float.unwrap(LibDecimalFloat.packLossless(1, 0)), "e^0"); | |
| checkHappy( | |
| "_: exp(1);", | |
| Float.unwrap(LibDecimalFloat.packLossless(271828182845904523536028747135266249770000, -41)), | |
| "e^1" | |
| ); | |
| checkHappy( | |
| "_: exp(0.5);", | |
| Float.unwrap(LibDecimalFloat.packLossless(164864091422952261768014373567633124880000, -41)), | |
| "e^0.5" | |
| ); | |
| checkHappy( | |
| "_: exp(2);", | |
| Float.unwrap(LibDecimalFloat.packLossless(73901273138361809414411498854106499908000, -40)), | |
| "e^2" | |
| ); | |
| checkHappy( | |
| "_: exp(3);", | |
| Float.unwrap(LibDecimalFloat.packLossless(200884548537713570608086241405798749310000, -40)), | |
| "e^3" | |
| ); | |
| } | |
| function testOpExpEval() external view { | |
| checkHappy("_: exp(0);", Float.unwrap(LibDecimalFloat.packLossless(1, 0)), "e^0"); | |
| checkHappy( | |
| "_: exp(1);", | |
| Float.unwrap(LibDecimalFloat.packLossless(271828182845904523536028747135266249770000, -41)), | |
| "e^1" | |
| ); | |
| checkHappy( | |
| "_: exp(0.5);", | |
| Float.unwrap(LibDecimalFloat.packLossless(164864091422952261768014373567633124880000, -41)), | |
| "e^0.5" | |
| ); | |
| checkHappy( | |
| "_: exp(2);", | |
| Float.unwrap(LibDecimalFloat.packLossless(73901273138361809414411498854106499908000, -40)), | |
| "e^2" | |
| ); | |
| checkHappy( | |
| "_: exp(3);", | |
| Float.unwrap(LibDecimalFloat.packLossless(200884548537713570608086241405798749310000, -40)), | |
| "e^3" | |
| ); | |
| + checkHappy( | |
| + "_: exp(-1);", | |
| + Float.unwrap(LibDecimalFloat.packLossless(367897834377610754067267193921935356200000, -42)), | |
| + "e^-1" | |
| + ); | |
| + checkHappy( | |
| + "_: exp(-2);", | |
| + Float.unwrap(LibDecimalFloat.packLossless(135263130223700958314976198650936531970000, -42)), | |
| + "e^-2" | |
| + ); | |
| } |
🤖 Prompt for AI Agents
In test/src/lib/op/math/LibOpExp.t.sol between lines 40 and 62, the
testOpExpEval function only tests positive inputs for the exponential function.
Add additional test cases with negative input values such as -1, -0.5, and -2 to
verify that the exponential function correctly handles negative exponents. Use
the same checkHappy pattern with appropriate expected results for these negative
inputs.
|
|
||
| /// Directly test the runtime logic of LibOpSqrt. | ||
| function testOpSqrtRun(Float a) public view { | ||
| vm.assume(!a.eq(LibDecimalFloat.FLOAT_MIN_NEGATIVE_VALUE)); |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document why FLOAT_MIN_NEGATIVE_VALUE is excluded from testing
The test excludes LibDecimalFloat.FLOAT_MIN_NEGATIVE_VALUE from the fuzz inputs. This edge case exclusion could mask potential bugs in the sqrt implementation. Please add a comment explaining why this specific value needs to be excluded, or consider handling it properly in the implementation.
🏁 Script executed:
#!/bin/bash
# Check if FLOAT_MIN_NEGATIVE_VALUE is handled consistently across other math operations
ast-grep --pattern 'FLOAT_MIN_NEGATIVE_VALUE' Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for all references to FLOAT_MIN_NEGATIVE_VALUE in the repo
rg -n "FLOAT_MIN_NEGATIVE_VALUE" .
# Locate its definition in the LibDecimalFloat contract
rg -n "constant FLOAT_MIN_NEGATIVE_VALUE" -t solLength of output: 496
🏁 Script executed:
#!/bin/bash
# Locate the Solidity file defining LibDecimalFloat
rg -l "library LibDecimalFloat" --glob '*.sol'
# Search for FLOAT_MIN_NEGATIVE_VALUE definition in all .sol files
rg -n "FLOAT_MIN_NEGATIVE_VALUE" --glob '*.sol'Length of output: 473
🏁 Script executed:
#!/bin/bash
# Display the sqrt test with context around the assume
sed -n '1,80p' test/src/lib/op/math/LibOpSqrt.t.sol
# Locate and display the implementation of LibOpSqrt
file=$(fd LibOpSqrt.sol --search-path src)
if [ -n "$file" ]; then
echo "Implementation file: $file"
sed -n '1,80p' "$file"
else
echo "LibOpSqrt.sol not found"
fiLength of output: 4913
Add a comment explaining why the smallest negative float is excluded
The line in testOpSqrtRun excludes LibDecimalFloat.FLOAT_MIN_NEGATIVE_VALUE, likely because calling .abs() on the smallest negative value overflows or is undefined. Please document this edge-case in the test (or, if you intend to support it, handle it in the implementation and add a test). For example:
/// Directly test the runtime logic of LibOpSqrt.
function testOpSqrtRun(Float a) public view {
- vm.assume(!a.eq(LibDecimalFloat.FLOAT_MIN_NEGATIVE_VALUE));
+ // Exclude the minimum negative float because abs(min) cannot be represented
+ vm.assume(!a.eq(LibDecimalFloat.FLOAT_MIN_NEGATIVE_VALUE));
a = a.abs();
InterpreterState memory state = opTestDefaultInterpreterState();
…• File: test/src/lib/op/math/LibOpSqrt.t.sol
Line: 27
📝 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.
Suggested change
| vm.assume(!a.eq(LibDecimalFloat.FLOAT_MIN_NEGATIVE_VALUE)); | |
| /// Directly test the runtime logic of LibOpSqrt. | |
| function testOpSqrtRun(Float a) public view { | |
| - vm.assume(!a.eq(LibDecimalFloat.FLOAT_MIN_NEGATIVE_VALUE)); | |
| + // Exclude the minimum negative float because abs(min) cannot be represented | |
| + vm.assume(!a.eq(LibDecimalFloat.FLOAT_MIN_NEGATIVE_VALUE)); | |
| a = a.abs(); | |
| InterpreterState memory state = opTestDefaultInterpreterState(); | |
| … |
🤖 Prompt for AI Agents
In test/src/lib/op/math/LibOpSqrt.t.sol at line 27, add a comment explaining
that the smallest negative float is excluded because taking its absolute value
causes overflow or undefined behavior. This clarifies the reason for the
vm.assume condition and documents the edge case for future reference.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests