Embed TxEnvelope into `rpc-types-eth::Transaction` #1460

prestwich · 2024-10-13T14:39:43Z

closes #1165. Alternative smaller approach than #1169

This is a breaking change

cc @klkvr

Solution

changes:

Embed T in alloy_rpc_types_eth::Transaction, default to TxEnvelope
Modify implementation of TransactionTrait for Transaction<T>
delete rpc_types_eth::Signature as in [wip] feat: embed TxEnvelope into rpc_types::Transaction #1169
delete tests that tested invalid responses, as those are no longer deserialized
introduce AnyTxEnvelope, AnyTypedTransaction and AnyReceiptEnvelope abstractions
add TransactionResponse: AsRef<Self::TxEnvelope> bound to network assoc type

drive-by

mark tokio as a non-wasm dependency to silence warnings in cargo hack --target wasm32-unknown-unknown

is this correct?

remove legacy gasPrice field from TxPoolContent. My belief is that this is now only produced for legacy transactions, so removing it is correct behavior and will not break deserializers

PR Checklist

Added Tests
Added Documentation
Breaking changes

klkvr · 2024-10-13T14:45:49Z

this would break AnyNetwork because rpc_types_eth::Transaction would not be able to deserialize non-ethereum transaction types

prestwich · 2024-10-13T14:51:16Z

this would break AnyNetwork because rpc_types_eth::Transaction would not be able to deserialize non-ethereum transaction types

<AnyNetwork as Network>::TxEnvelope is already set to TxEnvelope, which means the provider already cannot correctly sign or send non-ethereum transactions. So this would not break it any worse than it is already broken. It would make the type somewhat more internally consistent too

klkvr · 2024-10-13T14:53:36Z

this would break AnyNetwork because rpc_types_eth::Transaction would not be able to deserialize non-ethereum transaction types

<AnyNetwork as Network>::TxEnvelope is already set to TxEnvelope so this would not break it any worse than it is already broken. It would make the type somewhat more internally consistent too

TransactionResponse is still an rpc type. changing transaction respose type to an envelope would break eg get_block(..., true) requests deserialization on OP because of deposit transaction

prestwich · 2024-10-13T14:57:43Z

current CI error seems to indicate that anvil sometimes doesn't return a type flag?

"{\"hash\":\"0x4b56f1a6bdceb76d1b843e978c70ab88e38aa19f1a67be851b10ce4eec65b7d4\",\"nonce\":\"0x0\",\"blockHash\":\"0xc3d639df51a3a7af3c0587b16d4a59c66bb8c2724d288fca3d3d0b3a505689e0\",\"blockNumber\":\"0x1\",\"transactionIndex\":\"0x0\",\"from\":\"0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266\",\"to\":\"0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045\",\"value\":\"0x64\",\"gasPrice\":\"0x4a817c800\",\"gas\":\"0x5208\",\"input\":\"0x\",\"r\":\"0x71575df226771c8a9c18eab02457d65bc02ab1b978c78ec4b291f903305291b6\",\"s\":\"0xd04224f8696f449af09a1851bedcaeae3c5836cedb8ccbe489622a34278b0da\",\"v\":\"0xf4f6\",\"chainId\":\"0x7a69\"}"

prestwich · 2024-10-13T15:01:25Z

TransactionResponse is still an rpc type. changing transaction respose type to an envelope would break eg get_block(..., true) requests deserialization on OP because of deposit transaction

yes, we agree that this is a breaking change. however, it is not going to break the AnyNetwork type, as it is already broken

a more-complete fix might look like this:

#[serde(untagged)]
pub enum AnyTxEnvelope { 
    Known(TxEnvelope), 
    Unknown { type: u8, fields: HashMap<String, Value> } 
}

but that's somewhat outside the scope of #1165

klkvr · 2024-10-13T15:05:23Z

current CI error seems to indicate that anvil sometimes doesn't return a type flag?

