feat: impl TryFrom<Transaction> for TxEnvelope

[klkvr]

Motivation

1. Adds conversion from alloy_rpc_types::Signature to alloy_primitives::Signature
2. Adds conversion from alloy_rpc_types::AccessList to alloy_eips::AccessList
3. Adds conversion from alloy_rpc_types::Transaction to TxEnvelope

Blocked by #338

PR Checklist

• Added Tests
• Added Documentation
• Breaking changes

[prestwich]

none of these are 2718 errors. they should not be in this enum

[klkvr]

What'd be the best place for them?

[klkvr]

created separate ConversionError for now

[prestwich]

@mattsse the other way to approach this would be to enforce completeness requirements during serde deser. i.e. make it impossible to construct a Transaction that cannot be converted to a TxEnvelope, and then we could implement From/Into instead of TryFrom/TryInto

[prestwich]

this could be done by embedding a TxEnvelope in the Transaction

[klkvr]

wouldn't such approach cause issues with deposit transaction? we have optional signature for this case I believe

[prestwich]

Optimism is on the hook for making their own rpc Transaction struct and consensus TxEnvelope struct, which could have different semantics

[mattsse]

the other way to approach this would be to enforce completeness requirements during serde deser.

since all fields are public, I don't think we can guarantee this easily,
I think we want the validation checks when we convert from rpc -> consensus

it's also possible that impls are faulty/incomplete for this endpoint, like missing field, I've seen this on a few newer networks, but this should not affect the rpc response imo and only be a problem when converting to consensus tx

[prestwich]

it's also possible that impls are faulty/incomplete for this endpoint, like missing field, I've seen this on a few newer networks, but this should not affect the rpc response imo and only be a problem when converting to consensus tx

we already have validity and correctness checks for the RPC response member data types, e.g. we reject an object in the gas_limit field. So why not check for type-level correctness? Is it just because we want to support faulty and/or off-spec nodes? That would imply we should significantly relax RPC deserialization overall, and seems like a non-goal anyway (or at least a goal for AnyNetwork instead of Ethereum)

[mattsse]

Is it just because we want to support faulty and/or off-spec nodes

this and I don't think we should do any additional checks on deserialization

Also, in order to actually enforce we'd need to make the rpc response type's field private, which I think would be inconvenient

[prestwich]

well wed just need a _:() to prevent incorrect instantiation?

[mattsse]

this only solves instantiation, but not mut access, for example:

let mut tx = p.tx_byhash(<>);
tx.max_fee_per_gas.take();

we could solve this by changing the rpc::Tx to an enum instead, but I'd really prefer a simple object here and keep conversion fallible

[prestwich]

ya, id like to revisit this in the future tho

[prestwich]

much improved 👌