Implement EIP 7702

[jochem-brouwer]

Implements 7702 according to https://github.com/ethereum/EIPs/blob/62419ca3f45375db00b04a368ea37c0bfb05386a/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]

This is super weird to me. Why not just use a single integer, 0 or greater? Also, is it dangerous to let nonce be null? Does this allow for unbounded use of some code?

[jochem-brouwer]

I understand the EIP as: there has to be a list, the length of this list can either be 0 or 1, and it is used to represent a certain nonce which the account should have in order to set the code to it.

[acolytec3]

The EIP says If any of the above steps fail, immediately stop processing that tuple and continue to the next tuple in the list. It will In the case of multiple tuples for the same authority, set the code specified by the address in the first occurrence. about validating each tuple in the authorization list. It doesn't indicate we should throw an error but just proceed to the next tuple. That's a little obtuse but I'm assuming this means that when we are processing one of these txns in vm, we should only set the code for that authority if the tuple is valid. Is this method intended to serve that purpose or is this just a sanity check?

[jochem-brouwer]

No, it is used to verify that the authority list is properly formatted. It does not check the tuples, this is done in VM. For instance, it checks that the encoded fields are there, and they have no leading zeros in them.

[acolytec3]

Suggested change

	//@ts-ignore
	s: undefined,
	s: undefined as never,

Would prefer to cast to never over ts-ignore but just a nit.

[jochem-brouwer]

Aha, does that work? I did not know, will change.

[acolytec3]

Suggested change

	// @ts-ignore
	nonce: undefined,
	nonce: undefined as never,