Skip to content
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

ExecutionClient returns PromiseInterface #2856

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

diegoximenes
Copy link
Contributor

@diegoximenes diegoximenes commented Dec 24, 2024

Resolves NIT-2566

As a preparation for running Consensus and Execution in different processes, in which they communicate through the network, this PR updates ExecutionClient interface functions to return promises.
Later, clients implementing EecutionClient will handle the network communication with Consensus side.

Instead of going over all functions from the different types of execution clients (ExecutionSequencer, ExecutionRecorder, etc), this PR only updates ExecutionClient.
Most alternative execution clients, besides the one that we already have based on geth, will only implement ExecutionClient instead of all execution clients.
So it seems reasonable to scope down and only introduce changes related to ExecutionClient for now.

This PR also introduces TestExecutionClientOnly, that guarantees that using a execution node that only implements functions related to ExecutionClient is enough to handle some scenarios.

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Dec 24, 2024
@diegoximenes diegoximenes marked this pull request as ready for review December 26, 2024 19:17
tsahee
tsahee previously requested changes Dec 27, 2024
Copy link
Collaborator

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

small changes requested. Others can be either made now or on a separate PR

execution/interface.go Outdated Show resolved Hide resolved
execution/gethexec/node.go Outdated Show resolved Hide resolved
execution/interface.go Show resolved Hide resolved
arbnode/node.go Outdated Show resolved Hide resolved
@diegoximenes diegoximenes marked this pull request as draft January 3, 2025 14:04
@diegoximenes diegoximenes requested a review from tsahee January 3, 2025 16:01
@diegoximenes diegoximenes marked this pull request as ready for review January 3, 2025 16:01
@diegoximenes diegoximenes requested a review from eljobe January 9, 2025 12:31
@tsahee tsahee added the after-next-version This PR shouldn't be merged until after the next version is released label Jan 10, 2025
eljobe
eljobe previously approved these changes Jan 13, 2025
arbnode/transaction_streamer.go Outdated Show resolved Hide resolved
execution/interface.go Show resolved Hide resolved
Copy link
Member

@eljobe eljobe left a comment

Choose a reason for hiding this comment

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

LGTM

@eljobe eljobe enabled auto-merge January 16, 2025 14:35
@eljobe eljobe dismissed tsahee’s stale review January 16, 2025 14:36

I believe all of the changes you requested have been addressed.

@eljobe eljobe disabled auto-merge January 17, 2025 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
after-next-version This PR shouldn't be merged until after the next version is released design-approved s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants