Skip to content

test InsufficientFlowOutputs revert and boundary #442

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

Merged
thedavidmeister merged 2 commits into from
May 7, 2026
+42 −1
Merged

test InsufficientFlowOutputs revert and boundary #442

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cf7d5e
test InsufficientFlowOutputs revert and boundary
thedavidmeister May 4, 2026
2762839
Merge remote-tracking branch 'origin/main' into 2026-05-04-issue-311-…
thedavidmeister May 7, 2026
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
43 changes: 42 additions & 1 deletion test/src/concrete/Flow.construction.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import {Vm} from "forge-std/Test.sol";

import {EvaluableConfigV3} from "rain.interpreter.interface/interface/IInterpreterCallerV2.sol";
import {FlowTest} from "test/abstract/FlowTest.sol";
import {EmptyFlowConfig, UnsupportedFlowInputs} from "src/error/ErrFlow.sol";
import {EmptyFlowConfig, InsufficientFlowOutputs, UnsupportedFlowInputs} from "src/error/ErrFlow.sol";
import {MIN_FLOW_SENTINELS} from "src/interface/IFlowV5.sol";

contract FlowConstructionTest is FlowTest {
function testFlowConstructionEmptyConfigReverts() external {
Expand All @@ -16,6 +17,45 @@ contract FlowConstructionTest is FlowTest {
I_CLONE_FACTORY.clone(impl, abi.encode(emptyConfig));
}

/// Reverts with `InsufficientFlowOutputs` when the deployer reports a
/// `flowOutputs` byte below `MIN_FLOW_SENTINELS` (= 3). Pinning this
/// protects the lower-bound guard at `Flow.flowInit` against regression.
/// forge-config: default.fuzz.runs = 100
function testFlowConstructionRevertsOnInsufficientFlowOutputs(
address expression,
bytes memory bytecode,
uint256[] memory constants,
uint8 flowOutputs
) external {
vm.assume(flowOutputs < uint8(MIN_FLOW_SENTINELS));
bytes memory io = abi.encodePacked(uint8(0), flowOutputs);
expressionDeployerDeployExpression2MockCall(expression, io);

EvaluableConfigV3[] memory flowConfig = new EvaluableConfigV3[](1);
flowConfig[0] = EvaluableConfigV3(DEPLOYER, bytecode, constants);

address impl = deployFlowImplementation();
vm.expectRevert(InsufficientFlowOutputs.selector);
I_CLONE_FACTORY.clone(impl, abi.encode(flowConfig));
}

/// `flowOutputs == MIN_FLOW_SENTINELS` is the lowest accepted value.
/// Pinning this boundary against regression complements the negative
/// test above.
function testFlowConstructionAcceptsFlowOutputsAtMin(
Comment on lines +42 to +45
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add explicit fuzz-run pin for the boundary test.

Line 45 introduces the boundary test, but unlike the revert test it does not pin fuzz runs. If the intent is the stated 2048-run boundary sweep, add an explicit forge-config line so this is stable and reproducible.

Suggested patch 
     /// `flowOutputs == MIN_FLOW_SENTINELS` is the lowest accepted value.
     /// Pinning this boundary against regression complements the negative
     /// test above.
+    /// forge-config: default.fuzz.runs = 2048
     function testFlowConstructionAcceptsFlowOutputsAtMin(
📝 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
/// `flowOutputs == MIN_FLOW_SENTINELS` is the lowest accepted value.
/// Pinning this boundary against regression complements the negative
/// test above.
function testFlowConstructionAcceptsFlowOutputsAtMin(
/// `flowOutputs == MIN_FLOW_SENTINELS` is the lowest accepted value.
/// Pinning this boundary against regression complements the negative
/// test above.
/// forge-config: default.fuzz.runs = 2048
function testFlowConstructionAcceptsFlowOutputsAtMin(
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/src/concrete/Flow.construction.t.sol` around lines 42 - 45, Add an
explicit fuzz-run pin for the boundary test by configuring the test to run a
fixed number of fuzz iterations (e.g., 2048) so the sweep is stable and
reproducible; update the test harness or metadata for the function
testFlowConstructionAcceptsFlowOutputsAtMin to include the forge-config /
fuzz-runs directive (matching the pattern used in the revert test), ensuring the
test is executed with the pinned run count every time.

Copy link
Copy Markdown
Contributor Author

@thedavidmeister thedavidmeister May 7, 2026

Choose a reason for hiding this comment

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

Rejecting. The pattern you reference doesn't exist:

  • Revert test pins forge-config: default.fuzz.runs = 100 — an override down from the project default to keep the slow vm.assume sweep cheap.
  • Accept test has no override and inherits foundry.toml's fuzz.runs = 2048 already.

Adding default.fuzz.runs = 2048 to the accept test is a no-op duplicate of the project default, not "matching the pattern". The boundary test is already running 2048 iterations.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 7, 2026

Choose a reason for hiding this comment

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

@thedavidmeister, you're right — I misread the intent. The forge-config directive on the revert test is a deliberate downward override to offset the cost of vm.assume rejection sampling, while the accept test correctly relies on the foundry.toml project default of 2048 runs. Adding the directive there would be a no-op. Acknowledged and sorry for the noise.

✏️ Learnings added 
Learnt from: thedavidmeister
Repo: rainlanguage/flow PR: 442
File: test/src/concrete/Flow.construction.t.sol:42-45
Timestamp: 2026-05-07T12:52:45.226Z
Learning: In the `rainlanguage/flow` repository (Solidity/Foundry), `forge-config: default.fuzz.runs = N` in test NatDoc comments is used as a **downward** override from the project default (set to 2048 in `foundry.toml`). Tests with a heavy `vm.assume` rejection loop (e.g., `vm.assume(x < MIN_FLOW_SENTINELS)`) pin a lower run count (e.g., 100) to keep CI cheap. Tests without the directive intentionally inherit the 2048-run project default — adding `forge-config: default.fuzz.runs = 2048` to such tests is a no-op duplicate and should not be suggested.

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.

address expression,
bytes memory bytecode,
uint256[] memory constants
) external {
bytes memory io = abi.encodePacked(uint8(0), uint8(MIN_FLOW_SENTINELS));
expressionDeployerDeployExpression2MockCall(expression, io);

EvaluableConfigV3[] memory flowConfig = new EvaluableConfigV3[](1);
flowConfig[0] = EvaluableConfigV3(DEPLOYER, bytecode, constants);

I_CLONE_FACTORY.clone(deployFlowImplementation(), abi.encode(flowConfig));
}

/// Reverts with `UnsupportedFlowInputs` when the deployer reports any
/// non-zero `flowInputs` byte in the IO string. Pinning this protects
/// against a future deployer behaviour change leaking non-zero inputs
Expand All @@ -40,6 +80,7 @@ contract FlowConstructionTest is FlowTest {
I_CLONE_FACTORY.clone(impl, abi.encode(flowConfig));
}


function testFlowConstructionInitialize(address expression, bytes memory bytecode, uint256[] memory constants)
external
{
Expand Down
Loading