Tx Constructor Restructuring

[holgerd77]

This PR is more of a suggestion and a base for discussion than a concrete PR. It changes the tx constructor to directly make use of the TxData interface object instead of taking in the separate values. I came here after further thinking about class inheritance on Tx along reviewing the Object.freeze issue #940 from @cgewecke and generally having another look at the tx constructor we have right now.

I think with this change it becomes a lot easier and will be cleaner for people to subclass the tx class - e.g. when they want to add their own tx values they can just build upon the current interface infrastructure and extend the TxData interface instead of squeezing in their own values in the subclass constructor respectively generally have to repeat all the value parameters once again in their own constructors.

The Tx.fromTxData() factory method in fact becomes simply an alias for the constructor. While this might feel a bit sobering on first sight, I would generally feel safer with this solution, to no have a main constructor in place which just should not be used (but likely will be in several cases) and is therefore eventually not cared for too much. Especially since there is also simply no loss on functionality with this change but just an internal restructuring taking place.

I also generally perceive this constructor cleaner as before not having the problem of this always a bit unlucky situation with optional values (v, r, s) one might not want to pass followed by a value (opts) which independently often needs to be passed in. This always leads to this bridging situation where people just quickly fill in some placeholder for the not-wanted values (eventually with something not intended like null instead of undefined) just to get to the last entry.

@ryanio, also @cgewecke (and others), what do you think? Does this solution go along with downsides I am currently not seeing or this a change we should make before releasing?

[codecov]

Codecov Report

Merging #944 (39e37cc) into master (d7d9123) will decrease coverage by 0.39%.
The diff coverage is 94.11%.

Flag	Coverage Δ
block	75.04% <ø> (-0.33%)	⬇️
blockchain	77.39% <ø> (ø)
common	91.87% <ø> (-0.25%)	⬇️
ethash	82.08% <ø> (ø)
tx	86.25% <94.11%> (-2.38%)	⬇️
vm	87.21% <ø> (ø)

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

[cgewecke]

@holgerd77

Yes, this makes a lot of sense to me. It definitely makes the constructor itself easier to work with.

[holgerd77]

Ok, this is now ready for review, respectively can be more or less just approved since both @cgewecke and @ryanio already gave their ok on this. 😄

Please ignore the failing API test displayed, all API tests are passing, this is just a linter problem which is currently discussed separately.

[jochem-brouwer]

Approving this since it was already approved by others (but LGTM as well!) 😄

[holgerd77]

Ok, great 😄 , with this we've got everything for the next beta releases I would assume. I will now target a release for tomorrow or Thursday. If this is important for you and you need this quickly for future work we can still try to get in #935 for rc.1 early next week.

[holgerd77]

@jochem-brouwer I think you can close to always merge my PRs if needed