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

feat: pool server abstraction and local server #268

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

dancoombs
Copy link
Collaborator

@dancoombs dancoombs commented Jul 21, 2023

Closes #85

src/builder/task.rs Outdated Show resolved Hide resolved
src/cli/node.rs Outdated Show resolved Hide resolved
src/common/grpc/mocks.rs Outdated Show resolved Hide resolved
@dancoombs dancoombs force-pushed the danc/local-server branch 3 times, most recently from 8f18040 to 92f591b Compare July 25, 2023 22:53
@dancoombs dancoombs changed the title feat: WIP add pool server abstraction feat: pool server abstraction and local server Jul 25, 2023
@dancoombs dancoombs marked this pull request as ready for review July 25, 2023 22:53
@@ -39,7 +46,7 @@ pub trait Mempool: Send + Sync + 'static {
) -> MempoolResult<H256>;

/// Removes a set of operations from the pool.
fn remove_operations<'a>(&self, hashes: impl IntoIterator<Item = &'a H256>);
fn remove_operations(&self, hashes: &[H256]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

automock doesn't handle impl Trait 😢

Base automatically changed from danc/builder-server to feat/local-server July 28, 2023 18:39
@@ -171,6 +178,102 @@ impl PoolOperation {
}
}

pub struct MempoolGroup<M> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New class

#[derive(Debug)]
pub struct Settings {
pub replacement_fee_percent_increase: u64,
pub max_fee_increases: u64,
}

#[derive(Debug)]
pub struct BundleSender<P, E, T>
pub struct BundleSenderImpl<P, E, T, C>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sender is now generic on PoolClient

@@ -66,13 +65,14 @@ pub trait BundleProposer: Send + Sync + 'static {
}

#[derive(Debug)]
pub struct BundleProposerImpl<S, E, P>
pub struct BundleProposerImpl<S, E, P, C>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Proposer is now generic on PoolClient

};

#[derive(Clone, Debug)]
pub struct LocalPoolClient {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New class

};

#[derive(Clone, Debug)]
pub struct RemotePoolClient {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New class

Ok(handle)
}

pub struct LocalPoolServer<M> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New class

@@ -0,0 +1,210 @@
use anyhow::{bail, Context};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is all just moved out of common

@@ -0,0 +1,317 @@
use std::{collections::HashMap, net::SocketAddr, sync::Arc};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code is all moved

@@ -46,16 +43,13 @@ pub trait DebugApi {
async fn bundler_dump_reputation(&self, entry_point: Address) -> RpcResult<Vec<RpcReputation>>;
}

pub struct DebugApi {
op_pool_client: op_pool_client::OpPoolClient<Channel>,
pub struct DebugApi<P> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now generic on PoolClient

@@ -98,23 +97,24 @@ where
}

#[derive(Debug)]
pub struct EthApi<C: JsonRpcClient + 'static> {
pub struct EthApi<C: JsonRpcClient + 'static, P: PoolClient> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now generic on pool client

Copy link
Collaborator

@dphilipson dphilipson left a comment

Choose a reason for hiding this comment

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

Nice work! Some initial thoughts, will look more in a bit.

src/builder/bundle_sender.rs Show resolved Hide resolved
src/op_pool/mempool/mod.rs Show resolved Hide resolved
src/op_pool/mempool/mod.rs Outdated Show resolved Hide resolved
src/op_pool/mempool/mod.rs Show resolved Hide resolved
src/op_pool/server/local/server.rs Show resolved Hide resolved
src/op_pool/server/remote/server.rs Show resolved Hide resolved
let _builder_loop_guard = {
SpawnGuard::spawn_with_guard(async move { bundle_sender.send_bundles_in_loop().await })
let mut builder: Box<dyn BundleSender> = match &self.args.pool_client_mode {
PoolClientMode::Local { sender } => {
Copy link
Collaborator

@dphilipson dphilipson Jul 31, 2023

Choose a reason for hiding this comment

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

The only difference between the two blocks here is pool_client, right? It's a bit hard to tell visually, unfortunately.

Something I'd like to see is if we can just box the pool client and have an impl

impl PoolClient for Box<dyn PoolClient>

That way we could have the branch just give us a Box<dyn PoolClient> and have all the rest of the config written once outside of the branch. The auto_impl crate might help with this.

But these trait impl macros tend to not work for us, so maybe it won't help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I actually played with this code for a while trying to make this cleaner. auto_impl and async_trait don't seem to work well together.

Copy link
Collaborator

@dphilipson dphilipson left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@dancoombs dancoombs merged commit 08ab644 into feat/local-server Aug 1, 2023
@dancoombs dancoombs deleted the danc/local-server branch August 1, 2023 00:00
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.

2 participants