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: make transaction type fields private #13915

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/reth/src/commands/debug_cmd/build_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ impl<C: ChainSpecParser<ChainSpec = ChainSpec>> Command<C> {
.try_ecrecovered()
.ok_or_else(|| eyre::eyre!("failed to recover tx"))?;

let encoded_length = match &transaction.transaction {
let encoded_length = match &transaction.transaction() {
Transaction::Eip4844(TxEip4844 { blob_versioned_hashes, .. }) => {
let blobs_bundle = blobs_bundle.as_mut().ok_or_else(|| {
eyre::eyre!("encountered a blob tx. `--blobs-bundle-path` must be provided")
Expand Down
2 changes: 1 addition & 1 deletion crates/ethereum/payload/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ where
// invalid, which removes its dependent transactions from
// the iterator. This is similar to the gas limit condition
// for regular transactions above.
trace!(target: "payload_builder", tx=?tx.hash, ?sum_blob_gas_used, ?tx_blob_gas, "skipping blob transaction because it would exceed the max data gas per block");
trace!(target: "payload_builder", tx=?tx.hash(), ?sum_blob_gas_used, ?tx_blob_gas, "skipping blob transaction because it would exceed the max data gas per block");
best_txs.mark_invalid(
&pool_tx,
InvalidPoolTransactionError::ExceedsGasLimit(
Expand Down
3 changes: 2 additions & 1 deletion crates/ethereum/primitives/src/alloy_compat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ impl TryFrom<AnyRpcTransaction> for TransactionSigned {
_ => return Err(ConversionError::Custom("unknown transaction type".to_string())),
};

Ok(Self::new(transaction, signature, hash))
let transaction_signed = Self::new(transaction, signature, hash);
Ok(transaction_signed)
Comment on lines +41 to +42
Copy link
Collaborator

Choose a reason for hiding this comment

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

this didn't require a change

}
}
27 changes: 24 additions & 3 deletions crates/ethereum/primitives/src/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,13 +286,13 @@ impl From<TypedTransaction> for Transaction {
pub struct TransactionSigned {
/// Transaction hash
#[serde(skip)]
pub hash: OnceLock<TxHash>,
hash: OnceLock<TxHash>,
/// The transaction signature values
pub signature: Signature,
signature: Signature,
/// Raw transaction info
#[deref]
#[as_ref]
pub transaction: Transaction,
transaction: Transaction,
}

impl Default for TransactionSigned {
Expand Down Expand Up @@ -328,6 +328,21 @@ impl TransactionSigned {
Self { hash: hash.into(), signature, transaction }
}

/// Returns the transaction.
pub const fn transaction(&self) -> &Transaction {
&self.transaction
}

/// Returns the transaction hash.
pub fn hash(&self) -> &B256 {
self.hash.get_or_init(|| self.recalculate_hash())
}

/// Returns the transaction signature.
pub const fn signature(&self) -> &Signature {
&self.signature
}

/// Creates a new signed transaction from the given transaction and signature without the hash.
///
/// Note: this only calculates the hash on the first [`TransactionSigned::hash`] call.
Expand Down Expand Up @@ -369,6 +384,12 @@ impl TransactionSigned {
}
}

impl core::ops::DerefMut for TransactionSigned {
fn deref_mut(&mut self) -> &mut Transaction {
&mut self.transaction
}
}

impl Typed2718 for TransactionSigned {
fn ty(&self) -> u8 {
self.transaction.ty()
Expand Down
3 changes: 2 additions & 1 deletion crates/rpc/rpc/src/eth/helpers/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ where
) -> Result<Self::Transaction, Self::Error> {
let from = tx.signer();
let hash = *tx.tx_hash();
let TransactionSigned { transaction, signature, .. } = tx.into_tx();
let transaction = tx.transaction().clone();
let signature = *tx.signature();

let inner: TxEnvelope = match transaction {
reth_primitives::Transaction::Legacy(tx) => {
Expand Down
5 changes: 4 additions & 1 deletion crates/stages/stages/src/stages/sender_recovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,10 @@ mod tests {
let transaction: TransactionSigned = provider
.transaction_by_id_unhashed(tx_id)?
.map(|tx| {
TransactionSigned::new_unhashed(tx.transaction, tx.signature)
TransactionSigned::new_unhashed(
tx.transaction().clone(),
*tx.signature(),
Comment on lines +676 to +678
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks odd, why does this require a conversion at all,

in any case we need decomposition fns like

/// Splits the block into its header and body.
fn split(self) -> (Self::Header, Self::Body);

)
})
.expect("no transaction entry");
let signer =
Expand Down
2 changes: 1 addition & 1 deletion crates/storage/provider/src/providers/static_file/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ mod tests {
writer.append_receipt(*next_tx_num, &receipt).unwrap();
} else {
// Used as ID for validation
tx.transaction.set_nonce(*next_tx_num);
(*tx).set_nonce(*next_tx_num);
writer.append_transaction(*next_tx_num, &tx).unwrap();
}
*next_tx_num += 1;
Expand Down
100 changes: 50 additions & 50 deletions crates/transaction-pool/src/test_utils/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -913,7 +913,7 @@ impl TryFrom<Recovered<TransactionSigned>> for MockTransaction {
let size = transaction.size();

#[allow(unreachable_patterns)]
match transaction.transaction {
match transaction.transaction() {
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 want fn into_transaction then these changes aren't necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should I change this one

    pub const fn transaction(&self) -> &Transaction {
        &self.transaction
    }

with this one?

    pub fn into_transaction(self) -> Transaction {
        self.transaction
    }

Transaction::Legacy(TxLegacy {
chain_id,
nonce,
Expand All @@ -923,17 +923,17 @@ impl TryFrom<Recovered<TransactionSigned>> for MockTransaction {
value,
input,
}) => Ok(Self::Legacy {
chain_id,
chain_id: *chain_id,
hash,
sender,
nonce,
gas_price,
gas_limit,
to,
value,
input,
nonce: *nonce,
gas_price: *gas_price,
gas_limit: *gas_limit,
to: *to,
value: *value,
input: input.clone(),
size,
cost: U256::from(gas_limit) * U256::from(gas_price) + value,
cost: U256::from(*gas_limit) * U256::from(*gas_price) + value,
}),
Transaction::Eip2930(TxEip2930 {
chain_id,
Expand All @@ -945,18 +945,18 @@ impl TryFrom<Recovered<TransactionSigned>> for MockTransaction {
input,
access_list,
}) => Ok(Self::Eip2930 {
chain_id,
chain_id: *chain_id,
hash,
sender,
nonce,
gas_price,
gas_limit,
to,
value,
input,
access_list,
nonce: *nonce,
gas_price: *gas_price,
gas_limit: *gas_limit,
to: *to,
value: *value,
input: input.clone(),
access_list: access_list.clone(),
size,
cost: U256::from(gas_limit) * U256::from(gas_price) + value,
cost: U256::from(*gas_limit) * U256::from(*gas_price) + value,
}),
Transaction::Eip1559(TxEip1559 {
chain_id,
Expand All @@ -969,19 +969,19 @@ impl TryFrom<Recovered<TransactionSigned>> for MockTransaction {
input,
access_list,
}) => Ok(Self::Eip1559 {
chain_id,
chain_id: *chain_id,
hash,
sender,
nonce,
max_fee_per_gas,
max_priority_fee_per_gas,
gas_limit,
to,
value,
input,
access_list,
nonce: *nonce,
max_fee_per_gas: *max_fee_per_gas,
max_priority_fee_per_gas: *max_priority_fee_per_gas,
gas_limit: *gas_limit,
to: *to,
value: *value,
input: input.clone(),
access_list: access_list.clone(),
size,
cost: U256::from(gas_limit) * U256::from(max_fee_per_gas) + value,
cost: U256::from(*gas_limit) * U256::from(*max_fee_per_gas) + value,
}),
Transaction::Eip4844(TxEip4844 {
chain_id,
Expand All @@ -996,21 +996,21 @@ impl TryFrom<Recovered<TransactionSigned>> for MockTransaction {
blob_versioned_hashes: _,
max_fee_per_blob_gas,
}) => Ok(Self::Eip4844 {
chain_id,
chain_id: *chain_id,
hash,
sender,
nonce,
max_fee_per_gas,
max_priority_fee_per_gas,
max_fee_per_blob_gas,
gas_limit,
to,
value,
input,
access_list,
nonce: *nonce,
max_fee_per_gas: *max_fee_per_gas,
max_priority_fee_per_gas: *max_priority_fee_per_gas,
max_fee_per_blob_gas: *max_fee_per_blob_gas,
gas_limit: *gas_limit,
to: *to,
value: *value,
input: input.clone(),
access_list: access_list.clone(),
sidecar: BlobTransactionSidecar::default(),
size,
cost: U256::from(gas_limit) * U256::from(max_fee_per_gas) + value,
cost: U256::from(*gas_limit) * U256::from(*max_fee_per_gas) + value,
}),
Transaction::Eip7702(TxEip7702 {
chain_id,
Expand All @@ -1024,20 +1024,20 @@ impl TryFrom<Recovered<TransactionSigned>> for MockTransaction {
authorization_list,
input,
}) => Ok(Self::Eip7702 {
chain_id,
chain_id: *chain_id,
hash,
sender,
nonce,
max_fee_per_gas,
max_priority_fee_per_gas,
gas_limit,
to,
value,
input,
access_list,
authorization_list,
nonce: *nonce,
max_fee_per_gas: *max_fee_per_gas,
max_priority_fee_per_gas: *max_priority_fee_per_gas,
gas_limit: *gas_limit,
to: *to,
value: *value,
input: input.clone(),
access_list: access_list.clone(),
authorization_list: authorization_list.clone(),
size,
cost: U256::from(gas_limit) * U256::from(max_fee_per_gas) + value,
cost: U256::from(*gas_limit) * U256::from(*max_fee_per_gas) + value,
}),
tx => Err(TryFromRecoveredTransactionError::UnsupportedTransactionType(tx.ty())),
}
Expand Down
12 changes: 6 additions & 6 deletions crates/transaction-pool/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1326,7 +1326,7 @@ impl PoolTransaction for EthPooledTransaction {
///
/// This is also commonly referred to as the "Gas Fee Cap" (`GasFeeCap`).
fn max_fee_per_gas(&self) -> u128 {
self.transaction.transaction.max_fee_per_gas()
self.transaction.transaction().max_fee_per_gas()
}

fn access_list(&self) -> Option<&AccessList> {
Expand All @@ -1337,7 +1337,7 @@ impl PoolTransaction for EthPooledTransaction {
///
/// This will return `None` for non-EIP1559 transactions
fn max_priority_fee_per_gas(&self) -> Option<u128> {
self.transaction.transaction.max_priority_fee_per_gas()
self.transaction.transaction().max_priority_fee_per_gas()
}

fn max_fee_per_blob_gas(&self) -> Option<u128> {
Expand Down Expand Up @@ -1375,7 +1375,7 @@ impl PoolTransaction for EthPooledTransaction {

/// Returns a measurement of the heap usage of this type and all its internals.
fn size(&self) -> usize {
self.transaction.transaction.input().len()
self.transaction.transaction().input().len()
}

/// Returns the transaction type
Expand Down Expand Up @@ -1404,7 +1404,7 @@ impl EthPoolTransaction for EthPooledTransaction {
}

fn blob_count(&self) -> usize {
match &self.transaction.transaction {
match &self.transaction.transaction() {
Transaction::Eip4844(tx) => tx.blob_versioned_hashes.len(),
_ => 0,
}
Expand Down Expand Up @@ -1437,14 +1437,14 @@ impl EthPoolTransaction for EthPooledTransaction {
sidecar: &BlobTransactionSidecar,
settings: &KzgSettings,
) -> Result<(), BlobTransactionValidationError> {
match &self.transaction.transaction {
match &self.transaction.transaction() {
Transaction::Eip4844(tx) => tx.validate_blob(sidecar, settings),
_ => Err(BlobTransactionValidationError::NotBlobTransaction(self.tx_type())),
}
}

fn authorization_count(&self) -> usize {
match &self.transaction.transaction {
match &self.transaction.transaction() {
Transaction::Eip7702(tx) => tx.authorization_list.len(),
_ => 0,
}
Expand Down
2 changes: 1 addition & 1 deletion testing/testing-utils/src/generators.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ pub fn random_block<R: Rng>(rng: &mut R, number: u64, block_params: BlockParams)
let tx_count = block_params.tx_count.unwrap_or_else(|| rng.gen::<u8>());
let transactions: Vec<TransactionSigned> =
(0..tx_count).map(|_| random_signed_tx(rng)).collect();
let total_gas = transactions.iter().fold(0, |sum, tx| sum + tx.transaction.gas_limit());
let total_gas = transactions.iter().fold(0, |sum, tx| sum + tx.transaction().gas_limit());

// Generate ommers
let ommers_count = block_params.ommers_count.unwrap_or_else(|| rng.gen_range(0..2));
Expand Down
Loading