"{\"hash\":\"0x4b56f1a6bdceb76d1b843e978c70ab88e38aa19f1a67be851b10ce4eec65b7d4\",\"nonce\":\"0x0\",\"blockHash\":\"0xc3d639df51a3a7af3c0587b16d4a59c66bb8c2724d288fca3d3d0b3a505689e0\",\"blockNumber\":\"0x1\",\"transactionIndex\":\"0x0\",\"from\":\"0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266\",\"to\":\"0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045\",\"value\":\"0x64\",\"gasPrice\":\"0x4a817c800\",\"gas\":\"0x5208\",\"input\":\"0x\",\"r\":\"0x71575df226771c8a9c18eab02457d65bc02ab1b978c78ec4b291f903305291b6\",\"s\":\"0xd04224f8696f449af09a1851bedcaeae3c5836cedb8ccbe489622a34278b0da\",\"v\":\"0xf4f6\",\"chainId\":\"0x7a69\"}"

yep, anvil does not return type flag for legacy transactions atm, I think we should fix this though I believe this should be handled during deserialization as well

klkvr · 2024-10-13T15:07:13Z

#[serde(untagged)]
pub enum AnyTxEnvelope { 
    Known(TxEnvelope), 
    Unknown { type: u8, fields: HashMap<String, Value> } 
}

this won't work because of rlp bounds on envelope

klkvr · 2024-10-13T15:10:18Z

yes, we agree that this is a breaking change. however, it is not going to break the AnyNetwork type, as it is already broken

right now we are using AnyNetwork in foundry to fetch and display/re-execute arbitrary transactions through AnyNetwork. including this PR as is would break this

klkvr · 2024-10-13T15:15:34Z

it would be great if we'd figure out a way to have TransactionResponse: Into<Self::TxEnvelope> bound on Network but this is just not possible if we want to have a catch-all network implementation (which AnyNetwork is supposed to be)

prestwich · 2024-10-13T15:20:58Z

this won't work because of rlp bounds on envelope

Envelope doesn't have RLP bounds, only 2718 bounds, which can be satisfied with a dummy implementation. It's impossible to correctly model unknown 2718 encodings so incorrect behavior here should be acceptable

prestwich · 2024-10-13T15:52:44Z

latest commit includes AnyTxEnvelope and AnyTypedTransaction, as well as workarounds for allowing them to meet the transaction trait by memoizing desered values

prestwich · 2024-10-13T22:31:19Z

okay @klkvr this should be ready for re-review with above concerns addressed. I also put a summary of changes in the PR description

this would break AnyNetwork because rpc_types_eth::Transaction would not be able to deserialize non-ethereum transaction types

now handled by the new Any_____ types

yep, anvil does not return type flag for legacy transactions atm, I think we should fix this though I believe this should be handled during deserialization as well

handled by MaybeTagged deser

this won't work because of rlp bounds on envelope

handled by the dummy 2718 impls

it would be great if we'd figure out a way to have TransactionResponse: IntoSelf::TxEnvelope bound on Network but this is just not possible if we want to have a catch-all network implementation (which AnyNetwork is supposed to be)

I added this as AsRef<Self::TxEnvelope>. An Into bound is blocked by WithOtherFields, as we can't generically impl<T, U> Into<U> for WithOtherFields where T: Into<U> because it conflicts with std lib impls

klkvr · 2024-10-13T23:12:07Z

nice! thanks for spending your time on this

handled by the dummy 2718 impls

this is kind of hacky but I don't think there's a way to fit AnyNetwork into current abstraction without this hack or another one. wdyt @mattsse @emhane

crates/network/src/any/mod.rs

prestwich · 2024-10-14T00:14:04Z

also, agree the 2718 is pretty jank >.> i thought about making it panic instead

klkvr · 2024-10-14T00:58:36Z

also, agree the 2718 is pretty jank >.> i thought about making it panic instead

something we could do is divide Network into 2 traits — one which would just contain the ATs we need for provider without much bounds (basically we just need serde for responses and 2718 for tx envelope). could call it ProviderNetwork

and second trait which would represent a normal network with infallible rpc<->consensus conversions which would have a blanket impl for all ProviderNetwork implementations satisfying stronger bounds. AnyNetwork would just not implement it

however doing all of this just for AnyNetwork doesn't make much sense

prestwich · 2024-10-14T12:43:17Z

something we could do is divide Network into 2 traits — one which would just contain the ATs we need for provider without much bounds (basically we just need serde for responses and 2718 for tx envelope). could call it ProviderNetwork

and second trait which would represent a normal network with infallible rpc<->consensus conversions which would have a blanket impl for all ProviderNetwork implementations satisfying stronger bounds. AnyNetwork would just not implement it

