Skip to content

Conversation

@m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented Oct 3, 2025

@m-Peter m-Peter self-assigned this Oct 3, 2025
@m-Peter m-Peter requested a review from a team as a code owner October 3, 2025 08:01
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Member

@Kay-Zee Kay-Zee left a comment

Choose a reason for hiding this comment

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

Great! Looks like the main difference is changing from run to runAndReturn, which also completely removes the run function, which had previously used invokeMetaTransaction and now just use runMetaTransaction, I believe the main thing here is that the events are returned, were there any other side effects to this change?

@m-Peter
Copy link
Collaborator Author

m-Peter commented Oct 6, 2025

Great! Looks like the main difference is changing from run to runAndReturn, which also completely removes the run function, which had previously used invokeMetaTransaction and now just use runMetaTransaction, I believe the main thing here is that the events are returned, were there any other side effects to this change?

Hey @Kay-Zee , so invokeMetaTransaction is just a wrapper around runMetaTransaction, with the only difference being that it doesn't return the ProcedureOutput, but only the procedure's Err.

You are right, the main reason to use runMetaTransaction is so that we can collect the events that are included in the resulting ProcedureOutput. These events are accumulated throughout the bootstrapping process of the VM bridge, and at last they are included in the bootstrapExecutor's ProcedureOutput, which previously was simply an empty object .

The only other issue I had to fix, was setting a proper transaction index, for each transaction that was executed as part of the VM bridge bootstrap, as the order to EVM.TransactionExecuted events is critical for the EVM Gateway.

Other than that, I didn't notice any side effects.

@Kay-Zee
Copy link
Member

Kay-Zee commented Oct 6, 2025

Alright, LGTM!

Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Nice!
Only had one minor comment.

@m-Peter
Copy link
Collaborator Author

m-Peter commented Oct 7, 2025

Nice! Only had one minor comment.

@janezpodhostnik There doesn't seem to be any written comments 😅

fvm/bootstrap.go Outdated
panic(fmt.Sprintf("failed to build pause the bridge contracts transaction: %s", err))
}
run(txBody, "failed to pause the bridge contracts: %s")
runAndReturn(txBody, "failed to pause the bridge contracts: %s")
Copy link
Contributor

Choose a reason for hiding this comment

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

if we don't need run anymore can we rename runAndReturn to run

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point 👍 Renamed it in 46df27c .

@janezpodhostnik
Copy link
Contributor

Sorry about that. Added it now.

@m-Peter m-Peter force-pushed the mpeter/fix-evm-vm-bridge-bootstrapping branch from 7de2383 to 46df27c Compare October 7, 2025 16:15
@m-Peter m-Peter added this pull request to the merge queue Oct 7, 2025
Merged via the queue into master with commit c6967df Oct 7, 2025
57 checks passed
@m-Peter m-Peter deleted the mpeter/fix-evm-vm-bridge-bootstrapping branch October 7, 2025 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants