-
Notifications
You must be signed in to change notification settings - Fork 773
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
Block release v3.0.0 #682
Block release v3.0.0 #682
Conversation
Codecov Report
@@ Coverage Diff @@
## vm/release-v5 #682 +/- ##
=================================================
- Coverage 91.69% 90.34% -1.35%
=================================================
Files 47 43 -4
Lines 3011 3005 -6
Branches 469 469
=================================================
- Hits 2761 2715 -46
- Misses 151 194 +43
+ Partials 99 96 -3
Continue to review full report at Codecov.
|
@@ -61,6 +60,7 @@ function normalizeTxParams(_txParams: any) { | |||
// strict byte length checking | |||
txParams.to = txParams.to ? ethUtil.setLengthLeft(ethUtil.toBuffer(txParams.to), 20) : null | |||
// v as raw signature value {0,1} | |||
txParams.v = txParams.v < 27 ? txParams.v + 27 : txParams.v | |||
const v: number = txParams.v | |||
txParams.v = v < 27 ? v + 27 : v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is happening here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tsc
fix. This whole method needs a refactor to remove the magic numbers and is on my to-do list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@holgerd77 I added a more descriptive comment to explain this magic number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evertonfraga looks great, would make for a good trivia question 😄
@@ -314,7 +312,7 @@ export default class Blockchain implements BlockchainInterface { | |||
* @hidden | |||
*/ | |||
_setCanonicalGenesisBlock(cb: any): void { | |||
const genesisBlock = new Block(null, { common: this._common }) | |||
const genesisBlock = new Block(undefined, { common: this._common }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for understanding: was this a necessary change which triggered some test failure or something before respectively came along with some code changes in v3.0.0
? Or is this just an adjustment to make things cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This first round of changes is to comply with the linter and tsc
. There were several compilation failures and Blockchain code was not functional when bumping Block to 3.0.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this specific case, null
is not informed as a valid type in the Block constructor()
.
constructor(
data: Buffer | [Buffer[], Buffer[], Buffer[]] | BlockData = {},
opts: ChainOptions = {},
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention this in the v3.0.0
release notes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@holgerd77 I am leaning towards allowing null
in Block constructor. What are your thoughts?
I can then revert the null
to undefined
changes made in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also have a tendency to keep it "as is".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it. working on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. In the end, I have changed my mind regarding this.
The TypeScript community in general is not much of a fan of null
values representing undefined
values. In fact, this is a broad issue on how null
is much often misused.
So, as the default code from TypeScript version of Block wasn't allowing null
in their signature, I stick to it, and for JavaScript consumers, though, I added this check, which makes sure the method will behave as expected, so if a JS code initializes Block with data === null
, data
will become its null-object equivalent with value {}
.
The impact in our TS code of using new Block(undefined, {})
is super low, only used in Block tests.
6d32754
to
334e248
Compare
4a540c6
to
a526d49
Compare
43aaefa
to
e11b29a
Compare
@@ -54,7 +54,6 @@ | |||
"@types/node": "^11.13.4", | |||
"@types/tape": "^4.2.33", | |||
"browserify": "^16.2.3", | |||
"ethereumjs-blockchain": "^4.0.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed this circular dependency
"bootstrap": { | ||
"ignore": "ethereumjs-block" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ethereumjs-block is now integrated to Lerna workflow, so this change removes it from the ignore list for the bootstrap command.
@@ -11,8 +11,9 @@ | |||
} | |||
}, | |||
"scripts": { | |||
"bootstrap": "lerna bootstrap --ignore-scripts && lerna exec npm i --scope=ethereumjs-block", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lerna bootstrap
now installs packages for ethereumjs-block as well, so removing npm i
here.
|
||
[3.0.0]: https://github.com/ethereumjs/ethereumjs-vm/compare/%40ethereumjs%2Fblock%402.2.0...%40ethereumjs%2Fblock%403.0.0 | ||
|
||
## [2.2.2] - 2019-12-17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
picked 3.0.0, 2.2.1 and 2.2.2 changelog entries from its own branches in ethereumjs-block. Modified a bit 3.0.0 text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great 👍
@@ -24,21 +134,21 @@ PR [#64](https://github.com/ethereumjs/ethereumjs-block/pull/64) | |||
- Remove `ethereumjs-testing` dependency (much smaller dev dependencies), | |||
PR [#61](https://github.com/ethereumjs/ethereumjs-block/pull/61) | |||
|
|||
[2.2.0]: https://github.com/ethereumjs/ethereumjs-vm/compare/%40ethereumjs%2Fvm%402.1.0...%40ethereumjs%2Fvm%402.2.0 | |||
[2.2.0]: https://github.com/ethereumjs/ethereumjs-vm/compare/%40ethereumjs%2Fblock%402.1.0...%40ethereumjs%2Fblock%402.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed comparison link, somehow the sed
command used during the migration substituted it erroneously.
|
||
## [2.1.0] - 2018-10-19 | ||
|
||
- **Constantinople** support, added difficulty bomb delay (EIP-1234), PR [#54](https://github.com/ethereumjs/ethereumjs-block/pull/54) | ||
- Updated test data, added Constantinople tests, PR [#56](https://github.com/ethereumjs/ethereumjs-block/pull/56), [#57](https://github.com/ethereumjs/ethereumjs-block/pull/57) | ||
- Added `timestamp` field to `setGenesisParams()`, PR [#52](https://github.com/ethereumjs/ethereumjs-block/pull/52) | ||
|
||
[2.1.0]: https://github.com/ethereumjs/ethereumjs-vm/compare/%40ethereumjs%2Fvm%402.0.1...%40ethereumjs%2Fvm%402.1.0 | ||
[2.1.0]: https://github.com/ethereumjs/ethereumjs-vm/compare/%40ethereumjs%2Fblock%402.0.1...%40ethereumjs%2Fblock%402.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
monorepo migration fix.
|
||
## [2.0.1] - 2018-08-08 | ||
|
||
- Fixes `BlockHeader.prototype.validate()` bug, see PR [#49](https://github.com/ethereumjs/ethereumjs-block/pull/49) | ||
|
||
[2.0.1]: https://github.com/ethereumjs/ethereumjs-vm/compare/%40ethereumjs%2Fvm%402.0.0...%40ethereumjs%2Fvm%402.0.1 | ||
[2.0.1]: https://github.com/ethereumjs/ethereumjs-vm/compare/%40ethereumjs%2Fblock%402.0.0...%40ethereumjs%2Fblock%402.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
monorepo migration fix.
- Correct block validation for all know hardforks, PR | ||
[#47](https://github.com/ethereumjs/ethereumjs-block/pull/47), if no hardfork is set validation logic | ||
is determined by block number in combination with the `chain` set | ||
- Genesis block initialization depending on the `chain` set (see `ethereumjs-common` for supported chains) | ||
- Extensive test additions to cover the newly introduced capabilities and changes | ||
- Fix default value for `nonce` (empty buffer -> `<Buffer 00 00 00 00 00 00 00 00>`), PR [#42](https://github.com/ethereumjs/ethereumjs-block/pull/42) | ||
|
||
[2.0.0]: https://github.com/ethereumjs/ethereumjs-vm/compare/%40ethereumjs%2Fvm%401.7.1...%40ethereumjs%2Fvm%402.0.0 | ||
[2.0.0]: https://github.com/ethereumjs/ethereumjs-vm/compare/%40ethereumjs%2Fblock%401.7.1...%40ethereumjs%2Fblock%402.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
monorepo link migration fix.
@@ -52,23 +162,23 @@ Changes in detail: | |||
- New initialization parameters `opts.chain` (default: `mainnet`) and `opts.hardfork` | |||
(default: `null`, block number-based behaviour), PR [#44](https://github.com/ethereumjs/ethereumjs-block/pull/44) | |||
- Alternatively a `Common` class object can be provided directly with the `opts.common` parameter, | |||
see [API](https://github.com/ethereumjs/ethereumjs-block/blob/master/docs/index.md) docs | |||
see [API](https://github.com/ethereumjs/ethereumjs-vm/blob/master/packages/block/docs/index.md) docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
migrating link to monorepo structure.
@@ -10,7 +10,7 @@ | |||
"scripts": { | |||
"build": "ethereumjs-config-build", | |||
"prepublishOnly": "npm run test && npm run build", | |||
"coverage": "npx nyc npm run test:node", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npx
is irrelevant here as nyc
is listed as devDependency
.
@@ -20,7 +20,7 @@ | |||
"lint": "ethereumjs-config-lint", | |||
"lint:fix": "ethereumjs-config-lint-fix", | |||
"test": "npm run test:node && npm run test:browser", | |||
"test:node": "npx tape -r ts-node/register test/*.spec.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npx
is irrelevant here as tape
is listed as devDependency
.
@@ -70,8 +69,5 @@ | |||
"typedoc-plugin-markdown": "^2.2.17", | |||
"typescript": "^3.4.3", | |||
"typestrict": "^1.0.2" | |||
}, | |||
"publishConfig": { | |||
"directory": "src" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do the direct typescript code linking yet without handling the browser tests in a cross-package scope.
st.end() | ||
}) | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding some basic initialization tests
let body | ||
try { | ||
body = await this.getBody(hash, number) | ||
} catch (e) { | ||
body = [[], []] | ||
} | ||
|
||
return new Block([header].concat(body), { common: this._common }) | ||
const blockData = [header].concat(body) as [Buffer[], Buffer[], Buffer[]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TS compiler was erroring out on this block of code.
### Promise-based API | ||
|
||
The API of this library is now completely promise-based, the old callback-style | ||
interface has been dropped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Thanks for the review, @ryanio! |
// Checking at runtime, to prevent errors down the path for JavaScript consumers. | ||
if (data === null) { | ||
data = {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great
const Ethash = require('ethashjs') | ||
const Stoplight = require('flow-stoplight') | ||
const level = require('level-mem') | ||
const semaphore = require('semaphore') | ||
|
||
export type Block = any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
@@ -470,28 +468,29 @@ export default class Blockchain implements BlockchainInterface { | |||
|
|||
async.series( | |||
[ | |||
verify, | |||
async.asyncify(async function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a full refactor should be done to simplify all these mixed async
methods. But that's already planned :)
@@ -182,6 +182,8 @@ function runTests(name, runnerArgs, cb) { | |||
const runner = require(`./${name}Runner.js`) | |||
// Tests for HFs before Istanbul have been moved under `LegacyTests/Constantinople`: | |||
// https://github.com/ethereum/tests/releases/tag/v7.0.0-beta.1 | |||
|
|||
// TODO: Replace with Common.lteHardfork('Istanbul') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 anything blocking this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no! but I'd rather make in a separate PR, this one is beyond big. It should be handled with care, testing several test scopes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow looks great, nice work on everything that needed to happen for this integration @evertonfraga 👍
Whew, this is really a biggy! 😁 Congrats on this, feel free to merge in from my side as well. |
* 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
* 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
* 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
This PR initially supersedes ethereumjs/ethereumjs-block#74. Given that this is now a monorepo, I aim to perform the respective integrations in downstream libraries (see diagram below).
Closes #680