search

ethereumjs / ethereumjs-tx

Project is in active development and has been moved to the EthereumJS VM monorepo.
https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/tx
Mozilla Public License 2.0
779 stars 235 forks source link

Update common resources dependency #130

Closed youfoundron closed 5 years ago

youfoundron commented 5 years ago

Replaces deprecated dependency ethereum-common with ethereumjs-common.

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-1.02%) to 97.778% when pulling b3cc6b4df1ec07065dce2d1c5a644b61421b1a08 on youfoundron:update-common-resources-dependency into 40551b01045f888c0693e63fb2a9a630567519d0 on ethereumjs:master.

youfoundron commented 5 years ago

@holgerd77

Lol I'm not sure how I didn't catch that there was a whole API in the docs, thought I this was basically a repository of config files.

I've updated PR to use params, will look into the block library shortly.

holgerd77 commented 5 years ago

Hi @youfoundron, this already looks much better, thanks for the update! :-)

Actually we had various updates along this line in the block but also in the blockchain and VM repos adding network and hardfork support to the libraries by expanding the external constructor API (for both normal index.js and fake fake.js transaction) and allowing to pass either chain and hardfork parameter in a separate additional optsdict in the constructor (for explicitly fixing both parameters) or alternatively having a common instance passed (e.g. for implicitly taking the values from a block).

I think this would be the way to go here as well, would you be willing to go along this extra mile? Would be cool! 😄

For the API this should be very analogue to the code (see also the JSDoc comments along) in the block library.

In the past we have always somewhat implicitly updated our libraries to the latest hardfork rules. So for convenience and to remain compatibility hardfork should probably also default to byzantium for now like being done here in the VM.

Main indicator that a post-chain start version of tx is taken as default in the library (I am also not so totally deep into the tx library) is that the current homestead flag is set to true by default. This whole flag should be removed, since this is replaced in a more general way through the hardfork setting. All other references to the flag should be replaced with _common.gteHardfork('...') comparisons similar to here in the VM (so logic from the method is "if the hardfork set in common is greater or equal than the hardfork stated do x"). Probably it is safe to use homestead here, maybe we should nevertheless have a closer look if it was really homestead which introduced the respective changes (e.g. I don't see any new gas price changes in the homestead.json file in the common library. This might very well also be an inconsistency/bug in the common library though).

I've seen that you already gave some thumbs up on the comment on the tests PR from @danjm, would be great if you guys could coordinate on that, than Dan can directly use the result from here for his test PR.

holgerd77 commented 5 years ago

Thanks for this, this looks already really good! 😄 Might take some days till review.

danjm commented 5 years ago

@youfoundron this looks good! I have left some comments for some small changes.

@holgerd77 I have reviewed this as I have been further refining the test rewrite PR. My latest commit to that PR is compatible with the changes introduced here, and can be rebased once this is merged.

I merged both PRs locally and tested them along with further changes we need for "Spurious Dragon" compatibility (for which I will make another PR). Everything seems to be working together smoothly.

youfoundron commented 5 years ago

@holgerd77 @danjm Great feedback. Will review this weekend.

holgerd77 commented 5 years ago

Hi @youfoundron, if you find some time to finish the work here and include the changes that would be great, other work is depending on this PR to be merged. 😄

If not - no problem - but please let us know as well, than we can think of alternative ways how we move forward here.

Cheers Holger

holgerd77 commented 5 years ago

Hi @youfoundron, thanks for the great work so far! 😄 This PR is currently blocking other work, would it be ok for you if @danjm re-pushes your state of the work directly to the repository and does the latest changes required?

youfoundron commented 5 years ago

@holgerd77 I've made the requested changes. Any idea what is up with tests failing? Not certain what to make of logs in CircleCI.

holgerd77 commented 5 years ago

Great, thanks for the updates!

Hmm, not sure if this was simply some Travis malfunctioning, since on the job run I checked output just stopped at some point. Have retriggered the failing jobs, sometimes these pass on a second run (and it can be relatively clearly attributed to Travis failing and not the tests).

holgerd77 commented 5 years ago

Failure can only be seen on Travis in the raw log, since output is so long and therefore cut off. String10MbData is the (first) test failing.