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

Updates to ethereumjs-config v2.0.0 (old) #886

Closed
wants to merge 44 commits into from

Conversation

evertonfraga
Copy link
Contributor

@evertonfraga evertonfraga commented Sep 21, 2020

This PR removes references to the old ethereumjs-config packages, replacing by the new ones. Configures all the new tooling.

This PR uses gitpkg urls to test monorepo package dependencies prior to their release.

Remaining checks:

  • Documentation to use unpublished packages with Verdaccio.

Fix lint results

Fix test runners

  • Account
  • Block
  • Blockchain
  • Common
  • Ethash
  • Tx
  • Vm

Other tasks

  • Fix coverage reporting for all packages
  • Fix bash errors on script
  • Point package references to NPM registry after Config release

@holgerd77
Copy link
Member

Seems to be pretty "en vogue" in the team these days to have no PR below 40 files changed. 😂 🤣 😅

@ryanio
Copy link
Contributor

ryanio commented Sep 21, 2020

haha oh yeah, careful with regenerating those docs too 😄

@evertonfraga
Copy link
Contributor Author

@holgerd77 🤣

I can move the docs to another PR, I think that's the most sensible thing to do.

@holgerd77
Copy link
Member

@evertonfraga I don't care too much, think we will also get this reviewed with the docs, whatever is more convenient for you.

@evertonfraga evertonfraga mentioned this pull request Sep 23, 2020
1 task
@evertonfraga evertonfraga force-pushed the update-ethereumjs-config branch 2 times, most recently from 099e073 to 3875a61 Compare October 8, 2020 07:41
@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #886 into master will decrease coverage by 1.27%.
The diff coverage is 90.47%.

Impacted file tree graph

Flag Coverage Δ
#account 90.00% <ø> (-2.86%) ⬇️
#block 75.62% <86.36%> (-1.31%) ⬇️
#blockchain 79.80% <100.00%> (-0.34%) ⬇️
#common 92.03% <100.00%> (-0.98%) ⬇️
#ethash 81.25% <100.00%> (-0.98%) ⬇️
#tx 91.79% <100.00%> (-1.30%) ⬇️
#vm 87.06% <87.50%> (-1.11%) ⬇️

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

@evertonfraga evertonfraga marked this pull request as ready for review October 8, 2020 08:20
@evertonfraga
Copy link
Contributor Author

After review, I'll release the ethereumjs-config 2.0.0, then update here on this branch and merge it!

@holgerd77 holgerd77 mentioned this pull request Oct 8, 2020
39 tasks
holgerd77
holgerd77 previously approved these changes Oct 8, 2020
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.

Nothing to hold off the merge, looks good, thanks Everton for the great work on this!

{
"semi": false,
"singleQuote": true
}
Copy link
Member

Choose a reason for hiding this comment

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

No merge blocker, but just for understanding: why are these settings added separately and not as some general configuration?

Copy link
Contributor Author

@evertonfraga evertonfraga Oct 8, 2020

Choose a reason for hiding this comment

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

@holgerd77 So the linting process can occur independently on the CI and locally, for each package.

I understand the repetition, and I can look into alternatives, but I'd rather play on the safer side first, where we can have more granular control over the ethereumjs-config packages

"compilerOptions": {
"outDir": "./dist.browser"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

These new browser builds needs also be anchored in package.json I guess - see README from config PR, can be tackled in a separate PR though as well eventually.

Copy link
Member

Choose a reason for hiding this comment

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

Update: have added this to the V5 Todo list, so this won't be forgotten.

Copy link
Member

Choose a reason for hiding this comment

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

I would pass on this whole task to @cgewecke - so to eventually integrate and to check that these two new build targets are working as expected and eventually give side effects another thought - if both of you are ok with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@holgerd77 I am OK with it, and can follow up with @cgewecke on that.

'no-unused-vars': 'off',
'prefer-const': 'off',
'sonarjs/no-duplicated-branches': 'off',
}
Copy link
Member

Choose a reason for hiding this comment

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

Ah, so many exceptions for the VM necessary? 😄

No problem of course on this round. Nevertheless interesting to state and see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! there were so many eslint complaints. I decided to add those as exceptions for now, and ship the config sooner, as it's working perfectly.

We can tackle those in other PRs.

this.refundGas(new BN(this._common.param('gasPrices', 'selfdestructRefund')))
this.refundGas(
new BN(this._common.param('gasPrices', 'selfdestructRefund'))
)
Copy link
Member

Choose a reason for hiding this comment

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

I actually liked the printWidth setting being extended to 100 lines (from 800 or so) on the old format package, this gives a bit more space and there was a somewhat longer discussion around this during the introduction of config until we decided upon this.

This seems to not have made it, was this intentional? (at least I am assuming during re-formattings like the above, haven't concretely checked).

Not sure, should we keep / post-introduce this, any opinions @cgewecke @ryanio or @jochem-brouwer ?

@evertonfraga in doubt and if too much process interrupting merge here anyhow, we'll get this settled out later, eventually with some follow-up PR after discussion just with the linting updates.

"@ethereumjs/account": "^3.0.0",
"@ethereumjs/block": "^3.0.0",
"@ethereumjs/blockchain": "^4.0.3",
"@ethereumjs/common": "^1.5.1",
"@ethereumjs/tx": "^2.1.2",
"async-eventemitter": "^0.2.4",
"core-js-pure": "^3.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

What are these two new dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@holgerd77 they were already there. I have just moved them to the correct order (@, A-Z)

holgerd77
holgerd77 previously approved these changes Oct 8, 2020
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.

Reapproval after new commits.

@holgerd77
Copy link
Member

@evertonfraga Thanks for the latest updates here, looks great 🙂 , let me know once you need a review!

@holgerd77 holgerd77 changed the title Updates to ethereumjs-config v2.0.0 Updates to ethereumjs-config v2.0.0 (old) Oct 16, 2020
@holgerd77
Copy link
Member

Superseded by #913, will close.

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

Successfully merging this pull request may close these issues.

3 participants