Add supertrait alloy_consensus::Transaction to RPC TransactionResponse #1387
Conversation
emhane
requested review from
DaniPopes,
gakonst,
mattsse,
onbjerg and
prestwich
as code owners
| /// Return the max priority fee per gas if the transaction is an EIP-1559 transaction, and | ||
| /// otherwise return the gas price. | ||
| fn max_priority_fee_per_gas(&self) -> u128; |
There was a problem hiding this comment.
why is this no longer an option?
There was a problem hiding this comment.
removed redundancy within the consensus Transaction trait. gas_price has been deprecated in favour of max fee per gas and max priority fee per gas. checking if this returns an option is a bad way to check if this is a pre-eip1559 tx, and instead trait method Transaction::ty should be used.
There was a problem hiding this comment.
this should be an option because not every tx type has this
| fn max_fee_per_gas(&self) -> u128 { | ||
| self.max_fee_per_gas.unwrap_or(self.gas_price.unwrap_or_default()) | ||
| } | ||
|
|
||
| fn transaction_index(&self) -> Option<u64> { | ||
| self.transaction_index | ||
| fn max_priority_fee_per_gas(&self) -> u128 { | ||
| self.max_fee_per_gas.unwrap_or(self.gas_price.unwrap_or_default()) |
There was a problem hiding this comment.
this is not great but I guess it should never happen and will get cleaned up eventually once we have #1165
There was a problem hiding this comment.
will get cleaned up by manual impl of serialisation for the the alloy_rpc_types_eth::Transaction type. then we don't need to have one field each for the essentially mutually exclusive fields max fee per gas and gas price, we just need the field max fee per gas, and the manual serialisation impl will just check if self.ty() < 2 then use gasPrice as label, otherwise use maxBaseFee
| @@ -47,20 +47,16 @@ pub trait Transaction: any::Any + Send + Sync + 'static { | |||
| /// Get `gas_limit`. | |||
| fn gas_limit(&self) -> u64; | |||
|
|
|||
| /// Get `gas_price`. | |||
| fn gas_price(&self) -> Option<u128>; | |||
There was a problem hiding this comment.
what's our motivation for removing this? I can see how it's redundant from execution perspective, though it is an actual separate property of transaction which is important from RPC PoV
I think it's pretty much a question on how we expect this trait to be consumed and whether separate getters might be useful
There was a problem hiding this comment.
the gas price field is never used on tx types since max fee per gas was introduced
| } | ||
|
|
||
| fn transaction_type(&self) -> Option<u8> { | ||
| self.transaction_type | ||
| fn to(&self) -> Option<Address> { |
There was a problem hiding this comment.
I think this can be removed? given that we already have to on consensus Transaction
There was a problem hiding this comment.
consensus Transaction returns TxKind. requiring every caller of to in rpc context to do this conversion to Option<Address> isn't good.
There was a problem hiding this comment.
I think this is fine? the conversion is as simple as calling kind.to()
and we can probably change to type to TxKind on rpc types as it's done in consensus serde
alloy/crates/consensus/src/transaction/legacy.rs
Lines 48 to 49 in 599e577
There was a problem hiding this comment.
following matze's directive, prio of this pr should be not to change the API. pls open an issue.
alloy-rs#1387) * Add supertrait alloy_consensus::Transaction to RPC TransactionResponse * Feature gate alloy_consensus::Transaction impl for WithOtherFields * Enable alloy-consensus/serde to impl TransactionResponse for WithOtherFields * Revert unify gas price and max fee per gas in alloy_consensus::Transaction * Simplify syntax * Fix lint * Fix bug max prio fee per gas getter impl * Fix typo Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com> --------- Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
Motivation
Closes #1344
Solution
max_fee_per_gasandgas_pricein traitalloy_consensus::Transaction. Removes getter for deprecated namegas_pricefrom traitalloy_consensus::Transaction.alloy_consensus::Transactionas super trait ofTransactionResponseTransactionResponseFollow up would be to manually implement de-/serialisation for
alloy_rpc_types_eth::Transaction, and remove trait methods ofTransactionResponse:to,gas_price,max_fee_per_gasandtransaction_type, which are just matching the values returned byalloy_consensus::Transaction, with the right label w.r.t. to the json object standard for the transaction type.PR Checklist