Implement EIP 7702

[jochem-brouwer]

Implements 7702 according to https://github.com/ethereum/EIPs/blob/14400434e1199c57d912082127b1d22643788d11/EIPS/eip-7702.md

Note that there are many open PRs in the EIP repo:

https://github.com/ethereum/EIPs/pulls?q=is%3Apr+is%3Aopen+7702

So, the spec is likely to change.

Some other items I found:

• EIP 1559 extends the logic of EIP 2930, but the code seems to be copied
• I had to do the same with EIP 7702
• In a later refactor of tx we should consider making these txs "stackable" (I tried this, but this is somewhat a hassle with the typing).
• So we currently now have a lot of copied code regarding 2930 / 1559 / 7702 which we should address soon(ish)
• The problem is that you cannot do EOFCodeEIP7702Transaction extends FeeMarketEIP1559Transaction is that the return types of some methods change, i.e. the new txs now have these mandatory authorityLists fields, so we should likely think of something the same as how we have created BaseTransaction now (these new txs now also accept a return type, so we can "stack" the tx types)

Still WIP, main things to do:

• Add logic to VM
• Write tests
• Watch the spec for updates

[holgerd77]

So this is touching the whole topic of a fitting structural setup for the tx library which we litterally have for years, so since tx types have been introduced.

I don't think "stacking" or inheritance is a good design choice here, also cc-ing @gabrocheleau here since you had some related exchange in the chat, since this is just tighly bundling functionality together which now "accidentally" matches but will likely bite us in the future.

Tx types are designed to be independent of each other (for the good and the bad), so there is no such thing as "this tx type inherits its properties from tx type x" or similar stated in EIPs, and in the future arbitrary things can (and likely will) change, be it the way access lists are structured or the fee market is organzied (to pick up the 2930/1559 example). If you think this along the lines of a potentially introduced inheritance, things get messy pretty quickly (or: first one is starting with some still-working work-arounds (inherited oldAccessLists boolean flags or whatever), and then things get messy in round 2 😋).

This actually also goes for the BaseTransaction, I was rather always thinking in my head that we should get rid of this at some point, since this is also some relatively random mix of "features" for txs grown out of historic reasons, atm mainly the signature scheme, which might also change at some point.

I think what describes a desired design goal best is the concept of "composability", so that one can plug the different "features" togehter, as independently as possible. That's why we came up with this Capabilities concept which Gabriel implemented recently, which already matches this well (after exploring (and failing) more sophisticated solutions like multiple inheritance or the like).

When looking at the current code I find this actually not "quite as bad" already. 🙂 (things would be a lot worse without our Capabilities stuff)

So: I wouldn't count these one liner integrations like:

  getMessageToSign(): Uint8Array {
    return EIP2718.serialize(this, this.raw().slice(0, 9))
  }

actually as redundancy. This is more "by design" and rather has its beauty by stating explicitly what this respective tx type composes off (takes the base seralization functionality from generic tx types).

What is admittedly a bit suboptimal here is this code doc doubling, maybe we can look into this a bit specifically at some point and find a targeted solution here (not sure, is it possible to "programmatically reference existing code docs" to be included? 🤔).

I generally think to improve here needs specific looks into what is doubled/getting redundant. I think we just have different "more subtle" problem sets still in which needs to be addressed separately and are a bit a left-over (respectively just: weren't addressed yet) along this capability refactor (since this was the big first step) and are just "step 2".

I will try to tear this a bit apart, will see how far I'll come 🙂:

raw() method

So the raw() method is admittedly just a very specifc method which is different for more or less every tx type.

What we could do here is to put the 1559 inner method code over to the 1559 capabilites and call the capability code from the 1559 tx and then also call the 1559 capability code from the 7702 tx and then use splice() or so to rebuild the array.

Might be worth it for code redundancy reduction, and might also not have relevant performance implications.

Note: I guess these kind of things I write down here are structurally present throughout the wider code base and will likely be able to applied on other tx types as well (if we decide we want that).

Static constructors

    if (
      equalsBytes(serialized.subarray(0, 1), txTypeBytes(TransactionType.EOACodeEIP7702)) === false
    ) {
      throw new Error(
        `Invalid serialized tx input: not an EIP-7702 transaction (wrong tx type, expected: ${
          TransactionType.EOACodeEIP7702
        }, received: ${bytesToHex(serialized.subarray(0, 1))}`
      )
    }

    const values = RLP.decode(serialized.subarray(1))

    if (!Array.isArray(values)) {
      throw new Error('Invalid serialized tx input: must be array')
    }

I think this can go into a 2718 capability helper method taking in the tx type and an EIP string (EIP-7702) and we should do that (for all tx classes where applicable).

Guess fromValuesArray() is so type specific that we want to live with that (?).

main construtor

Not sure if it's possible to put the following into a one-liner in the 2930 capabilites, might be worth it:

// Populate the access list fields
   const accessListData = AccessLists.getAccessListData(accessList ?? [])
   this.accessList = accessListData.accessList
   this.AccessListJSON = accessListData.AccessListJSON
   // Verify the access list format.
   AccessLists.verifyAccessList(this.accessList)

I think most of the following can be put into a 1559 capability check method? 🤔

this.maxFeePerGas = bytesToBigInt(toBytes(maxFeePerGas))
    this.maxPriorityFeePerGas = bytesToBigInt(toBytes(maxPriorityFeePerGas))

    this._validateCannotExceedMaxInteger({
      maxFeePerGas: this.maxFeePerGas,
      maxPriorityFeePerGas: this.maxPriorityFeePerGas,
    })

    BaseTransaction._validateNotArray(txData)

    if (this.gasLimit * this.maxFeePerGas > MAX_INTEGER) {
      const msg = this._errorMsg('gasLimit * maxFeePerGas cannot exceed MAX_INTEGER (2^256-1)')
      throw new Error(msg)
    }

    if (this.maxFeePerGas < this.maxPriorityFeePerGas) {
      const msg = this._errorMsg(
        'maxFeePerGas cannot be less than maxPriorityFeePerGas (The total must be the larger of the two)'
      )
      throw new Error(msg)
    }

addSignature

I have the impression this is just a bit ugly because there is generally something a bit suboptimally designed around here. Not sure, maybe this can be generally made more elegant throughout the library (might or might not need some breaking changes).

Yeah, so I would say that's already mostly it.

I would agree that if we "fix" the doc redundancy problem (generally), that would be a somewhat big win for the library.

But otherwise things should already look substantially (and, from my POV: sufficiently) better 🙂 without the need for very deep reaching refactoring.

[acolytec3]

LGTM - we should remove the TODO and merge.

[codecov]

Codecov Report

Attention: Patch coverage is 16.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.28%. Comparing base (d24ca11) to head (3cebd3c).
Report is 41 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
blockchain	90.97% <ø> (ø)
client	84.27% <16.66%> (?)
common	?
devp2p	81.83% <ø> (?)
statemanager	?
util	81.46% <ø> (+0.03%)	⬆️

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

[g11tech]

why are we saving json upfront in the tx field

[jochem-brouwer]

We also do this in 1559 / 2929, I suggest we factor this out in another PR which also covers those txs

[g11tech]

i think typescript should be able to infer the other type by itself if you use !isAuthorizationList

but am not 100% sure of typescript shenanigans

[jochem-brouwer]

I have copied this pattern from the access lists :)

