every#381
Conversation
WalkthroughThis change integrates a new "every" opcode into the interpreter by replacing the previous Changes
Sequence Diagram(s)sequenceDiagram
participant Interpreter
participant LibOpEvery
Interpreter->>LibOpEvery: integrity(state, operand)
LibOpEvery-->>Interpreter: (inputCount, outputCount)
Interpreter->>LibOpEvery: run(state, operand, stackTop)
LibOpEvery-->>Interpreter: updated stackTop (last nonzero if all nonzero, else 0)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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}
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 (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 (6)
.gas-snapshot(34 hunks)src/lib/op/LibAllStandardOps.sol(6 hunks)src/lib/op/logic/LibOpEvery.sol(1 hunks)src/lib/op/logic/LibOpEveryNP.sol(0 hunks)test/src/lib/op/logic/LibOpEvery.t.sol(1 hunks)test/src/lib/op/logic/LibOpEveryNP.t.sol(0 hunks)
💤 Files with no reviewable changes (2)
- src/lib/op/logic/LibOpEveryNP.sol
- test/src/lib/op/logic/LibOpEveryNP.t.sol
🧰 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.
.gas-snapshot (1)
Learnt from: thedavidmeister
PR: #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.
src/lib/op/logic/LibOpEvery.sol (1)
Learnt from: thedavidmeister
PR: #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.
test/src/lib/op/logic/LibOpEvery.t.sol (1)
Learnt from: thedavidmeister
PR: #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.
⏰ 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, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: git-clean
🔇 Additional comments (15)
.gas-snapshot (1)
3-958: LGTM! Gas snapshot properly reflects the every opcode integration.The gas snapshot changes appropriately capture:
- Minor gas increases across existing tests due to the larger contract with the new opcode
- Comprehensive test coverage for the new
LibOpEveryTestfunctionality- All adjustments are within expected ranges for this type of system change
src/lib/op/LibAllStandardOps.sol (6)
54-54: LGTM! Import updated to use the new LibOpEvery implementation.The import change correctly references the new
LibOpEverylibrary, replacing the deprecatedLibOpEveryNP.
112-112: LGTM! Constant properly updated for the new opcode.The
ALL_STANDARD_OPS_LENGTHconstant correctly increments from 56 to 57 to account for the addition of the "every" opcode.
221-221: LGTM! Clear and accurate metadata for the every opcode.The metadata entry clearly describes the "every" opcode behavior as "The last nonzero value out of all inputs, or 0 if any input is 0." This effectively differentiates it from the "any" opcode and provides clear usage guidance.
424-425: LGTM! Operand handler correctly configured.The "every" opcode is properly configured with
handleOperandDisallowed, which is appropriate since this opcode doesn't accept operands, consistent with similar logic opcodes.
571-571: LGTM! Integrity function pointer correctly integrated.The
LibOpEvery.integrityfunction pointer is properly added to the integrity function pointers array, maintaining correct alphabetical ordering and array indexing.
679-679: LGTM! Runtime function pointer correctly integrated.The
LibOpEvery.runfunction pointer is properly added to the opcode function pointers array, completing the systematic integration of the new "every" opcode implementation.src/lib/op/logic/LibOpEvery.sol (3)
4-9: LGTM! Clean and appropriate imports.The imports are well-organized and all appear necessary for the opcode implementation.
17-22: LGTM! Correct integrity validation.The function properly extracts input count from the operand and enforces the minimum requirement of 1 input, which is essential for the "every" operation.
48-65: LGTM! Correct reference implementation.The reference function correctly implements the "every" logic for testing purposes, providing a clear and straightforward implementation that matches the expected behavior.
test/src/lib/op/logic/LibOpEvery.t.sol (5)
1-17: LGTM! Well-organized test setup.The imports are comprehensive and appropriate for testing the LibOpEvery functionality.
21-44: LGTM! Comprehensive integrity testing.The tests properly validate both happy and unhappy paths for the integrity function, including the important edge case of zero inputs.
55-86: LGTM! Comprehensive evaluation testing.The tests cover all important scenarios for the "every" opcode behavior, with clear expected results and good test organization.
95-101: LGTM! Good output validation testing.These tests ensure the "every" opcode properly enforces the requirement of exactly 1 output.
46-53: Cannot runforgein sandbox—please verifytestOpEveryRunlocallyThe sandbox environment doesn’t have Foundry/forge installed, so we can’t confirm the fuzz test outcome here. Please run this command on your machine to ensure the optimized
runfunction still matches the reference (and fails as expected if the missing logic is present):
forge test --match-test testOpEveryRun -vv
| /// EVERY is the last nonzero item, else 0. | ||
| function run(InterpreterState memory, OperandV2 operand, Pointer stackTop) internal pure returns (Pointer) { | ||
| unchecked { | ||
| uint256 length = 0x20 * uint256((OperandV2.unwrap(operand) >> 0x10) & bytes32(uint256(0x0F))); | ||
| Float item; | ||
| Pointer cursor = stackTop; | ||
| Pointer end = Pointer.wrap(Pointer.unwrap(stackTop) + length); | ||
| stackTop = Pointer.wrap(Pointer.unwrap(end) - 0x20); | ||
| while (Pointer.unwrap(cursor) < Pointer.unwrap(end)) { | ||
| assembly ("memory-safe") { | ||
| item := mload(cursor) | ||
| } | ||
| if (item.isZero()) { | ||
| assembly ("memory-safe") { | ||
| mstore(stackTop, item) | ||
| } | ||
| break; | ||
| } | ||
| cursor = Pointer.wrap(Pointer.unwrap(cursor) + 0x20); | ||
| } | ||
| } | ||
| return stackTop; | ||
| } |
There was a problem hiding this comment.
Critical issue: Incomplete logic in run function.
The function correctly handles the case where a zero is found (stores zero and breaks), but it doesn't handle the case where all items are nonzero. In that scenario, the last item should be stored at stackTop, but this logic is missing.
Add the missing logic to store the last item when all are nonzero:
while (Pointer.unwrap(cursor) < Pointer.unwrap(end)) {
assembly ("memory-safe") {
item := mload(cursor)
}
if (item.isZero()) {
assembly ("memory-safe") {
mstore(stackTop, item)
}
break;
}
cursor = Pointer.wrap(Pointer.unwrap(cursor) + 0x20);
}
+ // If we reached here without breaking, all items were nonzero
+ // Store the last item (which is in 'item' variable)
+ if (Pointer.unwrap(cursor) >= Pointer.unwrap(end)) {
+ assembly ("memory-safe") {
+ mstore(stackTop, item)
+ }
+ }📝 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.
| /// EVERY is the last nonzero item, else 0. | |
| function run(InterpreterState memory, OperandV2 operand, Pointer stackTop) internal pure returns (Pointer) { | |
| unchecked { | |
| uint256 length = 0x20 * uint256((OperandV2.unwrap(operand) >> 0x10) & bytes32(uint256(0x0F))); | |
| Float item; | |
| Pointer cursor = stackTop; | |
| Pointer end = Pointer.wrap(Pointer.unwrap(stackTop) + length); | |
| stackTop = Pointer.wrap(Pointer.unwrap(end) - 0x20); | |
| while (Pointer.unwrap(cursor) < Pointer.unwrap(end)) { | |
| assembly ("memory-safe") { | |
| item := mload(cursor) | |
| } | |
| if (item.isZero()) { | |
| assembly ("memory-safe") { | |
| mstore(stackTop, item) | |
| } | |
| break; | |
| } | |
| cursor = Pointer.wrap(Pointer.unwrap(cursor) + 0x20); | |
| } | |
| } | |
| return stackTop; | |
| } | |
| /// EVERY is the last nonzero item, else 0. | |
| function run(InterpreterState memory, OperandV2 operand, Pointer stackTop) internal pure returns (Pointer) { | |
| unchecked { | |
| uint256 length = 0x20 * uint256((OperandV2.unwrap(operand) >> 0x10) & bytes32(uint256(0x0F))); | |
| Float item; | |
| Pointer cursor = stackTop; | |
| Pointer end = Pointer.wrap(Pointer.unwrap(stackTop) + length); | |
| stackTop = Pointer.wrap(Pointer.unwrap(end) - 0x20); | |
| while (Pointer.unwrap(cursor) < Pointer.unwrap(end)) { | |
| assembly ("memory-safe") { | |
| item := mload(cursor) | |
| } | |
| if (item.isZero()) { | |
| assembly ("memory-safe") { | |
| mstore(stackTop, item) | |
| } | |
| break; | |
| } | |
| cursor = Pointer.wrap(Pointer.unwrap(cursor) + 0x20); | |
| } | |
| // If we reached here without breaking, all items were nonzero | |
| // Store the last item (which is in 'item' variable) | |
| if (Pointer.unwrap(cursor) >= Pointer.unwrap(end)) { | |
| assembly ("memory-safe") { | |
| mstore(stackTop, item) | |
| } | |
| } | |
| } | |
| return stackTop; | |
| } |
🤖 Prompt for AI Agents
In src/lib/op/logic/LibOpEvery.sol between lines 24 and 46, the run function
currently only stores and breaks when a zero item is found but lacks handling
for the case when all items are nonzero. To fix this, after the while loop
completes without encountering a zero, add logic to store the last nonzero item
at stackTop before returning. This ensures the function returns the last nonzero
item as intended.
There was a problem hiding this comment.
item is read from that position so it doesn't need to be written
There was a problem hiding this comment.
@thedavidmeister You're absolutely correct! I misunderstood the stack manipulation logic.
Looking at it again:
stackTopis set to point to the last item's position:Pointer.wrap(Pointer.unwrap(end) - 0x20)- When all items are nonzero, the loop completes without modifying the stack contents
- The last item is already at the
stackTopposition, which is exactly where the output should be - No explicit write is needed because the output position coincides with the last input's position
Thank you for the clarification - the logic is indeed complete as written.
✏️ Learnings added
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#381
File: src/lib/op/logic/LibOpEvery.sol:24-46
Timestamp: 2025-07-27T22:56:57.898Z
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.
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.
🧠 Learnings used
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.
Learnt from: 0xgleb
PR: rainlanguage/rain.interpreter#334
File: crates/eval/src/trace.rs:92-118
Timestamp: 2025-06-17T18:01:06.316Z
Learning: User 0xgleb considers refactoring to remove a single duplicate as premature optimization in crates/eval/src/trace.rs when dealing with trace-filtering logic.
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.
| /// Test that every without inputs fails integrity check. | ||
| function testOpAnyEvalFail() public { | ||
| vm.expectRevert(abi.encodeWithSelector(BadOpInputsLength.selector, 0, 1, 0)); | ||
| bytes memory bytecode = iDeployer.parse2("_: every();"); | ||
| (bytecode); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix the test function name.
The function tests the "every" opcode but is named "testOpAnyEvalFail", which appears to be a copy-paste error.
- function testOpAnyEvalFail() public {
+ function testOpEveryEvalFail() public {📝 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.
| /// Test that every without inputs fails integrity check. | |
| function testOpAnyEvalFail() public { | |
| vm.expectRevert(abi.encodeWithSelector(BadOpInputsLength.selector, 0, 1, 0)); | |
| bytes memory bytecode = iDeployer.parse2("_: every();"); | |
| (bytecode); | |
| } | |
| /// Test that every without inputs fails integrity check. | |
| function testOpEveryEvalFail() public { | |
| vm.expectRevert(abi.encodeWithSelector(BadOpInputsLength.selector, 0, 1, 0)); | |
| bytes memory bytecode = iDeployer.parse2("_: every();"); | |
| (bytecode); | |
| } |
🤖 Prompt for AI Agents
In test/src/lib/op/logic/LibOpEvery.t.sol around lines 88 to 93, the test
function name is incorrect; it is named testOpAnyEvalFail but tests the "every"
opcode. Rename the function to testOpEveryEvalFail to accurately reflect the
opcode being tested.
1 participant
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores