-
Notifications
You must be signed in to change notification settings - Fork 771
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
TX refactor PR #3733
base: master
Are you sure you want to change the base?
TX refactor PR #3733
Conversation
ff99a30
to
f4887ef
Compare
That sounds all great (I have one small comment but I will go the dive-deeper later)! ⭐ 😄 Some note: there is only one file (and only types) pushed for now, is this intended? |
Yes, and I actually intended not to push that. I have 2 monorepo folders open to experiment a bit, and sometimes (if I switch branches for instance to test a PR) I stash/commit my experiment so I can continue later on. (The local branches here are super dirty 😓 ). I accidentally pushed this branch, will force-push and remove it later on and then cherry-pick the relevant commits in to tidy things up :) Side note: I think the earliest I can spin up a PR which would be in a kind-of re-viewable state would be Monday |
Yeah, I mean this late pushing - especially on larger work - always bears the risk of stronger discussions and eventual change needs, so it also has some benefits to push earlier on, even if things are not fully cleaned up. But as you want. 🙂 |
Yes, good point. I have to note that I have started over from scratch many times (also recently) with new ideas (or finding out that the previous approach did not work). I will push once I have a clean PoC so we can start to get some eyes on there and leave some feedback and open the discussion 😄 |
For phase 2 of tx refactor will push more of the composition pattern to the tx, together with Holgers document and some past ideas I had the following idea: The 7702 transaction for instance is a composition of the logic of Access Lists (2930), Fee Market (1559) and EOA code deployment (7702). What I was thinking about is along the following lines: currently we have Capabilities logic per EIP, but I think we should swap this out to something more specific. For instance, a tx in the future could have parts of an EIP but also some parts not. So I would propose: The capabilities get renamed: 2930 -> AccessLists, 1559 -> FeeMarket, 4844 -> Blobs, 7702 -> EOACode. A If we do this composition pattern, then for instance AccessLists capability will demand the Besides this pattern, this new "features" will then also allow is (thinking a bit of L2 support): "Signable" (this allows the This also allows us to create a SUPER MINIMAL tx interface with only the required fields. We now have some super deep logic hardcoded into the tx interface, for instance such as Minimal interface would likely only be methods as: |
Here is the code overview of master as of 287f960 (to track if we blow up the package with a lot of lines or not) |
Had to add a change to open a PR. I will use this PR to dump in any thoughts / finds upon the TX refactor and will cherry-pick in my commits. I will update this text and likely also post some comments here.
Closes #3723, closes #3694, closes #3484
The main goal of this is to refactor the Tx library.
Updating from an inheritance pattern to a composition pattern
We have used the inheritance pattern by extending the
BaseTransaction
(this holds the "shared logic" of currently all txs which we support). This does seem to make sense since all transaction types on L1 currently share a lot of logic. However, when we take into account L2 txs:Arbitrum: https://docs.arbitrum.io/how-arbitrum-works/arbos/geth#transaction-types
Optimism: https://specs.optimism.io/protocol/deposits.html#the-deposited-transaction-type
(two examples, obviously not all)
These txs could for instance not have a signature, or not have a nonce. However, these still run the VM (and likely are also included in the block, but I did not check this).
We have created
Capabilities
into Tx before (thanks @gabrocheleau #2993, #3010). It seems to make much more sense (especially when keeping in mind support for the L2 txs) to move away from the inheritance pattern and instead use a composition pattern.That is:
BaseTransaction
and will likely improve performance, see: Tx: High Cost of super() call in Constructors / Inheritance #3694