Embed TxEnvelope into `rpc-types-eth::Transaction` by prestwich · Pull Request #1460 · alloy-rs/alloy

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-31T12:49:32Z

we don't have any more deserialization issues, right?

Not that I know of

This is rebased and ready for review

prestwich · 2024-10-31T12:51:06Z

side note, because the type aliases for AnyRpcBlock have to reference AnyTxEnvelope they've been pushed into the alloy-network crate and re-exported from the any module. I also re-exported AnyHeader and AnyReceipt there, so all Any____ can be accessed from the same module

klkvr · 2024-10-31T12:51:39Z

crates/network/src/any/mod.rs

+pub use alloy_consensus::{AnyHeader, AnyReceiptEnvelope};
+
+/// A catch-all header type for handling headers on multiple networks.
+pub type AnyRpcHeader = alloy_rpc_types_eth::Header<alloy_consensus::AnyHeader>;
+
+/// A catch-all block type for handling blocks on multiple networks.
+pub type AnyRpcBlock =
+    WithOtherFields<Block<WithOtherFields<Transaction<AnyTxEnvelope>>, AnyRpcHeader>>;
+


we already have these here

alloy/crates/rpc-types-eth/src/lib.rs

Line 26 in 273d784

pub type AnyNetworkHeader = Header<alloy_consensus::AnyHeader>;

#1460 (comment)

Those can't be in the rpc-types-eth crate as they need to import AnyTxEnvelope now. In addition, it's somewhat unclean to put AnyRpc____ objects into rpc-types-eth. Do you thinkwe should add rpc-types-any as a crate?

AnyRecipt and AnyHeader are already in alloy-consensus

can't AnyTxEnvelope be there too?

i don't think it was a good idea to put them there in the first place. alloy-consensus has always been specifically eth consensus. It's also much more invasive to move a large type downwards instead of moving 2 aliases upwards

followup work:
#1598

crates/network/src/any/mod.rs

mattsse · 2024-10-31T13:31:55Z

crates/network/src/any/mod.rs

+    }
+}
+
+impl alloy_consensus::Transaction for AnyTxEnvelope {


do we need this to satisfy the the Network trait constraint

we don't, but it seems sufficiently useful to users to have this

crates/network/src/any/mod.rs

mattsse

cool, I think this is a pretty big UX improvement.

pending @klkvr

crates/network/Cargo.toml

mattsse · 2024-10-31T21:32:46Z

crates/network/src/any/mod.rs

+/// A catch-all header type for handling headers on multiple networks.
+pub type AnyRpcHeader = alloy_rpc_types_eth::Header<alloy_consensus::AnyHeader>;


moving these aliases here makes sense imo

klkvr

lgtm! will try to plug this into reth and see if anything breaks

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

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

klkvr reviewed Oct 13, 2024

View reviewed changes

crates/network/src/any/mod.rs Outdated 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

Closed

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

prestwich added 8 commits October 31, 2024 08:49

chore: delete unused variants of ConversionError

74c9d97

fix: trie_hash for anytxenvelope

81fb96b

fix: trie_hash on encodable2718 for envelope

96f88a7

fix: clippy

e4f2d3f

refactor: make encoding panic

342ee88

chore: remove commented code

3304b2c

cleanup: misc use of deser_by_key

f4660c0

fix: hash in anytx, generic default on block

914ef71

prestwich force-pushed the prestwich/tx-embed branch from 0ba1495 to 914ef71 Compare October 31, 2024 12:49

klkvr reviewed Oct 31, 2024

View reviewed changes

prestwich mentioned this pull request Oct 31, 2024

[Feature] Move AnyNetwork types into their own crate(s) #1598

Open

mattsse requested changes Oct 31, 2024

View reviewed changes

prestwich added 3 commits October 31, 2024 16:30

fix: make Unknown variants their own types

ee2deec

fix: flattens

bc158e1

refactor: break out more files

156d7d9

prestwich force-pushed the prestwich/tx-embed branch from 0b9fbc0 to 156d7d9 Compare October 31, 2024 21:02

prestwich added 2 commits October 31, 2024 17:02

lint: clippy

3a199d1

fix: doclinks

a600b06

mattsse approved these changes Oct 31, 2024

View reviewed changes

klkvr approved these changes Oct 31, 2024

View reviewed changes

Update crates/network/Cargo.toml

5978390

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

klkvr enabled auto-merge (squash) October 31, 2024 23:33

klkvr merged commit 1042998 into main Oct 31, 2024

klkvr deleted the prestwich/tx-embed branch October 31, 2024 23:41

This was referenced Nov 1, 2024

feat: integrate signature with boolean parity #1540

Merged

feat: implement Arbitrary for transaction types #1603

Merged

feat: remove AnyNetwork usage paradigmxyz/reth#12280

Merged

yash-atreya mentioned this pull request Nov 7, 2024

fix(network): impl NetworkWallet<AnyNetwork> #1631

Merged

3 tasks

		/// A catch-all header type for handling headers on multiple networks.
		pub type AnyRpcHeader = alloy_rpc_types_eth::Header<alloy_consensus::AnyHeader>;

Conversation

prestwich commented Oct 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Solution

PR Checklist

Uh oh!

klkvr commented Oct 13, 2024

Uh oh!

prestwich commented Oct 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klkvr commented Oct 13, 2024

Uh oh!

prestwich commented Oct 13, 2024

Uh oh!

prestwich commented Oct 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klkvr commented Oct 13, 2024

Uh oh!

klkvr commented Oct 13, 2024

Uh oh!

klkvr commented Oct 13, 2024

Uh oh!

klkvr commented Oct 13, 2024

Uh oh!

prestwich commented Oct 13, 2024

Uh oh!

prestwich commented Oct 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prestwich commented Oct 13, 2024

Uh oh!

klkvr commented Oct 13, 2024

Uh oh!

Uh oh!

prestwich commented Oct 14, 2024

Uh oh!

klkvr commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prestwich commented Oct 14, 2024

Uh oh!

prestwich commented Oct 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prestwich Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prestwich commented Oct 31, 2024

Uh oh!

prestwich commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 31, 2024 •

edited

Loading