fix!: use TransactionRequest in Network impl#525
Conversation
TransactionRequest in Network impl
mattsse
left a comment
There was a problem hiding this comment.
smol nits
I think this is reasonable, no reason to introduce a new txrequest type here really because most of the server apis are restricted to TransactionRequest, so even if this does allow configuring 4844 fields, this seems fine
* removes alloy-compat from default feature * revert ci change
crates/consensus/Cargo.toml
Outdated
| # compat | ||
| alloy-network = { workspace = true, optional = true } | ||
| alloy-rpc-types-eth = { workspace = true, optional = true } | ||
| alloy-rpc-types-eth.workspace = true |
There was a problem hiding this comment.
this should not be mandatory here
There was a problem hiding this comment.
This is due to the impl From for TransactionRequest conversion being moved to their respective files after removal of OpTransactionRequest.
These conversions are:
-
impl From<TxDeposit> for TransactionRequestin deposit.rs -
impl From<OpTxEnvelope> for TransactionRequestin envelope.rs -
impl From<OpTypedTransaction> for TransactionRequestin typed.rs
We need these now because of trait bounds on the Network::TransactionRequest associated type
There was a problem hiding this comment.
then this should be feature gated
There was a problem hiding this comment.
why do we need this conversion in this pr?
There was a problem hiding this comment.
It was implemeted for OpTransactionRequest:
After removing that, implemented similar conversion for TransactionRequest
crates/network/src/lib.rs
Outdated
| let tx = OpTypedTransaction::Eip1559(tx); | ||
| Ok(tx) | ||
| } | ||
| EthereumTypedTransaction::Eip4844(tx) => { |
There was a problem hiding this comment.
why do we have this here?
this isn't supported, unclear why we need to handle this case
There was a problem hiding this comment.
Can remove it. It is being handled on main. Just ported it here. See: #525 (comment)
It's equivalent to build_typed_tx currently on main https://github.com/alloy-rs/op-alloy/blob/main/crates/rpc-types/src/transaction/request.rs#L104-L117 which doesn't return an error.
crates/consensus/Cargo.toml
Outdated
| # compat | ||
| alloy-network = { workspace = true, optional = true } | ||
| alloy-rpc-types-eth = { workspace = true, optional = true } | ||
| alloy-rpc-types-eth.workspace = true |
There was a problem hiding this comment.
then this should be feature gated
|
fyi @RomanHodulak |
…ionRequest` of `Optimism` (#557) ## Motivation The change #525 introduced ambiguity of `alloy_network::TransactionBuilder` implementation for `TransactionRequest`, being implemented twice once for `Ethereum` and once for `Optimism`. This then requires fully qualified syntax to differentiate. ## Solution Bring back `OpTransactionRequest` ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes
<!-- Thank you for your Pull Request. Please provide a description above and review the requirements below. Bug fixes and new features should include tests. Contributors guide: https://github.com/alloy-rs/core/blob/main/CONTRIBUTING.md The contributors guide includes instructions for running rustfmt and building the documentation. --> <!-- ** Please select "Allow edits from maintainers" in the PR Options ** --> ## Motivation Ref alloy-rs/alloy#2478 `OpTransactionRequest` is a wrapper around `TransactionRequest`. It doesn't implement `TransactionBuilder4844` (It shouldn't). But this makes it incompatible with `ProviderBuilder` default constructor `new` <!-- Explain the context and why you're making that change. What is the problem you're trying to solve? In some cases there is not a problem and this can be thought of as being the motivation for your change. --> ## Solution - Remove OpTransactionRequest - Move related `From` conversions - Use TransactionRequest in network impl <!-- Summarize the solution and provide any necessary context needed to understand the code change. --> ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes --------- Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Motivation
Ref alloy-rs/alloy#2478
OpTransactionRequestis a wrapper aroundTransactionRequest.It doesn't implement
TransactionBuilder4844(It shouldn't). But this makes it incompatible withProviderBuilderdefault constructornewSolution
FromconversionsPR Checklist