however doing all of this just for AnyNetwork doesn't make much sense

I like the idea generally, but i think you're right that doing it just for AnyNetwork doesn't make much sense. It would introduce a lot of mental overhead for bounds-writing, which is common, while using AnyNetwork is somewhat uncommon

prestwich · 2024-10-14T12:45:36Z

updated the docs for AnyNetwork to reflect some known rough edges :)

prestwich · 2024-10-14T14:56:08Z

crates/rpc-types-eth/src/transaction/mod.rs

-    #[cfg_attr(feature = "serde", serde(default, skip_serializing_if = "Option::is_none"))]
-    pub authorization_list: Option<Vec<SignedAuthorization>>,
+    // /// Transaction hash
+    // pub hash: B256,


This is the last outstanding issue. For some reason, uncommenting this makes inner deserialization fail. Probably a subtle and annoying serde interaction 😮‍💨

ah, it's because

Signed has an unskipped hash field

the outer Transaction consumes the "hash" in deserialization

there is no "hash" field left for Signed to deserialize

so the problem becomes

let Signed deserialize the hash

make hash available on Transaction in a generic way

OR

manually impl deserialize for Transaction to avoid consuming the hash

this seems easier and doesn't require introducing new bounds 👍

should we just remove the hash from Transaction?

I believe transaction hash is just keccak256(tx_envelope.encode_2718())? which should be possible to do for any envelope

will look into this today. at first blush, it looks like trie_hash is provided on the Encodable2718 trait, so we should be able to remove from Signed. We still strongly want memoization tho so need to figure out how to keep that without interfering with serialization 🤔

it also looks like d0794e5 lightly broke eip2718 abstraction by making type flag encoding tx-associated instead of envelope-associated. We would now behave badly for networks where the type flag is different (e.g. TxEip1559 is flag 17, or where both 4 and 5 are TxEip7702, so we should probably open a separate issue for that

I believe transaction hash is just keccak256(tx_envelope.encode_2718())? which should be possible to do for any envelope

this is actually not the case for eip4844 with sidecar 😮‍💨 but it's straightforward to work around i think

I'm wondering if we could solve this by adding a TxEnvelope trait which would have a tx_hash method? all normal variants already keep hash in Signed and we can just add a mandatory hash field to Other variant

I think #1485 is reasonable as well because sealable and signable are 2 valid distinct properties of transaction (eg deposit transaction is only sealable). but would like to keep scope here smaller if possible as PR is already pretty big

I'm wondering if we could solve this by adding a TxEnvelope trait which would have a tx_hash method? all normal variants already keep hash in Signed and we can just add a mandatory hash field to Other variant

the trie_hash method already exists on Encodable2718, so we are covered there :)

the trie_hash method already exists on Encodable2718, so we are covered there :)

yeah but our impl of Encodable2718 is not correct for Other variant?

prestwich · 2024-10-15T21:31:42Z

but would like to keep scope here smaller if possible as PR is already pretty big

yes. I'd like to merge #1485 and then see how much of this i can trim while still keeping functionality right. Might break this up into several prepatory PRs

prestwich · 2024-10-15T21:53:47Z

okay I split up a bunch of the non-central functionality into small PRs. should help trim scope some

prestwich requested review from DaniPopes, gakonst, mattsse, onbjerg and emhane as code owners October 13, 2024 14:39

klkvr reviewed Oct 13, 2024

View reviewed changes

crates/network/src/any/mod.rs Show resolved Hide resolved

klkvr mentioned this pull request Oct 14, 2024

fix: remove signature assoc type from tx response trait #1451

Merged

3 tasks

prestwich force-pushed the prestwich/tx-embed branch from df5b08a to b6aa8a4 Compare October 14, 2024 12:34

prestwich force-pushed the prestwich/tx-embed branch from 2b0f30b to 1136ec6 Compare October 14, 2024 14:54

prestwich commented Oct 14, 2024

View reviewed changes

prestwich mentioned this pull request Oct 15, 2024

Fix: move type flag encoding back into encode_2718 #1483

Open

prestwich force-pushed the prestwich/tx-embed branch 2 times, most recently from 81c92fd to 6e5065e Compare October 15, 2024 16:48

