test duplicate evaluable configs emit twice idempotently#448
test duplicate evaluable configs emit twice idempotently#448thedavidmeister wants to merge 2 commits into
Conversation
`flowInit` does not de-duplicate identical evaluable configs. Two configs that produce the same (interpreter, store, expression) triple emit two FlowInitialized events and write the same registeredFlows slot twice. The registration is idempotent — the resulting clone is still functional with that evaluable. Pinning this prevents a future change from silently introducing dedup, suppressing the second emit, or rejecting duplicates outright without an explicit ABI bump. Closes #327. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 2 minutes and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA new test is added to ChangesTest Addition: Duplicate Evaluables Behavior Verification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. 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: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/src/concrete/Flow.construction.t.sol`:
- Around line 64-71: After asserting the two FlowInitialized logs and that ev0
and ev1 (both typed EvaluableV2) are identical, add a functional assertion that
the deployed clone is callable with that evaluable: invoke the test-suite's
existing flow execution helper (the same helper used elsewhere in this test
file) against the newly deployed clone using ev0 and assert the call succeeds
(no revert) and returns the expected result/state change; reference the decoded
ev0/ev1 values and the FlowInitialized event from vm.getRecordedLogs() when
locating where to add this execution/assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fc94e11-ecdb-4bd2-b8b9-4d434fa8c6aa
📒 Files selected for processing (1)
test/src/concrete/Flow.construction.t.sol
| Vm.Log[] memory init = | ||
| vm.getRecordedLogs().findEvents(keccak256("FlowInitialized(address,(address,address,address))")); | ||
| assertEq(init.length, 2, "duplicate configs MUST emit two FlowInitialized events"); | ||
|
|
||
| (, EvaluableV2 memory ev0) = abi.decode(init[0].data, (address, EvaluableV2)); | ||
| (, EvaluableV2 memory ev1) = abi.decode(init[1].data, (address, EvaluableV2)); | ||
| assertEq(keccak256(abi.encode(ev0)), keccak256(abi.encode(ev1)), "duplicate events MUST carry identical evaluable"); | ||
| } |
There was a problem hiding this comment.
Missing functional assertion after duplicate initialization.
The new test proves duplicate emits (Lines 64-70), but it does not verify the deployed clone remains callable with that evaluable. That post-condition is part of the stated objective, so please add an execution/assertion step after clone deployment using the existing flow execution helper in this test suite.
🤖 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.construction.t.sol` around lines 64 - 71, After
asserting the two FlowInitialized logs and that ev0 and ev1 (both typed
EvaluableV2) are identical, add a functional assertion that the deployed clone
is callable with that evaluable: invoke the test-suite's existing flow execution
helper (the same helper used elsewhere in this test file) against the newly
deployed clone using ev0 and assert the call succeeds (no revert) and returns
the expected result/state change; reference the decoded ev0/ev1 values and the
FlowInitialized event from vm.getRecordedLogs() when locating where to add this
execution/assertion.
Resolves an import-block conflict in test/src/concrete/Flow.construction.t.sol as the union of both sides: keep this PR's EmptyFlowConfig + EvaluableV2 imports (used by the duplicate-evaluable test) alongside main's InsufficientFlowOutputs, UnsupportedFlowInputs, and MIN_FLOW_SENTINELS imports (used by main's insufficient-outputs / non-zero-inputs / at-min tests). forge fmt swept the file. forge build and `forge test --match-path` on the file are green (6/6 pass). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Reviewed 66dc2ff: test duplicate evaluable configs emit twice idempotently — rebased onto main (test-only union conflict resolution + forge fmt), all checks green. LGTM. |
1 participant
Summary
flowInitdoes not de-duplicate identical evaluable configs. Two configs that produce the same(interpreter, store, expression)triple emit twoFlowInitializedevents and write the sameregisteredFlowsslot twice. The registration is idempotent — the resulting clone is still functional with that evaluable.This PR pins the current behavior so a future change cannot silently introduce dedup, suppress the second emit, or reject duplicates outright without an explicit ABI bump.
Closes #327.
Test plan
testFlowConstructionDuplicateEvaluablesEmitTwiceIdempotently— 100 fuzz runs🤖 Generated with Claude Code
Summary by CodeRabbit