A possible simplification of this library

[alcuadrado]

I created this draft PR to show one possibility of what can be done in response to what we are discussing in #151.

I based this PR in #149, as doing so makes things easier.

Note that this is just a proof of concept and I haven't updated the tests, nor haven't paid much attention to this refactor's correctness yet.

[alcuadrado]

I kept working on this and made it pass all tests, with the exception of FakeTransaction and its tests.

I copied part of the behavior of the previous implementation to make the tests pass, and I'm now completely puzzled about a few things:

• Fields definitions and their validations seem mostly random. Most of them accept buffers up to 32 bytes long, but to, data, and v work differently. Does anyone know why? Was this an implementation detail? Or is this part of the consensus rules?

• Most fields have their zeros to the left stripped on assignment. I moved this behavior to strip them where needed, but I'm also doubting if this is required, or when. I expected to be able to strip all of them, but I believe to, data, and v shouldn't be stripped.

I think this proposal may look good if these two things are figured out.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 7d16f0133c9405428464b6cd0850c230dec83def into ed4b695869d23bd377a2b5a5a263b776e8e513b4 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

---

Comment posted by LGTM.com

[s1na]

Thanks for pursuing this! I think this is a great start. A few comments:

1. I overheard in #AllCoreDevs that folks are considering modifying the tx format for the Istanbul hardfork. I'd suggest we wait a bit before committing to a new design.
2. I liked the static factory methods, and making things like unsigned eip155 message, etc. explicit, and it actually gave me another idea (haven't thought about it in detail): having a tx interface, multiple tx implementations and a factory class which instantiates a tx implementation based on given data. The implementations are something like BaseUnsignedTx, BaseSignedTx, EIP155UnsignedTx, etc. (they can inherit most of logic from eachother). Users shouldn't know which implementation they're interacting with.
3. I'm myself for immutability, but an argument against it (in the case of this PR) is that fields like this._from and this._senderPubKey act as a cache, avoiding the costly call to getSenderPublicKey and ecrecover, and I know that at least the VM tries to get the sender address multiple times in a row. Although this caching can be pushed to e.g. to VM, I'd be okay with that.
4. Regarding your questions, I'm not sure myself and would have to investigate. But one way of finding out whether they're consensus rules is to integrate the changes in VM and see if tests start to fail.

[alcuadrado]

1. I overheard in #AllCoreDevs that folks are considering modifying the tx format for the Istanbul hardfork. I'd suggest we wait a bit before committing to a new design.

Sure. As mentioned in #153, this PR is mostly to discuss possible alternatives. No need to rush it.

2. I liked the static factory methods, and making things like unsigned eip155 message, etc. explicit, and it actually gave me another idea (haven't thought about it in detail): having a tx interface, multiple tx implementations and a factory class which instantiates a tx implementation based on given data. The implementations are something like BaseUnsignedTx, BaseSignedTx, EIP155UnsignedTx, etc. (they can inherit most of logic from eachother). Users shouldn't know which implementation they're interacting with.

This is possible if we go for a more immutable alternative. Things like tx.sign(pk) can't work this way. This would work const signedTx : Transaction = unsignedTx.sign(pk). (note: The type annotation isn't needed)

TBH I'm not a big fan of classic OOP design, where business logic is encoded in the class hierarchy. My feeling is that it prioritizes code simplicity at the expense of making the big picture harder to see and comprehend. I don't hate it either, and there are places where it makes sense, so I think it's an alternative worth exploring.

3. I'm myself for immutability, but an argument against it (in the case of this PR) is that fields like this._from and this._senderPubKey act as a cache, avoiding the costly call to getSenderPublicKey and ecrecover, and I know that at least the VM tries to get the sender address multiple times in a row. Although this caching can be pushed to e.g. to VM, I'd be okay with that.

I just removed it temporarily to make things easier. I'm ok with caching things if they are common and slow operations. Without defineProperties it's easier to implement the cache correctly.

4. Regarding your questions, I'm not sure myself and would have to investigate. But one way of finding out whether they're consensus rules is to integrate the changes in VM and see if tests start to fail.

Section 4.2 of the yellow paper defines this. I barely skimmed it for now. Will read it in detail later.

[evertonfraga]

This repo is moving to a monorepo structure. Please keep any work related to this PR there: https://github.com/ethereumjs/ethereumjs-vm/