[g11tech]

would just reading the properities and comparing them to undefined with a || be simpler ?

[jochem-brouwer]

I'm not sure what you mean here?

[acolytec3]

LGTM

[holgerd77]

🎉

[holgerd77]

There is not a good reason that we have these AuthorizationList types in the Common interface file (these should really be reserved for the VIIs, to the "Very Important Interfaces" 😂 used to type on other libraries.

Same goes I guess for the access list interfaces (for both: unless I am unaware of some below-tx-library usages).

We should move them over to tx in a small clean-up PR.

[jochem-brouwer]

I just checked, for both (so also AccessList) the only packages which import those are tx and vm, and since vm has a tx dependency we can indeed move these into common.

I don't recall why we put AccessList in common (but to be consistent I also put AuthorizationList in here). I do slightly recall we maybe did this for package consumers to import these types (?) but then they would only need the common package and not import tx? However this does not sound like a very good reason for me, I don't know if there is any use case for those types which would not depend or use tx.

We should indeed clean-up and remove both AuthorizationList and AccessList from common (in same PR)

[holgerd77]

These repeated test data lists make this whole test setup pretty expansive. Can we clean this up by taking a base authorization list item data set (or the full authorization list, whatever fits better), and then only overwrite the data item to test (can maybe be done in one PR togehter with the interface movings to tx, see above).

Guess that's something to //cc @jochem-brouwer in.

[jochem-brouwer]

Yes, that would be a better approach in general, will address.

[holgerd77]

This is valid? To have the chainId set to 0x??

[holgerd77]

Respectively going further (without having read the EIP): shouldn't there be a check on our side if the chainId here matches the one from Common and otherwise (we) throw?

[jochem-brouwer]

Yes, 0x means it is valid on all chains. From the EIP:

2. Verify the chain id is either 0 or the chain’s current ID.

[holgerd77]

Ah, interesting.

[holgerd77]

Ok, but then I guess at least Common (and therefore signed tx through chain ID in signature) and chain ID here differing should throw on the tx side?

Because atm it does not, tested with mainnet Common and chain ID 2.

[holgerd77]

Had another look into the EIP, this (chain ID check) is only mentioned in execution, but would this make sense to allow such a tx in the network (so: in a block?).

If you haven't heard anything specific about that we should likely ask for confirmation/explanation/re-evaluation.

[jochem-brouwer]

Yes, it is thus a valid tx and it is thus allowed in the block, we should not throw. If the chain id does not match, we thus "skip" the tuple and do not invalidate the tx.

[holgerd77]

What was the reasoning to allow this during the EIP discussion/specification?

[holgerd77]

(or: was this a decision?)

[jochem-brouwer]

I don't know the exact reason, but I think it is among the lines just like with access lists, you are allowed to put the same address twice. This does not invalidate the tx, you just have to pay more gas (so no one wil do it). The same reasoning works here, you can input any authority tuple, but if it does not match the chain id it will do absolutely nothing, but you have to pay gas.

[holgerd77]

Ok, just seeing this here.

So I would interpret this (still without reading the EIP): txs with non-matching chain IDs are valid (could be included in mainnet), but execution just "does nothing" if not matching.

Is this correct/intended?

[jochem-brouwer]

Yes, from the EIP:

If any of the above steps fail, immediately stop processing that tuple and continue to the next tuple in the list.

The list contains point (2) the chain id check, it should either be 0 or the current chain id. If it thus does not match (and it is nonzero) then we continue processing, but it thus does not change any code. (Point 7 is changing the code of the authority).

See: https://eips.ethereum.org/EIPS/eip-7702#behavior