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

VM v5 changes #767

Merged
merged 16 commits into from
Jun 10, 2020
Merged

VM v5 changes #767

merged 16 commits into from
Jun 10, 2020

Conversation

evertonfraga
Copy link
Contributor

@evertonfraga evertonfraga commented Jun 6, 2020

While reviewing, don't be scared by the amount of changes. Most of them were already reviewed in previous PRs. I advise looking at the commits' tab individually instead of the changes tab.

Below, a breakdown of the commits, showing their associated PRs and review state (βœ…/πŸ‘).

# PR Commit message
e78085e #682 βœ… block,blockchain,vm: Block release v3.0.0
ebe8dc7 #748 βœ… update to ethereumjs-util 7.0.2
909bf63 #748 βœ… ethereumjs-util BN ts build compatibility: remove use of IBN
83fcc18 #748 βœ… Use caret for ethereumjs-util
de9c2b7 #679 πŸ‘ feat: Scoped packages
Converts all ethereumjs-xxx packages of this monorepo to @ethereumjs/xxx

@holgerd77
Copy link
Member

Could you be more explicit here what parts here have already been reviewed? Also: if these have already been reviewed, wouldn't it be easier to merge these up-front into master instead of wrangling in here? I am always afraid on PRs like these since it is just extremely hard to keep oversight. Wouldn't it be easier to separate?

@evertonfraga
Copy link
Contributor Author

@holgerd77 of course! I have updated the PR description with the requested information. Now I'm separating it in two parts:

  1. v5 already approved prs that just need a quick check
  2. improvements in manageable-sized PRs.

@evertonfraga evertonfraga force-pushed the vm/release-v5 branch 2 times, most recently from de9c2b7 to d901eec Compare June 9, 2020 21:37
@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #767 into master will decrease coverage by 1.37%.
The diff coverage is 91.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #767      +/-   ##
==========================================
- Coverage   92.06%   90.68%   -1.38%     
==========================================
  Files          46       43       -3     
  Lines        3025     3018       -7     
  Branches      471      472       +1     
==========================================
- Hits         2785     2737      -48     
- Misses        145      188      +43     
+ Partials       95       93       -2     
Flag Coverage Ξ”
#account 94.11% <ΓΈ> (ΓΈ)
#block 82.68% <86.79%> (-5.68%) ⬇️
#blockchain 89.22% <85.71%> (+0.07%) ⬆️
#common 93.52% <ΓΈ> (-0.26%) ⬇️
#tx 94.02% <100.00%> (ΓΈ)
#vm 93.10% <100.00%> (ΓΈ)
Impacted Files Coverage Ξ”
packages/block/src/header-from-rpc.ts 66.66% <ΓΈ> (-20.84%) ⬇️
packages/vm/lib/evm/eei.ts 94.53% <ΓΈ> (ΓΈ)
packages/vm/lib/evm/interpreter.ts 98.18% <ΓΈ> (ΓΈ)
packages/vm/lib/evm/opcodes.ts 100.00% <ΓΈ> (ΓΈ)
packages/vm/lib/runBlockchain.ts 88.00% <ΓΈ> (ΓΈ)
packages/block/src/header.ts 66.94% <71.42%> (-21.59%) ⬇️
packages/blockchain/src/index.ts 87.79% <81.25%> (+0.07%) ⬆️
packages/block/src/block.ts 71.15% <88.23%> (-14.15%) ⬇️
packages/block/src/from-rpc.ts 86.66% <100.00%> (+0.45%) ⬆️
packages/blockchain/src/dbManager.ts 95.31% <100.00%> (+0.07%) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 47e8ca1...9f62112. Read the comment docs.

holgerd77 and others added 11 commits June 9, 2020 22:47
* tooling: adding script to inspect TS interface diffs

* tooling: refactoring script to improve usability
* block: version bump

* block: adds changelog from v2.2.1 to v3.0.0

* blockchain: integrating ethereumjs block 3.0.0

* blockchain: converting canonicalDifficulty to buffer

* blockchain: instantiating Block with empty/undefined data

* blockchain: integrating Block 3.0.0

* block: lint:fix changes

* All siloed tests passing.

* vm: lint fix

* leftovers of a rebase

* lint fix

* lint fix

* vm,block: integrating Block with Lerna

* block: remove circular dependency to ethereumjs-blockchain introduced in ethereumjs/ethereumjs-block#93

* block: removing unnecessary npx command

* block: lint fix

* blockchain: removing typo

* vm: fix BlockHeader import

* blockchain: fixing block initialization [Buffer[], Buffer[], Buffer[]] format

* vm: updating methods to async

* vm: misc changes

* block: describing recovery bit normalization statement

* block: removing source linking

* docs: update for all packages
* Readme package renaming

* account: renaming package

* block: Migrating to scoped packages

* npm package URL

* block: updating dependencies to scoped packages

* blockchain: migrating to scoped packages

* common: migrating to scoped packages

* tx: migrating to scoped packages

* vm: migrating to scoped packages

* account,block: updating badges

* vm: updating patch to scoped packages

* fix: updating new occurrences of package names

* fix: updating new occurrences of package names

* fix: rebase re-touching

* ci: updating Block ci file

downgrading nyc in block
@holgerd77
Copy link
Member

There are some failing tests here? Will wait with a review for now until further notice.

@evertonfraga
Copy link
Contributor Author

@holgerd77 yes, I am getting a MaxBufferError from Block tests, which I'm investigating now.

lerna ERR! MaxBufferError: stdout maxBuffer exceeded
lerna ERR!     at PassThrough.<anonymous> (/home/runner/.npm/_npx/3188/lib/node_modules/lerna/node_modules/get-stream/index.js:41:19)
lerna ERR!     at PassThrough.emit (events.js:327:22)
lerna ERR!     at addChunk (_stream_readable.js:297:12)
lerna ERR!     at readableAddChunk (_stream_readable.js:269:11)
lerna ERR!     at PassThrough.Readable.push (_stream_readable.js:214:10)
lerna ERR!     at PassThrough.Transform.push (_stream_transform.js:152:32)
lerna ERR!     at PassThrough.afterTransform (_stream_transform.js:96:10)
lerna ERR!     at PassThrough._transform (_stream_passthrough.js:46:3)
lerna ERR!     at PassThrough.Transform._read (_stream_transform.js:191:10)

@evertonfraga
Copy link
Contributor Author

evertonfraga commented Jun 10, 2020

@holgerd77 ready for review!

The second part of this PR will be Changelog, so don't expect them here.

Note: istead of the usual rebase I make, I had chosen to bring changes from master in a merge-commit. Given this PR size, I didn't want to risk doing surgery-like rebases.

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.

Phew, Everton, this is really impressive! πŸ˜„

I had a look at the commit history, skimmed over all non-scoped-packages PR additions (all look good) and then did a more thorough review on the scope package changes.

Some things to address.

@evertonfraga
Copy link
Contributor Author

@holgerd77 I think your comment was incomplete.

@evertonfraga
Copy link
Contributor Author

I have performed the changes requested in #679.

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.

Yes, live following, looks good now! πŸ˜› 😊

@evertonfraga evertonfraga merged commit 0b11b9a into master Jun 10, 2020
@evertonfraga evertonfraga deleted the vm/release-v5 branch June 10, 2020 20:31
@evertonfraga evertonfraga added this to the VM v5 milestone Jun 12, 2020
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.

None yet

3 participants