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

VM: Replace Class-Referencing Tx Typing #3575

Closed
holgerd77 opened this issue Aug 9, 2024 · 5 comments · Fixed by #3601
Closed

VM: Replace Class-Referencing Tx Typing #3575

holgerd77 opened this issue Aug 9, 2024 · 5 comments · Fixed by #3601

Comments

@holgerd77
Copy link
Member

The VM in runTx() is drawing everything from tx in, for all tx types, even if just one specifc tx type is used (the following is from a simple legacy tx run).

grafik

What is generally structurally suboptimal and what is somewhat likely the cause here is that we type the tx by this class-referencing Transaction interface (in types.ts in tx):

export interface Transaction {
  [TransactionType.Legacy]: LegacyTransaction
  [TransactionType.FeeMarketEIP1559]: FeeMarketEIP1559Transaction
  [TransactionType.AccessListEIP2930]: AccessListEIP2930Transaction
  [TransactionType.BlobEIP4844]: BlobEIP4844Transaction
  [TransactionType.EOACodeEIP7702]: EOACodeEIP7702Transaction
}

So, e.g. FeeMarketEIP1559Transaction is just the class.

We do have a separate type structure, fully interface based in tx, with e.g. TransactionInterface and sub (?) interfaces EIP1559CompatibleTx.

This structure should - likely ? - be used instead and the Transaction interface removed.

This whole thing likely needs validation, I am not 100% sure. Just to note that this is not optimal yet, eventually this is a solution (or: improvement).

The whole thing might also go together with a further tx refactoring (which - if done - needs to be thought through really thoroughly).

@ScottyPoi
Copy link
Contributor

I worked on this a bit. At first it seemed like it would be fairly straightforward to replace Transaction[T] with TransactionInterface<T>.

Where it gets messy is where we had Transaction[T] as a return type for the transaction factory functions. I think there is some ambiguity in the way our interfaces are built, or the conditionals in the functions aren't explicit enough, because I can't use TransactionInterface<T> as a return type as its currently written without a bunch of as unknown helpers in there.

@ScottyPoi
Copy link
Contributor

ScottyPoi commented Aug 13, 2024

I think if we were to upgrade our version of Typescript, we would be able do do some clever things with interfaces that we can't do with our current version.

Currently we can't override attributes when we extend interfaces, but that is a feature of more current versions of typescript. If we can override the type attribute as we extend the TX data and TX interfaces, then functions that use the generic interfaces can infer the correct return type.

@holgerd77
Copy link
Member Author

Yeah, be careful with this task respectively this is definitely not a simple side task, this goes deep into the tx library and likely (guess as you also proofed) touches the current suboptimalities at its core.

So if you want to have further look, maybe don't try to "solve" it, have somethin ready or so, but rather put the effort on finding things out/getting some steps further, by reporting on the problems which arise, so that we can evolve here together.

Respectively this might also be some working towards @jochem-brouwer, who wants to do some broader continued/evolved refactoring, so that he might be able to integrate stuff/get ideas/bring things a bit more together.

@holgerd77
Copy link
Member Author

Also side comment: it is definitely desired that we go to a more up-to-date TypeScript version, so that would definitely be a good task "on its own" (so: without any non-necessary code improvements in the same PR).

So if you want to do that you are very welcome! 🙂 (if you do not, can you please open an issue, so that we keep having this on the plate?)

Maybe it's advisable to not directly go to 5.5 but make one or two in-between steps to not have to deal with too much stuff at once? 🤔 But also do not have a good feeling on how disruptive TypeScript releases really are!

Before we jump on new fancy TypeScript features we should also have a short exchange in the team chat what the effects might be, respectively always a good idea to google a bit, just as a side note.

@holgerd77
Copy link
Member Author

Ok, from a bundling perspective this is not a problem respectively was not the cause, I solved this in #3601, see initial notes for details. I will close here, since generally not sure if worth a rewrite, also since this issue was written from a bundling perspective.

if @jochem-brouwer still wants to change typing along his tx refactor this can for sure still be done, but this will then likely evolve from working with the libraries and not from a top-to-bottom issue.

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