TX refactor PR

[jochem-brouwer]

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:

• Transaction objects now do not share the BaseTransaction class. Instead, these call into Capabilities which does this shared logic for them
• Integrating L2 txs should be made more simpler
• Identify what properties/methods are used inside our own libraries (VM, Block (+client)) which should be exposed. This is likely a super small set. For runtime behavior, this already differs on the tx type (legacy txs have no access lists, so no access lists logic there. If there is an access list, trigger the access list logic). For block, this is likely only serializing txs, plus some extra logic (likely) for blobs.
• This logic should be tx-dependent and therefore keep a Capabilities-like mechanism and let consumer libraries call into these to see if a Capability is supported by the tx, and then use this Capability to run specific capability-like logic.
• The composition pattern gets rid of BaseTransaction and will likely improve performance, see: Tx: High Cost of super() call in Constructors / Inheritance #3694

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.28%. Comparing base (4470cc3) to head (ff99a30).
Report is 91 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
block	?
client	?
tx	?
vm	58.28% <ø> (?)
wallet	?

Flags with carried forward coverage won't be shown. Click here to find out more.

[holgerd77]

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?

[jochem-brouwer]

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

[holgerd77]

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. 🙂

[jochem-brouwer]

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 😄

[jochem-brouwer]

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 EOA7702Tx is then a tx composed of: AccessLists, FeeMarket, EOACode.

If we do this composition pattern, then for instance AccessLists capability will demand the accessLists property on the tx, plus it will also allow it in the TxData field. And FeeMarket will reject gasPrice and add maxPriorityFeePerGas and maxFeePerGas to the interface.

Besides this pattern, this new "features" will then also allow is (thinking a bit of L2 support): "Signable" (this allows the v/r/s fields) and we could go even further, "CreateContract" (this allows a to: null field, this is NOT allowed in 7702 / 4844).

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 toCreationAddress() (method to figure out if we create a contract, this is also required currently on 7702/4844 but always false.

Minimal interface would likely only be methods as: raw() (before serialization) serialize() (RLP serialize), hash(), toJSON.

[jochem-brouwer]

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)