prestwich mentioned this pull request Oct 15, 2024

refactor: move hash memoization out of signed #1485

Open

3 tasks

prestwich force-pushed the prestwich/tx-embed branch from 6e5065e to bac7800 Compare October 16, 2024 02:54

prestwich self-assigned this Oct 17, 2024

prestwich added 12 commits October 18, 2024 20:51

refactor: embed TxEnvelope into rpc Transaction

4babe08

refactor: bytes

afca827

feat: AnyTxEnvelope and AnyTypedTransaction

7d425b9

fix: impl transaction

cb9a27e

feat: deser memoization

073239e

fix: serde when tag is missing

c9898b7

fix: CI

f583da3

feat: asref bound for transaction response

a1a4918

doc: more of them for anynetwork

803721e

doc: fix link

706b21f

fix: incorrect rename

294cb90

chore: delete unused variants of ConversionError

7998bf5

prestwich force-pushed the prestwich/tx-embed branch from 02c50c7 to 7998bf5 Compare October 19, 2024 00:51

DaniPopes requested a review from klkvr October 19, 2024 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embed TxEnvelope into `rpc-types-eth::Transaction` #1460

Embed TxEnvelope into `rpc-types-eth::Transaction` #1460

prestwich commented Oct 13, 2024 •

edited

Loading

klkvr commented Oct 13, 2024

prestwich commented Oct 13, 2024 •

edited

Loading

klkvr commented Oct 13, 2024

prestwich commented Oct 13, 2024

prestwich commented Oct 13, 2024 •

edited

Loading

klkvr commented Oct 13, 2024

klkvr commented Oct 13, 2024

klkvr commented Oct 13, 2024

klkvr commented Oct 13, 2024

prestwich commented Oct 13, 2024

prestwich commented Oct 13, 2024 •

edited

Loading

prestwich commented Oct 13, 2024

klkvr commented Oct 13, 2024

prestwich commented Oct 14, 2024

klkvr commented Oct 14, 2024 •

edited

Loading

prestwich commented Oct 14, 2024

prestwich commented Oct 14, 2024

prestwich Oct 14, 2024

prestwich Oct 14, 2024

prestwich Oct 14, 2024

prestwich Oct 14, 2024

klkvr Oct 14, 2024

prestwich Oct 15, 2024 •

edited

Loading

prestwich Oct 15, 2024

klkvr Oct 15, 2024

prestwich Oct 15, 2024

klkvr Oct 16, 2024

prestwich commented Oct 15, 2024 •

edited

Loading

prestwich commented Oct 15, 2024

Embed TxEnvelope into rpc-types-eth::Transaction #1460

Are you sure you want to change the base?

Embed TxEnvelope into rpc-types-eth::Transaction #1460

Conversation

prestwich commented Oct 13, 2024 • edited Loading

Solution

PR Checklist

klkvr commented Oct 13, 2024

prestwich commented Oct 13, 2024 • edited Loading

klkvr commented Oct 13, 2024

prestwich commented Oct 13, 2024

prestwich commented Oct 13, 2024 • edited Loading

klkvr commented Oct 13, 2024

klkvr commented Oct 13, 2024

klkvr commented Oct 13, 2024

klkvr commented Oct 13, 2024

prestwich commented Oct 13, 2024

prestwich commented Oct 13, 2024 • edited Loading

prestwich commented Oct 13, 2024

klkvr commented Oct 13, 2024

prestwich commented Oct 14, 2024

klkvr commented Oct 14, 2024 • edited Loading

prestwich commented Oct 14, 2024

prestwich commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestwich Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestwich commented Oct 15, 2024 • edited Loading

prestwich commented Oct 15, 2024

Embed TxEnvelope into `rpc-types-eth::Transaction` #1460

Embed TxEnvelope into `rpc-types-eth::Transaction` #1460

prestwich commented Oct 13, 2024 •

edited

Loading

prestwich commented Oct 13, 2024 •

edited

Loading

prestwich commented Oct 13, 2024 •

edited

Loading

prestwich commented Oct 13, 2024 •

edited

Loading

klkvr commented Oct 14, 2024 •

edited

Loading

prestwich Oct 15, 2024 •

edited

Loading

prestwich commented Oct 15, 2024 •

edited

Loading