Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: move type flag encoding back into encode_2718 #1483

Open
prestwich opened this issue Oct 15, 2024 · 4 comments
Open

Fix: move type flag encoding back into encode_2718 #1483

prestwich opened this issue Oct 15, 2024 · 4 comments

Comments

@prestwich
Copy link
Member

prestwich commented Oct 15, 2024

it looks like d0794e5 lightly broke eip2718 abstraction by making type flags 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

It also means that if we have a tx type with two different on different networks, they can't re-use the same codepaths

Need to move the flag encoding back into the envelope-level encode_2718 implementation

Originally posted by @prestwich in #1460 (comment)

@prestwich prestwich changed the title > I believe transaction hash is just keccak256(tx_envelope.encode_2718())? which should be possible to do for any envelope Fix: move type flag encoding back into encode_2718 Oct 15, 2024
@prestwich
Copy link
Member Author

cc @Rjected

@prestwich
Copy link
Member Author

prestwich commented Oct 15, 2024

this also affects encode_for_signing in SignableTransaction, which somewhat implies we need to re-think the signing trait model 😮‍💨

@klkvr
Copy link
Member

klkvr commented Oct 15, 2024

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

I am not sure we should optimize for this edge case at cost of forcing users to route all encoding through envelope and introducing additional layering on envelope variants

@prestwich
Copy link
Member Author

I am not sure we should optimize for this edge case at cost of forcing users to route all encoding through envelope

The 2718 edge case aside, we already force users to route encoding through envelope, it was one of the main goals of introducing it as a type and the only reason for making all the encoding methods hidden in #529. There was a lot of discussion on this in issues and the telegram group at the time and we collectively agreed that running coding via the transaction types has proven to result in bad user behavior

Basically, from ethers we learned we can't rely on users to understand the distinction between eip2718, rlp, and network format. As a result, having those be a public API is a footgun for users. We saw this a lot with users RLP encoding 1559 transactions to submit to sendRawTransaction, which failed and resulted in an awful lot of confused messages in telegram. Making incorrect or confusing actions inaccessible to users was a primary goal of introducing the envelope type in the first place

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants