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

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

Merged
merged 114 commits into from
Sep 20, 2024

Conversation

emhane
Copy link
Member

@emhane emhane commented Jul 24, 2024

Ref #8780. Closes #8988. Closes #10632.

  • 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.

@emhane 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 emhane marked this pull request as draft July 24, 2024 16:36
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

+1 on the direction

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 emhane added the A-op-reth Related to Optimism and op-reth label Jul 26, 2024
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
Copy link
Member Author

emhane commented Sep 18, 2024

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

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

@@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

crates/rpc/rpc-types-compat/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/optimism/rpc/src/eth/transaction.rs Show resolved Hide resolved
crates/rpc/rpc-eth-types/Cargo.toml Show resolved Hide resolved
crates/rpc/rpc/src/eth/helpers/types.rs Show resolved Hide resolved
@emhane emhane requested a review from mattsse September 19, 2024 17:51
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

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

crates/node/builder/src/rpc.rs Show resolved Hide resolved
@emhane 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 emhane deleted the emhane/ethapi-types branch September 20, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-reth Related to Optimism and op-reth A-rpc Related to the RPC implementation A-sdk Related to reth's use as a library C-debt Refactor of code section that is hard to understand or maintain
Projects
None yet
3 participants