Skip to content

refactor: move ExtendedTxEnvelope to reth-primitives-traits #16102

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

Merged
merged 7 commits into from
May 8, 2025

Conversation

iTranscend
Copy link
Contributor

This PR moves ExtendedTxEnvelope and its impls to the reth-primitives-traits crate

resolves #16091

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, only have feature nits

Comment on lines 44 to 46
/// A [`SignedTransaction`] implementation that combines the [`OpTxEnvelope`] and another
/// transaction type.
pub type ExtendedOpTxEnvelope<T> = ExtendedTxEnvelope<OpTxEnvelope, T>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this alias we can remove

use revm_primitives::{Address, Bytes, TxKind, B256, U256};

use crate::{
serde_bincode_compat::SerdeBincodeCompat,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this impl we need to feature gate, with

serde-bincode-compat = [

@@ -9,12 +9,14 @@ use alloy_primitives::{bytes::Buf, ChainId, TxHash};
use alloy_rlp::{BufMut, Decodable, Encodable, Result as RlpResult};
use op_alloy_consensus::OpTxEnvelope;
use reth_codecs::Compact;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we also need to feature gate this impl with reth-codecs

@@ -33,10 +35,14 @@ macro_rules! delegate {
/// types must be unique.
#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, Hash, Eq, PartialEq)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

we also need to feature gate serde, see other types in this crate for reference

@github-project-automation github-project-automation bot moved this from Backlog to In Progress in Reth Tracker May 7, 2025
@mattsse mattsse added C-enhancement New feature or request A-sdk Related to reth's use as a library labels May 8, 2025
@fgimenez fgimenez force-pushed the feat/move-ext-tx-envelope branch from a718c7e to fd97853 Compare May 8, 2025 15:27
Comment on lines +42 to +43
impl<Tx> TryFrom<ExtendedTxEnvelope<OpTxEnvelope, Tx>>
for ExtendedTxEnvelope<OpPooledTransaction, Tx>
Copy link
Collaborator

Choose a reason for hiding this comment

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

we either need to relax this so that this works for any Builtin type or feature gat behind op feature

}
}
}

impl From<OpPooledTransaction> for ExtendedTxEnvelope<OpTxEnvelope, CustomTransactionEnvelope> {
impl<Tx> From<OpPooledTransaction> for ExtendedTxEnvelope<OpTxEnvelope, Tx> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this also needs an op feature

{
fn to_compact<Buf>(&self, buf: &mut Buf) -> usize
#[cfg(feature = "reth-codec")]
mod compact {
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 like to avoid this compact module and use full paths instead, this is how we do compact feature gate elsewhere

@mattsse mattsse added this pull request to the merge queue May 8, 2025
Merged via the queue into paradigmxyz:main with commit 124bd39 May 8, 2025
45 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Reth Tracker May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sdk Related to reth's use as a library C-enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Move ExtendedTxEnvelope and impls to reth-primitives-traits
3 participants