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

TxRequest into EIP-4844 without sidecar #1093

Merged
Merged
Changes from 4 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
52 changes: 49 additions & 3 deletions crates/rpc-types-eth/src/transaction/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,51 @@ impl TransactionRequest {
}
}

/// Build an EIP-4844 transaction.
/// Build an EIP-4844 transaction variant - either with or without sidecar.
///
/// # Panics
///
/// If required fields are missing. Use `complete_4844` to check if the
/// request can be built.
fn build_4844_variant(self) -> TxEip4844Variant {
if self.sidecar.is_none() {
self.build_4844_without_sidecar().into()
} else {
self.build_4844().into()
}
}

/// Build an EIP-4844 transaction without sidecar.
///
/// # Panics
///
/// If required fields are missing. Use `complete_4844` to check if the
/// request can be built.
fn build_4844_without_sidecar(self) -> TxEip4844 {
Copy link
Member

Choose a reason for hiding this comment

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

this seems appropriate as an additional function

let checked_to = self.to.expect("checked in complete_4844.");
let to_address = match checked_to {
TxKind::Create => panic!("the field `to` can only be of type TxKind::Call(Account). Please change it accordingly."),
TxKind::Call(to) => to,
};

TxEip4844 {
chain_id: self.chain_id.unwrap_or(1),
nonce: self.nonce.expect("checked in complete_4844"),
gas_limit: self.gas.expect("checked in complete_4844"),
max_fee_per_gas: self.max_fee_per_gas.expect("checked in complete_4844"),
max_priority_fee_per_gas: self
.max_priority_fee_per_gas
.expect("checked in complete_4844"),
to: to_address,
value: self.value.unwrap_or_default(),
access_list: self.access_list.unwrap_or_default(),
blob_versioned_hashes: self.blob_versioned_hashes.expect("checked in complete_4844"),
max_fee_per_blob_gas: self.max_fee_per_blob_gas.expect("checked in complete_4844"),
input: self.input.into_input().unwrap_or_default(),
}
}

/// Build an EIP-4844 transaction with sidecar.
///
/// # Panics
///
Expand Down Expand Up @@ -390,8 +434,10 @@ impl TransactionRequest {
missing.push("to");
}

if self.sidecar.is_none() {
// Either `sidecar` or `blob_versioned_hashes` must be set.
if self.sidecar.is_none() && self.blob_versioned_hashes.is_none() {
missing.push("sidecar");
missing.push("blob_versioned_hashes");
Copy link
Member

Choose a reason for hiding this comment

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

this is a bit of a footgun, because this now also allows without sidecar which was not the original intention of this because this is supposed to be a transactionbuilder and 4844 without sidecar can't be sent.

I'd like to keep it that way but rather expose another transactionbuilder function explicitly for this usecase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted changes to the legacy, now it should be the same as before (instead of the panics).
I've left a TODO/question for you in the code, please let me know what you think.

Introduced a dedicated TryFrom to handle the conversion I need.

}

if self.max_fee_per_blob_gas.is_none() {
Expand Down Expand Up @@ -472,7 +518,7 @@ impl TransactionRequest {
TxType::Legacy => self.build_legacy().into(),
TxType::Eip2930 => self.build_2930().into(),
TxType::Eip1559 => self.build_1559().into(),
TxType::Eip4844 => self.build_4844().into(),
TxType::Eip4844 => self.build_4844_variant().into(),
})
}
}
Expand Down