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

refactor(ethexe): service based architecture #4456

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

Conversation

breathx
Copy link
Member

@breathx breathx commented Jan 20, 2025

No description provided.

@breathx breathx added the A0-pleasereview PR is ready to be reviewed by the team label Jan 20, 2025
@ark0f ark0f changed the title refactor(services-arch): #1 initial port of Observer and Network refactor(ethexe-service): Initial redesign of Observer and Network Jan 20, 2025
Copy link
Member

@techraed techraed left a comment

Choose a reason for hiding this comment

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

interim review

.gossipsub
.publish(gpu_commitments_topic(), data)
{
log::debug!("gossipsub publishing failed: {e}")
Copy link
Member

Choose a reason for hiding this comment

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

That must be log::error

Last time when semi-redundant delay was removed from the Node::start_service method here the test was failing and the reason for that was that node's networking service was trying to publish a message in a network with 2 nodes (the third one because of no delay didn't have enough time to start it's networking service). Having error log could make it really easy to locate an error

Comment on lines +15 to +16
ethexe-validator.workspace = true
ethexe-sequencer.workspace = true
Copy link
Member

Choose a reason for hiding this comment

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

how is that used?

type BlobDownloadFuture = BoxFuture<'static, Result<(CodeId, Vec<u8>)>>;

impl ObserverService {
pub async fn cloned(&self) -> Result<Self> {
Copy link
Member

Choose a reason for hiding this comment

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

can be just clone - that seems closer to the naming convetion, as cloned and copied is used for creating an owned version of some reference to the type

@breathx breathx changed the title refactor(ethexe-service): Initial redesign of Observer and Network refactor(ethexe): service based architecture Jan 23, 2025
@@ -34,3 +34,10 @@ pub const fn u64_into_uint48_be_bytes_lossy(val: u64) -> [u8; 6] {

[b1, b2, b3, b4, b5, b6]
}

pub fn option_call<S>(
Copy link
Member

Choose a reason for hiding this comment

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

mb make it as trait OptionExt { fn option_call(...) -> ... {} }
option_call(&mut sequencer, |s| s.on_new_head(hash))?;
vs
sequencer.as_mut().option_call(|s| s.on_new_head(hash))

};
Some(event) = observer.next() => {
match event? {
ObserverEvent::Block(block) => {

Choose a reason for hiding this comment

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

match in order of

pub enum ObserverEvent {
    Blob { code_id: CodeId, code: Vec<u8> },
    Block(BlockData),
}

sequencer.as_mut(),
network_sender.as_mut(),
);
SequencerEvent::CollectionRoundEnded { block_hash: _} => {

Choose a reason for hiding this comment

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

cargo fmt does not work for macros

Suggested change
SequencerEvent::CollectionRoundEnded { block_hash: _} => {
SequencerEvent::CollectionRoundEnded { block_hash: _ } => {

Choose a reason for hiding this comment

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

self.swarm.behaviour().peer_score.handle()
}

pub fn publish_message(&mut self, data: Vec<u8>) {

Choose a reason for hiding this comment

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

mb intoduce methods with impl Encode and Vec<u8>?

Suggested change
pub fn publish_message(&mut self, data: Vec<u8>) {
pub fn publish_message(&mut self, data: impl Encode) {
pub fn publish_message(&mut self, data: Vec<u8>)  {}
pub fn publish_network_message(&mut self, message: impl Encode)  {}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview PR is ready to be reviewed by the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants