remove transaction forwarder trait

[nkysg]

Closes #9562

[mattsse]

awesome
some design suggestion,

needs input from @emhane on how to include the sequencer URL in the nodeaddon / op-ethapi builder

[emhane]

not sure if we can remove the trait UpdateTxForwarder since the type of the field on OpEthApi is a generic, however we could remove the trait as super trait from FullEthApiServer and just specify it as trait bound for the impl of BuilderProvider for OpEthApi.

[mattsse]

I see,

@nkysg I'll try something with this to get this unblocked and will get back to you

[nkysg]

I have tried add pub ext_args: Addons::ExtArgs in pub struct Addons

pub struct AddOns<Node: FullNodeComponents, AddOns: NodeAddOns<Node>> {
    /// Additional `NodeHooks` that are called at specific points in the node's launch lifecycle.
    pub hooks: NodeHooks<Node, AddOns>,
    /// The `ExExs` (execution extensions) of the node.
    pub exexs: Vec<(String, Box<dyn BoxedLaunchExEx<Node>>)>,
    /// Additional RPC add-ons.
    pub rpc: RpcAddOns<Node, AddOns::EthApi>,
    /// Additional node add-ons args.
    + pub ext_args: AddOns::ExtArgs,
}

pub trait NodeAddOns<N: FullNodeComponents>: Send + Sync + Unpin + Clone + 'static {
    /// The core `eth` namespace API type to install on the RPC server (see
    /// `reth_rpc_eth_api::EthApiServer`).
    type EthApi: Send + Clone;
    /// The extent arguments required for the node add-ons.
   + type ExtArgs: Send + Clone + Default;
}

impl<N: FullNodeComponents> NodeAddOns<N> for OptimismAddOns {
    type EthApi = OpEthApi<N>;
   + type ExtArgs = Option<String>;
}

And I have defined func update_ext_args in WithLaunchContext and NodeBuilderWithComponents

impl<T, CB, AO> WithLaunchContext<NodeBuilderWithComponents<T, CB, AO>>
where
    T: FullNodeTypes,
    CB: NodeComponentsBuilder<T>,
    AO: NodeAddOns<NodeAdapter<T, CB::Components>>,
    AO::EthApi: FullEthApiServer + AddDevSigners,
{
   
    /// Update Addons extent args
    pub fn update_ext_args(self, args: AO::ExtArgs) -> Self {
        Self { builder: self.builder.update_ext_args(args), task_executor: self.task_executor }
    }
}

impl<T, CB, AO> NodeBuilderWithComponents<T, CB, AO>
where
    T: FullNodeTypes,
    CB: NodeComponentsBuilder<T>,
    AO: NodeAddOns<NodeAdapter<T, CB::Components>>,
{
          /// update the  AddOns extent args
    pub fn update_ext_args(mut self, args: AO::ExtArgs) -> Self {
        self.add_ons.ext_args = args;
        self
    }
}

right now I need to process the ext_args into EngineApi::new,

 async fn launch_node(
        self,
        target: NodeBuilderWithComponents<T, CB, AO>,
    ) -> eyre::Result<Self::Node> {
       let NodeBuilderWithComponents {
            adapter: NodeTypesAdapter { database },
            components_builder,
            add_ons: AddOns { hooks, rpc, exexs: installed_exex, ext_args: ext_args },
            config,
        } = target;

....
        
        let engine_api = EngineApi::new(
            ctx.blockchain_db().clone(),
            ctx.chain_spec(),
            beacon_engine_handle,
            ctx.components().payload_builder().clone().into(),
            Box::new(ctx.task_executor().clone()),
            client,
            EngineCapabilities::default(),
        );

one q, How to pass the ext_args and then I can call below func as your mentioned

reth/crates/optimism/rpc/src/eth/mod.rs

Lines 176 to 180 in 5a9de03

	type Ctx<'a> = <Eth as BuilderProvider<N>>::Ctx<'a>;
	fn builder() -> Box<dyn for<'a> Fn(Self::Ctx<'a>) -> Self + Send> {
	Box::new(|ctx| Self { inner: Eth::builder()(ctx) })
	}

@mattsse please take look at it. Thx

[emhane]

one q, How to pass the ext_args and then I can call below func as your mentioned

you need to add the EngineApi type to be built as AT to NodeAddOns. It then has its own builder, with its own context type. Let the NodeAddOns::EngineApi provide its args instead. so remove the AT NodeAddOns::ExtArgs.

[mattsse]

sorry @nkysg I didn't find capacity just yet,

bumping prio and will look into how we should do this, in the meantime you could try proceeding with #10028

[nkysg]

@mattsse Thx. I will try proceeding with #10028

[nkysg]

one q, How to pass the ext_args and then I can call below func as your mentioned

you need to add the EngineApi type to be built as AT to NodeAddOns. It then has its own builder, with its own context type. Let the NodeAddOns::EngineApi provide its args instead. so remove the AT NodeAddOns::ExtArgs.

Thanks for your guiding. After #10028, I will try this.

[emhane]

on second look, addressing issue #9562 shouldn't have to touch EngineApi. we can make a new type OpEthApiBuilderCtx, which wraps the EthApiBuilderCtx and the sequencer url string. then the tx forwarder can be set in the OpEthApi builder function, this is possible since #9879. we don't need to touch NodeAddOns.

[nkysg]

Got it. I will try this.

[emhane]

actually @nkysg, there should be no need for an OpEthApiBuilderCtx type for now. just hard code the sequencer url in the OpEthApi builder function for this pr.

[nkysg]

@emhane I have this mind. Let's do it.

[emhane]

check it out @nkysg, at this point, it should be known that the eth api type returned by RpcRegistryInner::eth_api is OpEthApi,

reth/bin/reth/src/optimism.rs

Lines 38 to 47 in 8a802da

	.extend_rpc_modules(move |ctx| {
	// register sequencer tx forwarder
	if let Some(sequencer_http) = sequencer_http_arg {
	ctx.registry.set_eth_raw_transaction_forwarder(Arc::new(
	SequencerClient::new(sequencer_http),
	));
	}
	Ok(())
	})

so we should be able to add method OpEthApi::set_eth_raw_transaction_forwarder, and do this here

// register sequencer tx forwarder
if let Some(sequencer_http) = sequencer_http_arg {
    ctx.registry.eth_api().set_eth_raw_transaction_forwarder(Arc::new(
         SequencerClient::new(sequencer_http),
     ));
}

[emhane]

need any help here @nkysg ?

[nkysg]

@emhane Thanks for your help. I need to look into it again. Something I forget it.

[nkysg]

so we should be able to add method OpEthApi::set_eth_raw_transaction_forwarder, and do this here

// register sequencer tx forwarder
if let Some(sequencer_http) = sequencer_http_arg {
    ctx.registry.eth_api().set_eth_raw_transaction_forwarder(Arc::new(
         SequencerClient::new(sequencer_http),
     ));
}

I don't understand it. At #9562, Want to remove the two traits RawTransactionForwarder and UpdateRawTxForwarder @emhane

[emhane]

you're doing great!

[emhane]

killing it!

[emhane]

wonder if this code can be deleted for op? I'm aware it is executed also for op on main...will open issue for it though

addressed

[nkysg]

I see, we can unassigned mattse. @emhane .Thx.