Skip to content

Conversation

@Peponks9
Copy link
Contributor

closes #72

Copilot AI review requested due to automatic review settings November 18, 2025 18:55
Copilot finished reviewing on behalf of Peponks9 November 18, 2025 18:58
Copy link
Contributor

Copilot AI left a 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_bundle method 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.

Comment on lines 383 to 386
Ok(ExecResultAndState { result, .. })
if result.is_success() || allowed_to_fail =>
{
results.push(result);
Copy link

Copilot AI Nov 18, 2025

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

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

Comment on lines 328 to 329
/// previous transaction in the bundle, but changes are not committed or
/// persisted.
Copy link

Copilot AI Nov 18, 2025

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."

Suggested change
/// 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.

Copilot uses AI. Check for mistakes.
if result.is_success() || allowed_to_fail =>
{
results.push(result);
// Note: No db.commit(state) for simulation
Copy link

Copilot AI Nov 18, 2025

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)"

Suggested change
// Note: No db.commit(state) for simulation
// State changes committed in-memory but not returned (simulation)

Copilot uses AI. Check for mistakes.
@SozinM
Copy link
Collaborator

SozinM commented Nov 19, 2025

Nice!

@SozinM
Copy link
Collaborator

SozinM commented Nov 19, 2025

Pls address copilot comments as they are good ones

Copy link
Contributor Author

@Peponks9 Peponks9 left a 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

Comment on lines 350 to 351
/// Post-execution validation is skipped for simulation, as no state changes
/// are persisted.
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feat: Enable simulated execution for Bundle

3 participants