chore(rpc): remove use of extensible transaction + receipt types #9774

emhane · 2024-07-24T16:36:07Z

Removes optimism feature from reth-rpc-types-compat
Moves reth-rpc-types-compat transaction type conversion, which was previously optimism-feature gated, into new trait TransactionCompat. These are called many times during the program, hence are not used as trait objects.
Adds EthApiTypes::TransactionCompat, allowing different networks to use same transaction type but with different reth compatibility methods.
Adding EthApiTypes::TransactionCompat, allows for implementation of conversion reth types -> op-alloy-rpc-types to be implemented in reth repo as opposed to op-alloy repo.
Lifts concrete type constraint off RPC transaction type.
Lifts concrete type constraint off RPC receipt type.

Alt. solution:
alloy_network::Network associated types must provide reth compatibility via TransactionResponse, ReceiptResponse, etc. traits, then no additional associated type EthApiTypes::TransactionCompat is needed: alloy-rs/alloy#1173.

…m EthApiError

mattsse

+1 on the direction

crates/optimism/primitives/src/types.rs

crates/rpc/rpc-eth-api/src/core.rs

crates/rpc/rpc/src/eth/core.rs

crates/optimism/cli/src/commands/import_receipts.rs

…tion

emhane · 2024-09-18T19:50:14Z

equivalent to main in hive tests https://github.com/paradigmxyz/reth/actions/runs/10928588345/job/30337917567

mattsse

ty for suffering through this.

I get now why we need the compat trait, although I believe once we have primitive traits we can simplify a few things here. I'm fine with this trait since this unlocks a few things and further separates op from eth.

only have a few questions

mattsse · 2024-09-19T12:58:03Z

crates/node/builder/src/launch/mod.rs

@@ -54,13 +51,14 @@ use crate::{
 };

 /// Alias for [`reth_rpc_eth_types::EthApiBuilderCtx`], adapter for [`FullNodeComponents`].
-pub type EthApiBuilderCtx<N> = reth_rpc_eth_types::EthApiBuilderCtx<
+pub type EthApiBuilderCtx<N, Eth> = reth_rpc_eth_types::EthApiBuilderCtx<
    <N as FullNodeTypes>::Provider,
    <N as FullNodeComponents>::Pool,
    <N as FullNodeComponents>::Evm,
    <N as FullNodeComponents>::Network,
    TaskExecutor,


I'd actually like to remove the Tasks generic everywhere and use Box dyn (followup)

it's an unnecessary perf downgrade to use dynamic dispatch for types that are called (i) often and (ii) always return a trait object originating from the same type. I'd try to reserve dynamic dispatch for builders that are called once. or where trait objects are returned that originate from different types for each call.

mattsse · 2024-09-19T13:09:09Z

crates/rpc/rpc-types-compat/src/transaction/mod.rs

