test duplicate evaluable configs emit twice idempotently#448
Open
thedavidmeister wants to merge 1 commit intomainfrom
Open
test duplicate evaluable configs emit twice idempotently#448thedavidmeister wants to merge 1 commit intomainfrom
thedavidmeister wants to merge 1 commit intomainfrom
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>
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
Comment on lines
+64
to
+71
| 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.
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.
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