Skip to content

test kvs short-circuit and store.set revert paths in LibFlow.flow #449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thedavidmeister wants to merge 1 commit into
base: main
Choose a base branch
from
+36 −0
Open

test kvs short-circuit and store.set revert paths in LibFlow.flow #449

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions test/src/concrete/Flow.time.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,40 @@ contract FlowTimeTest is FlowTest {

flow.flow(evaluable, writeToStore, new SignedContextV1[](0));
}

/// `LibFlow.flow` short-circuits the `interpreterStore.set` call when
/// `kvs.length == 0`. Pin this with an explicit count=0 expectCall so
/// a future refactor that drops the short-circuit is caught.
function testFlowBasicFlowTimeNoStoreSetWhenKvsEmpty() public {
(IFlowV5 flow, EvaluableV2 memory evaluable) = deployFlow();

uint256[] memory stack = generateFlowStack(transferEmpty());
interpreterEval2MockCall(stack, new uint256[](0));

// Mock set to a no-op so the existing REVERTING_MOCK_BYTECODE on
// STORE doesn't accidentally pass the test for the wrong reason.
vm.mockCall(address(STORE), abi.encodeWithSelector(IInterpreterStoreV2.set.selector), abi.encode());
vm.expectCall(address(STORE), abi.encodeWithSelector(IInterpreterStoreV2.set.selector), 0);

flow.flow(evaluable, new uint256[](0), new SignedContextV1[](0));
}

/// A revert from `interpreterStore.set` MUST propagate out of `flow`
/// rather than being caught and swallowed.
/// forge-config: default.fuzz.runs = 100
function testFlowBasicFlowTimeStoreSetRevertBubbles(uint256[] memory writeToStore) public {
vm.assume(writeToStore.length != 0);

(IFlowV5 flow, EvaluableV2 memory evaluable) = deployFlow();

uint256[] memory stack = generateFlowStack(transferEmpty());
interpreterEval2MockCall(stack, writeToStore);

vm.mockCallRevert(
address(STORE), abi.encodeWithSelector(IInterpreterStoreV2.set.selector), abi.encode("STORE_SET_FAILED")
);

vm.expectRevert(abi.encode("STORE_SET_FAILED"));
Comment on lines +59 to +63
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Foundry vm.mockCallRevert revert data bytes encoding — does passing a string literal differ from abi.encode of a string?

💡 Result:

Yes, passing a string literal to vm.mockCallRevert differs from abi.encode(string) for the retdata bytes. The official Foundry documentation shows examples using string literals directly for retdata, such as vm.mockCallRevert(..., "BALANCE_CHECK_FAILED") [1], and then vm.expectRevert("BALANCE_CHECK_FAILED") succeeds [1][2]. This works because Solidity implicitly converts the string literal "msg" to bytes via bytes("msg"), producing raw bytes equal to the UTF-8 encoding of the string (length-prefixed dynamic bytes data without Error(string) selector) [3][4]. In contrast, abi.encode(string) on a string produces ABI-encoded bytes for a single dynamic string argument: 32-byte offset (0x20) followed by length and data [5]. For example, abi.encode("msg") yields different bytes than bytes("msg"). A plain string literal revert like revert("msg") produces Error(string) ABI-encoded data: 0x08c379a0 + offset + length + "msg" [6]. But Foundry docs specify "raw return message" for retdata [1], and examples use raw string bytes to match vm.expectRevert(string) [1]. Thus, string literal → bytes("msg") (raw) ≠ abi.encode("msg") (ABI single arg) ≠ abi.encodeWithSignature("Error(string)", "msg") (full revert) [3]. Use string literals for raw revert messages matching vm.expectRevert(string); use abi.encodeWithSelector for custom errors [1].

Citations:

🏁 Script executed:

rg "slither-disable-next-line" test/src/concrete/Flow.time.t.sol

Repository: rainlanguage/flow

Length of output: 43

Use a raw string literal for the mock revert payload instead of abi.encode().

The test is self-consistent and will pass, but abi.encode("STORE_SET_FAILED") produces ABI-encoded bytes (32-byte offset + 32-byte length + 32-byte padded data), whereas a raw string literal produces raw UTF-8 bytes without the ABI encoding wrapper. These are different formats.

The idiomatic Foundry pattern (as shown in official mockCallRevert documentation) is to pass a raw string literal directly:

vm.mockCallRevert(address(token), ..., "BALANCE_CHECK_FAILED");
vm.expectRevert("BALANCE_CHECK_FAILED");

Using abi.encode() here diverges from Foundry convention and could mislead future readers about the intended format.

♻️ Proposed fix 
-        vm.mockCallRevert(
-            address(STORE), abi.encodeWithSelector(IInterpreterStoreV2.set.selector), abi.encode("STORE_SET_FAILED")
-        );
-
-        vm.expectRevert(abi.encode("STORE_SET_FAILED"));
+        vm.mockCallRevert(
+            address(STORE), abi.encodeWithSelector(IInterpreterStoreV2.set.selector), "STORE_SET_FAILED"
+        );
+
+        vm.expectRevert("STORE_SET_FAILED");
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/src/concrete/Flow.time.t.sol` around lines 59 - 63, Replace the
ABI-encoded revert payloads with raw string literals: in the vm.mockCallRevert
call that currently passes abi.encode("STORE_SET_FAILED") (with
IInterpreterStoreV2.set.selector) and in the vm.expectRevert call that uses
abi.encode("STORE_SET_FAILED"), change both to the raw string "STORE_SET_FAILED"
so the mock revert and the expected revert use the same raw UTF-8 message per
Foundry convention.

flow.flow(evaluable, writeToStore, new SignedContextV1[](0));
}
}
Loading