-    let signer = tx.signer();
-    let signed_tx = tx.into_signed();
+/// Builds RPC transaction w.r.t. network.
+pub trait TransactionCompat: Send + Sync + Unpin + Clone + fmt::Debug {


I get why we currently need this.

but I worry that this could cause more trouble once we have network specific transaction types.

this is also not stateful, so these conversions are limited to the function input

yes agreed, this is a first step, hence parallel work on alloy-rs/alloy#1173, alloy-rs/alloy#1315

mattsse · 2024-09-19T13:13:46Z

crates/rpc/rpc-types-compat/src/transaction/mod.rs

+    /// Truncates the input of a transaction to only the first 4 bytes.
+    fn otterscan_api_truncate_input(tx: &mut Self::Transaction);


I find this a bit weird, this could also be solved by introducing a trait for the the transaction and not baking this into the compat trait.

yes absolutely, will open an issue, not to block anymore for alloy changes here. asides merge conflicts getting out of hand, I don't want to block this any longer since sdk work from arseni is moving fast and I'm uncertain if it's taking consideration of changes in this pr. I want to avoid anymore collisions by getting this in as is.

mattsse · 2024-09-19T13:14:14Z

crates/rpc/rpc-types-compat/src/transaction/mod.rs

-        other: Default::default(),
+    /// Create a new rpc transaction result for a _pending_ signed transaction, setting block
+    /// environment related fields to `None`.
+    fn fill(tx: TransactionSignedEcRecovered, tx_inf: TransactionInfo) -> Self::Transaction;


I also think this would go away once we have network specific primitives

again ref problem described here alloy-rs/alloy#1315

mattsse · 2024-09-19T13:16:21Z

crates/optimism/rpc/src/eth/transaction.rs

+            source_hash: signed_tx.source_hash(),
+            mint: signed_tx.mint(),
+            is_system_tx: signed_tx.is_deposit().then_some(signed_tx.is_system_transaction()),
+            deposit_receipt_version: None, // todo: how to fill this field?


how are we currently filling this?

also with None, the field feels a bit off on the tx type, or we need to figure our how to set it, as sep issue

reth/crates/rpc/rpc-types-compat/src/transaction/mod.rs

Lines 101 to 111 in c3d090a

// Optimism fields

#[cfg(feature = "optimism")]

other: reth_rpc_types::optimism::OptimismTransactionFields {

source_hash: signed_tx.source_hash(),

mint: signed_tx.mint(),

// only include is_system_tx if true: <https://github.com/ethereum-optimism/op-geth/blob/641e996a2dcf1f81bac9416cb6124f86a69f1de7/internal/ethapi/api.go#L1518-L1518>

is_system_tx: (signed_tx.is_deposit() && signed_tx.is_system_transaction())

.then_some(true),

deposit_receipt_version: None,

}

.into(),

crates/rpc/rpc-eth-types/Cargo.toml

mattsse · 2024-09-19T13:24:13Z

crates/rpc/rpc/src/eth/helpers/types.rs

+            signed_tx.chain_id(),
+        );
+
+        WithOtherFields {


do we still nicht WithOtherFields here?

when I implemented this is it wasn't possible to use Ethereum instead of AnyNetwork, may have changed now, though appreciate if this can be tackled in a follow pr, will open issue

mattsse

cool,
this gets us at least closer to where we want to go, the transactioncompat is necessary, tmp weirdness that will improve shortly

mattsse · 2024-09-20T06:22:01Z

crates/node/builder/src/rpc.rs

 pub trait EthApiBuilderProvider<N: FullNodeComponents>: BuilderProvider<N> + EthApiTypes {
    /// Returns the eth api builder.
    #[allow(clippy::type_complexity)]
-    fn eth_api_builder() -> Box<dyn Fn(&EthApiBuilderCtx<N>) -> Self + Send>;
+    fn eth_api_builder() -> Box<dyn Fn(&EthApiBuilderCtx<N, Self>) -> Self + Send>;
 }


I assume this will go away, if we do #10769 ?

this trait is very useful to avoid the very long generics that node builder has now, ref #9543

emhane added 9 commits July 17, 2024 15:22

Add EthApiTypes with error as AT and helper traits for conversion fro…

64777ff

…m EthApiError

Make it possible to reference EthApiTypes via EthApiServer helper traits

2f9b4d4

Fix rpc API impls

494b3d9

Merge branch 'main' into emhane/rpc-error

e05fd6f

Fix OpEthApi impl

916e509

Merge branch 'main' into emhane/rpc-error

f1d02a7

Merge branch 'main' into emhane/rpc-error

09a4192

Extend EthApiTypes with alloy_network::Network AT

20fae32

Merge branch 'emhane/rpc-error' into emhane/ethapi-types

ade6b8f

emhane added C-debt Refactor of code section that is hard to understand or maintain A-rpc Related to the RPC implementation labels Jul 24, 2024

emhane requested review from mattsse, Rjected, fgimenez and gakonst as code owners July 24, 2024 16:36

emhane marked this pull request as draft July 24, 2024 16:36

mattsse requested changes Jul 24, 2024

View reviewed changes

crates/optimism/primitives/src/types.rs Outdated Show resolved Hide resolved

crates/rpc/rpc-eth-api/src/core.rs Outdated Show resolved Hide resolved

Base automatically changed from emhane/rpc-error to main July 25, 2024 17:43

emhane added 5 commits July 26, 2024 21:52

Merge branch 'main' into emhane/ethapi-types

8a631dc

Import OP rpc types from op-alloy-network

83bd53b

Rename EthApiTypes::Types to EthApiTypes::NetworkTypes

ad29246

Remove optimism feature from reth-rpc-types-compat lib

6bd0f60

Incorporate TransactionBuilder into EthTransactions

e248a65

emhane added the A-op-reth Related to Optimism and op-reth label Jul 26, 2024

fgimenez reviewed Jul 27, 2024

View reviewed changes

crates/rpc/rpc/src/eth/core.rs Outdated Show resolved Hide resolved

crates/rpc/rpc/src/eth/core.rs Outdated Show resolved Hide resolved

crates/optimism/cli/src/commands/import_receipts.rs Outdated Show resolved Hide resolved

emhane added 5 commits July 27, 2024 13:12

Revert unrelated change

624e938

Fix conflicts from making rpc comapt generic over network for transac…

91ace3a

…tion

Add trait for building rpc block response

c506e8e

Add adapter for network type builders to EthApi and OpEthApi

0860e1c

Fix conflicts, resp ty has non-unique builder

f48038e

emhane added 2 commits September 16, 2024 22:29

Fix lint

a430d61

Remove completed todos

e51f52a

emhane requested a review from mattsse September 16, 2024 20:47

Revert unrelated changes

67d4ba8

emhane requested review from rkrasiuk, shekhirin, fgimenez, jenpaff, clabby and DaniPopes September 16, 2024 21:21

emhane added 2 commits September 18, 2024 15:37

Merge branch 'main' into emhane/ethapi-types

a3c203b

Merge branch 'main' into emhane/ethapi-types

d24e7f5

mattsse requested changes Sep 19, 2024

View reviewed changes

emhane added 6 commits September 19, 2024 18:29

Update docs

9ccf619

Add lost code comment

dc7d4d1

Add todo

a625312

Merge branch 'main' into emhane/ethapi-types

5555173

Fix merge conflicts

614cb7f

Fix merge conflicts

2c6bb1d

emhane requested a review from mattsse September 19, 2024 17:51

mattsse approved these changes Sep 20, 2024

View reviewed changes

Merge branch 'main' into emhane/ethapi-types

16dc6df

emhane enabled auto-merge September 20, 2024 14:47

emhane mentioned this pull request Sep 20, 2024

rpc: add ExtendedTransaction type alloy-rs/alloy#1317

Closed

emhane added this pull request to the merge queue Sep 20, 2024

Merged via the queue into main with commit b5adf24 Sep 20, 2024
37 checks passed

emhane deleted the emhane/ethapi-types branch September 20, 2024 15:27

emhane mentioned this pull request Sep 20, 2024

Replace reth-provider dep in reth-rpc-eth-types #11077

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(rpc): remove use of extensible transaction + receipt types #9774

chore(rpc): remove use of extensible transaction + receipt types #9774

emhane commented Jul 24, 2024 •

edited

Loading

mattsse left a comment

emhane commented Sep 18, 2024

mattsse left a comment

mattsse Sep 19, 2024

emhane Sep 19, 2024

mattsse Sep 19, 2024

emhane Sep 19, 2024

mattsse Sep 19, 2024

emhane Sep 19, 2024

mattsse Sep 19, 2024

emhane Sep 19, 2024

mattsse Sep 19, 2024

emhane Sep 19, 2024

mattsse Sep 19, 2024

emhane Sep 19, 2024

mattsse left a comment

mattsse Sep 20, 2024

emhane Sep 20, 2024

		/// Truncates the input of a transaction to only the first 4 bytes.
		fn otterscan_api_truncate_input(tx: &mut Self::Transaction);

	// Optimism fields
	#[cfg(feature = "optimism")]
	other: reth_rpc_types::optimism::OptimismTransactionFields {
	source_hash: signed_tx.source_hash(),
	mint: signed_tx.mint(),
	// only include is_system_tx if true: <https://github.com/ethereum-optimism/op-geth/blob/641e996a2dcf1f81bac9416cb6124f86a69f1de7/internal/ethapi/api.go#L1518-L1518>
	is_system_tx: (signed_tx.is_deposit() && signed_tx.is_system_transaction())
	.then_some(true),
	deposit_receipt_version: None,
	}
	.into(),

chore(rpc): remove use of extensible transaction + receipt types #9774

chore(rpc): remove use of extensible transaction + receipt types #9774

Conversation

emhane commented Jul 24, 2024 • edited Loading

mattsse left a comment

Choose a reason for hiding this comment

emhane commented Sep 18, 2024

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emhane commented Jul 24, 2024 •

edited

Loading