Skip to content
This repository has been archived by the owner on Apr 6, 2020. It is now read-only.

Update common resources dependency #130

Merged

Conversation

youfoundron
Copy link
Contributor

Replaces deprecated dependency ethereum-common with ethereumjs-common.

@coveralls
Copy link

coveralls commented Dec 4, 2018

Coverage Status

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

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @youfoundron, thanks for the PR, very much appreciated to have the update on the ethereumjs-common library. However this is completely ignoring the official API of the library, it is not intended to directly go through to the constants.

Can you please access the parameters through the param() method and do the hardfork block comparison through the respective API method?

This would then usefully also go along with an update on the instantiation API, see the block library for reference how this is done. Can you please do that along?

We should then also thing along with this if we need restrictions on network and hardfork (see e.g. the VM how restrictions can be imposed) or if we just can allow everything here.

Cheers
Holger

@youfoundron
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Member

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

danjm
danjm previously requested changes Jan 4, 2019
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test/transactionRunner.js Show resolved Hide resolved
danjm added a commit that referenced this pull request Jan 4, 2019
@danjm
Copy link
Contributor

danjm commented Jan 4, 2019

@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.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally pretty good, I have left some more comments.

There is this one extra failing test 119 on the CI run, can't really judge this, any idea where this is coming from?

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test/transactionRunner.js Show resolved Hide resolved
@youfoundron
Copy link
Contributor Author

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

@holgerd77
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

youfoundron commented Feb 4, 2019

@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
Copy link
Member

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
Copy link
Member

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.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, great, thanks a lot for generally take on this and so quickly re-take and finish! Super cool! 👍 😄

@holgerd77 holgerd77 dismissed danjm’s stale review February 6, 2019 19:02

All review comments addressed.

@holgerd77 holgerd77 merged commit 471a13e into ethereumjs:master Feb 6, 2019
@youfoundron youfoundron deleted the update-common-resources-dependency branch February 6, 2019 19:19
danjm added a commit that referenced this pull request Mar 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants