-
Notifications
You must be signed in to change notification settings - Fork 16
chore: implement simulated execution for bundles in Executable::simulate
#74
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements simulated execution for bundles by replacing the placeholder unreachable!("asd") with a complete simulate_bundle method that mirrors execute_bundle logic but without persisting state changes.
Key Changes:
- Replaces
unreachable!("asd")with actual bundle simulation implementation - Adds comprehensive
simulate_bundlemethod with detailed documentation - Implements the same transaction handling logic as
execute_bundle(optional transactions, allowed-to-fail transactions)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/payload/exec.rs
Outdated
| Ok(ExecResultAndState { result, .. }) | ||
| if result.is_success() || allowed_to_fail => | ||
| { | ||
| results.push(result); |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical Bug: State changes are not committed between bundle transactions
The documentation states that "Each transaction is simulated on the in-memory state produced by the previous transaction in the bundle" (lines 327-329), but the implementation does not commit state changes after successful transactions.
In the execute_bundle method (line 222), db.commit(state) is called after each successful transaction to ensure subsequent transactions see the state changes. However, in this simulation implementation, the state from ExecResultAndState is discarded without being committed.
Solution: Add state.commit(tx_state) after pushing the result:
Ok(ExecResultAndState { result, state: tx_state })
if result.is_success() || allowed_to_fail =>
{
results.push(result);
state.commit(tx_state); // Commit to allow next tx to see changes
// Note: No db.commit(state) for simulation
}Without this fix, each transaction in the bundle will execute against the original database state, not the cumulative state from previous transactions, which breaks bundle semantics.
| Ok(ExecResultAndState { result, .. }) | |
| if result.is_success() || allowed_to_fail => | |
| { | |
| results.push(result); | |
| Ok(ExecResultAndState { result, state: tx_state }) | |
| if result.is_success() || allowed_to_fail => | |
| { | |
| results.push(result); | |
| state.commit(tx_state); // Commit to allow next tx to see changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you should do db.commit
But you shouldn't do merge_transitions
src/payload/exec.rs
Outdated
| /// previous transaction in the bundle, but changes are not committed or | ||
| /// persisted. |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation Inaccuracy: State changes ARE persisted in simulation
The documentation states "Each transaction is simulated on the in-memory state produced by the previous transaction in the bundle, but changes are not committed or persisted" (lines 327-329). However, this is misleading.
While the final state is not returned (line 416 returns BundleState::default()), the in-memory state changes should still be committed within the State object between transactions for the simulation to work correctly. The documentation should clarify that:
- State changes are committed in-memory between transactions within the bundle
- The final aggregated state is not persisted/returned (unlike
execute_bundle)
Consider revising to: "Each transaction is simulated on the in-memory state produced by the previous transaction in the bundle. State changes are applied in-memory for subsequent transactions, but the final state is not persisted."
| /// previous transaction in the bundle, but changes are not committed or | |
| /// persisted. | |
| /// previous transaction in the bundle. State changes are applied in-memory | |
| /// for subsequent transactions, but the final state is not persisted. |
src/payload/exec.rs
Outdated
| if result.is_success() || allowed_to_fail => | ||
| { | ||
| results.push(result); | ||
| // Note: No db.commit(state) for simulation |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading comment about state commitment
The comment says "No db.commit(state) for simulation" but the variable is named state, not db. This is inconsistent with the actual code and could be confusing.
Additionally, this comment is misleading because state changes should be committed between transactions (as noted in the bug comment above). The comment should clarify that:
- State changes ARE committed in-memory between transactions (via
state.commit(tx_state)) - The final state is not returned (line 416 returns
BundleState::default())
Consider revising to: "State changes committed in-memory but not returned (simulation)"
| // Note: No db.commit(state) for simulation | |
| // State changes committed in-memory but not returned (simulation) |
|
Nice! |
|
Pls address copilot comments as they are good ones |
Peponks9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @SozinM, will address the feeback right away
src/payload/exec.rs
Outdated
| /// Post-execution validation is skipped for simulation, as no state changes | ||
| /// are persisted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why we shouldn't do post-execution validation on simulated bundle as well.
The implementation should be almost same as execute_bundle, but without merging transitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agree, thanks for the feeback julio. addressing it
closes #72