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

Move block producer requirements into corresponding crates #814

Merged
merged 4 commits into from
Dec 6, 2022

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Dec 5, 2022

I reused the pattern of the adapter for the block-producer and moved block-producer requirements into corresponding crates.
It is part of the change to return an ExecutionResult with a database transaction inside. The rest of the change will be in a separate PR.

Reused patter of adapter in the `fuel-core`.
@xgreenx xgreenx requested a review from a team December 5, 2022 01:04
@xgreenx xgreenx self-assigned this Dec 5, 2022
Copy link
Contributor

@ControlCplusControlV ControlCplusControlV left a comment

Choose a reason for hiding this comment

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

Wait for discussion for others, I had a couple questions, but nothing overall. Main concern is this + db causing large churn before other more pressing issues (like the ones mentioned on slack) can be fixed

fuel-block-producer/src/block_producer.rs Show resolved Hide resolved
fuel-core/src/service/modules.rs Outdated Show resolved Hide resolved
@@ -56,6 +59,24 @@ use tracing::{
warn,
};

#[async_trait::async_trait]
pub trait BlockProducer: Send + Sync {
// TODO: Right now production and execution of the block is one step, but in the future,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are refactoring right now, would it be worth considering building out this code path right now to make it easier for future milestones. produce_and_execute_block is relatively simple and we could make a produce function which returns a block, then just call an execute function right away, which preserves functionality but avoids a future breaking interface change on the BlockProducer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not refactoring mentioned in the #809. It is simply moving from one place to another to unblock the development for me and don't create a mess in one PR=)

The comment describes the problem bad=) We can't produce the block without the execution of transactions(otherwise, it is a useless block). The comment was meant to be, "The current code produces, executes, and commits the block. But it should only produce and execute".

It is that I'm trying to fix=) This "refactoring" for this change. But without #812 it looks ugly.

@ControlCplusControlV ControlCplusControlV added breaking A breaking api change fuel-block-executor architecture Something related to the architecture or the architecture description itself. labels Dec 5, 2022
@xgreenx xgreenx requested a review from a team December 5, 2022 03:30
}

#[async_trait::async_trait]
pub trait Relayer: Sync + Send {
Copy link
Member

@Voxelot Voxelot Dec 5, 2022

Choose a reason for hiding this comment

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

could we move this to a ports module? Just so we have a consistent pattern for locating these kinds of external dep interfaces within each crate.

@@ -301,3 +305,31 @@ impl BlockProducerRelayer for MaybeRelayerAdapter {
.unwrap_or_default())
}
}

struct PoACoordinatorAdapter {
Copy link
Member

@Voxelot Voxelot Dec 5, 2022

Choose a reason for hiding this comment

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

@@ -56,6 +59,24 @@ use tracing::{
warn,
};

#[async_trait::async_trait]
pub trait BlockProducer: Send + Sync {
Copy link
Member

Choose a reason for hiding this comment

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

[nit] consider moving to a crate-root ports module

Voxelot
Voxelot previously approved these changes Dec 5, 2022
Copy link
Member

@Voxelot Voxelot left a comment

Choose a reason for hiding this comment

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

overall looks good to me, just a couple nits about using consistent ports and adapters nomenclature for organizing these traits.

@xgreenx xgreenx merged commit d731735 into master Dec 6, 2022
@xgreenx xgreenx deleted the feature/block-producer-adapter branch December 6, 2022 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Something related to the architecture or the architecture description itself. breaking A breaking api change fuel-block-executor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants