-
Notifications
You must be signed in to change notification settings - Fork 753
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
Promisify blockchain and ethash packages #833
Conversation
75d3a35
to
4005168
Compare
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
This comment has been minimized.
This comment has been minimized.
4005168
to
38af987
Compare
Whew, that's a big one. Really great! 😄 |
38af987
to
ad9e983
Compare
ad9e983
to
b4e2b76
Compare
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 is really the most wonderful PR I've seen in a while! 😍
I would regard everything from blockchain except the tests as reviewed, have spent a whole 2 hours on this, everyone of course feel free to have a second look.
Some things to address up till now, nothing grave though.
@jochem-brouwer just as some suggestion: if you want to get deeper into the client library, integrating the changes from here might be a good starter.
packages/blockchain/src/dbManager.ts
Outdated
@@ -1,4 +1,6 @@ | |||
import * as rlp from 'rlp' | |||
import { Block, BlockHeader } from '@ethereumjs/block' | |||
import Common from '@ethereumjs/common' | |||
import Cache from './cache' | |||
import { | |||
headsKey, |
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.
Side note, totally unrelated to the code changes done in the PR, just not down since I stumbled upon during reading the code, maybe someone wants to change along a future PR on the library: I could better read/understand the related code parts if these constants from util
like headsKey
were capitalized (e.g. HEADS_KEY
). Now I was irritatedly searching for some class property or local variable with headsKey
and others written "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.
good idea, fixed in c4992ff
packages/blockchain/src/dbManager.ts
Outdated
@@ -175,7 +202,7 @@ export default class DBManager { | |||
/** | |||
* Performs a batch operation on db. | |||
*/ | |||
batch(ops: Array<any>): Promise<any> { | |||
return this._db.batch(ops) | |||
batch(ops: DBOp[]) { |
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.
So this is not an async operation and the return Promise
was a mistake before? 🤔
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.
added in e6e6487
packages/blockchain/src/dbManager.ts
Outdated
return this.get(headBlockKey) | ||
} | ||
|
||
/** | ||
* Fetches a block (header and body), given a block tag | ||
* which can be either its hash or its number. | ||
*/ | ||
async getBlock(blockTag: Buffer | BN | number): Promise<any> { | ||
async getBlock(blockTag: Buffer | BN | number): Promise<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.
Not directly change-related, just realizing that I don't like the name blockTag
here. A tag has a different relationship to objects with the same tag being allowed to be associated with different objects.
So I would find blockIdentifier
better fitting 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.
agree here and didn't find any use of "blockTag" outside of the blockchain package so have renamed in 8a2e714
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 actually looks good.
🦋 🐹 🐝
Will merge.
if (blocks.length < 10) { | ||
addNextBlock(blockNumber + 1) | ||
} else { | ||
st.equal(blocks.length, 10) |
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.
Uh, that was a nice JavaScript Array test before. 🤣
|
||
resolve(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.
For the record: this is changing the API of the block
library in the form that validate()
- calling into _getBlockByHash()
now only works with the yet to be released promisifed Blockchain
version.
Wonder if this would be a good case for adding a peerDependency
(see Peer Dependencies | Node.js) entry in the Block
package.json
referencing the Blockchain
version? // cc @alcuadrado
Do we want to do a more systematic approach and generally introduce peerDependency
references within the monorepo scope? These inter-library compatibility insecurities are a reoccurring problem.
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.
If someone reading this think this is a good idea eventually directly open an issue on this, optimally (but leave if no time) directly with a TODO list with the listed directed references/dependencies.
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 should also be added as a linked reference to the v5 release TODO list (if we agree upon), since it would make very much sense to add this along the new releases.
@@ -125,11 +125,11 @@ export default class Ethash { | |||
/** | |||
* Loads the seed and cache given a block number. | |||
*/ | |||
loadEpoc(number: number, cb: Function) { | |||
async loadEpoc(number: 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.
Functions could also need some return type, won't make this a blocker though.
|
||
_verifyPowAsync() | ||
}) | ||
return true | ||
} | ||
} |
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.
Ethash
changes look good.
{ | ||
"validBlockRlp": "f90667f905fba0a8d5b7a4793baaede98b5236954f634a0051842df6a252f6a80492fd888678bda01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347948888f1f195afa192cfee860698584c030f4c9db1a0f93c8db1e931daa2e22e39b5d2da6fb4074e3d544094857608536155e3521bc1a0bb7495628f9160ddbcf6354380ee32c300d594e833caec3a428041a66e7bade1a0c7778a7376099ee2e5c455791c1885b5c361b95713fddcbe32d97fd01334d296b90100000000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000008000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000400000000000000000000000000000000000000000000000000000008302000001832fefd882560b84559c17b9b9040001020304050607080910111213141516171819202122232410000000000000000000200000000000000000003000000000000000000040000000000000000000500000000000000000006000000000000000000070000000000000000000800000000000000000009000000000000000000010000000000000000000100000000000000000002000000000000000000030000000000000000000400000000000000000005000000000000000000060000000000000000000700000000000000000008000000000000000000090000000000000000000100000000000000000001000000000000000000020000000000000000000300000000000000000004000000000000000000050000000000000000000600000000000000000007000000000000000000080000000000000000000900000000000000000001000000000000000000010000000000000000000200000000000000000003000000000000000000040000000000000000000500000000000000000006000000000000000000070000000000000000000800000000000000000009000000000000000000010000000000000000000100000000000000000002000000000000000000030000000000000000000400000000000000000005000000000000000000060000000000000000000700000000000000000008000000000000000000090000000000000000000100000000000000000001000000000000000000020000000000000000000300000000000000000004000000000000000000050000000000000000000600000000000000000007000000000000000000080000000000000000000900000000000000000001000000000000000000010000000000000000000200000000000000000003000000000000000000040000000000000000000500000000000000000006000000000000000000070000000000000000000800000000000000000009000000000000000000010000000000000000000100000000000000000002000000000000000000030000000000000000000400000000000000000005000000000000000000060000000000000000000700000000000000000008000000000000000000090000000000000000000100000000000000000001000000000000000000020000000000000000000300000000000000000004000000000000000000050000000000000000000600000000000000000007000000000000000000080000000000000000000900000000000000000001000000000000000000010000000000000000000200000000000000000003000000000000000000040000000000000000000500000000000000000006000000000000000000070000000000000000000800000000000000000009000000000000000000010000000000000000000a09c7b47112a3afb385c12924bf6280d273c106eea7caeaf5131d8776f61056c148876ae05d46b58d1fff866f864800a82c35094095e7baea6a6c7c4c2dfeb977efac326af552d8785012a05f200801ba01d2c92cfaeb04e53acdff2b5d42005ff6aacdb0105e64eb8c30c273f445d2782a01e7d50ffce57840360c57d94977b8cdebde614da23e8d1e77dc07928763cfe21c0", | ||
"invalidBlockRlp": "f90667f905fba0a8d5b7a4793baaede98b5236954f634a0051842df6a252f6a80492fd888678bda01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347948888f1f195afa192cfee860698584c030f4c9db1a0f93c8db1e931daa2e22e39b5d2da6fb4074e3d544094857608536155e3521bc1a0bb7495628f9160ddbcf6354380ee32c300d594e833caec3a428041a66e7bade1a0c7778a7376099ee2e5c455791c1885b5c361b95713fddcbe32d97fd01334d296b90100000000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000008000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000400000000000000000000000000000000000000000000000000000008302000001832fefd882560b84559c17b9b9040001020304050607080910151213141516171819202122232410000000000000000000200000000000000000003000000000000000000040000000000000000000500000000000000000006000000000000000000070000000000000000000800000000000000000009000000000000000000010000000000000000000100000000000000000002000000000000000000030000000000000000000400000000000000000005000000000000000000060000000000000000000700000000000000000008000000000000000000090000000000000000000100000000000000000001000000000000000000020000000000000000000300000000000000000004000000000000000000050000000000000000000600000000000000000007000000000000000000080000000000000000000900000000000000000001000000000000000000010000000000000000000200000000000000000003000000000000000000040000000000000000000500000000000000000006000000000000000000070000000000000000000800000000000000000009000000000000000000010000000000000000000100000000000000000002000000000000000000030000000000000000000400000000000000000005000000000000000000060000000000000000000700000000000000000008000000000000000000090000000000000000000100000000000000000001000000000000000000020000000000000000000300000000000000000004000000000000000000050000000000000000000600000000000000000007000000000000000000080000000000000000000900000000000000000001000000000000000000010000000000000000000200000000000000000003000000000000000000040000000000000000000500000000000000000006000000000000000000070000000000000000000800000000000000000009000000000000000000010000000000000000000100000000000000000002000000000000000000030000000000000000000400000000000000000005000000000000000000060000000000000000000700000000000000000008000000000000000000090000000000000000000100000000000000000001000000000000000000020000000000000000000300000000000000000004000000000000000000050000000000000000000600000000000000000007000000000000000000080000000000000000000900000000000000000001000000000000000000010000000000000000000200000000000000000003000000000000000000040000000000000000000500000000000000000006000000000000000000070000000000000000000800000000000000000009000000000000000000010000000000000000000a09c7b47112a3afb385c12924bf6280d273c106eea7caeaf5131d8776f61056c148876ae05d46b58d1fff866f864800a82c35094095e7baea6a6c7c4c2dfeb977efac326af552d8785012a05f200801ba01d2c92cfaeb04e53acdff2b5d42005ff6aacdb0105e64eb8c30c273f445d2782a01e7d50ffce57840360c57d94977b8cdebde614da23e8d1e77dc07928763cfe21c0" | ||
} |
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.
Good idea to extract this, nice.
} catch (error) { | ||
// remove invalid block | ||
await blockchain.delBlock(block.header.hash()) | ||
throw error |
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.
WHuuaaH! We will soon be in a state that normal people will be able to read our code.
I am so excited! 😄 😛 😄
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.
But honestly: this "before" -> "after" is really insane. Sitting here - sighting.
Hey @holgerd77, sounds like a good idea to pick this up in order to integrate this in the client. Is it an idea to create a npm release tagged with "dev" or something? (Otherwise how am I supposed to use the promisified version of blockchain/ethash? I can point directly in |
@jochem-brouwer can you open an issue on that so that we can discuss this separately? This would be a somewhat significant change in the release process and it is always better to have stuff like that documented, for future reference and the transparency of the process. You can also just create a local package with |
This PR promisifies the blockchain and ethash packages.
These are the last two packages with callbacks remaining in their API, so after this PR is merged the monorepo will be entirely migrated to promises. 🎉
This PR:
any
types in blockchain for better representationsgetDetails()
in blockchain (not used anywhere)validate
option (use:validateBlock
,validatePow
)Special thanks to @jochem-brouwer for beginning some of this work in #779.