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

Promisify blockchain and ethash packages #833

Merged
merged 6 commits into from
Aug 18, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
466 changes: 109 additions & 357 deletions package-lock.json

Large diffs are not rendered by default.

15 changes: 5 additions & 10 deletions packages/block/src/header.ts
Original file line number Diff line number Diff line change
Expand Up @@ -351,15 +351,10 @@ export class BlockHeader {
}

private async _getBlockByHash(blockchain: Blockchain, hash: Buffer): Promise<Block | undefined> {
return new Promise((resolve, reject) => {
blockchain.getBlock(hash, (err, block) => {
if (err) {
reject(err)
return
}

resolve(block)
})
})
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

try {
return blockchain.getBlock(hash)
} catch (e) {
return undefined
}
}
}
2 changes: 1 addition & 1 deletion packages/block/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,5 @@ export interface BlockData {
}

export interface Blockchain {
getBlock(hash: Buffer, callback: (err: Error | null, block?: Block) => void): void
getBlock(hash: Buffer): Promise<Block>
}
8 changes: 2 additions & 6 deletions packages/blockchain/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,11 @@
"@ethereumjs/block": "^3.0.0",
"@ethereumjs/common": "^1.5.1",
"@ethereumjs/ethash": "^1.0.0",
"async": "^2.6.1",
"ethereumjs-util": "^7.0.4",
"flow-stoplight": "^1.0.0",
"level-mem": "^3.0.1",
"level-mem": "^5.0.1",
"lru-cache": "^5.1.1",
"rlp": "^2.2.3",
"semaphore": "^1.1.0",
"util.promisify": "^1.0.1"
"semaphore-async-await": "^1.5.1"
},
"devDependencies": {
"@ethereumjs/config-nyc": "^1.1.1",
Expand All @@ -57,7 +54,6 @@
"@types/bn.js": "^4.11.6",
"@types/lru-cache": "^5.1.0",
"@types/node": "^11.13.4",
"@types/semaphore": "^1.1.0",
"@types/tape": "^4.13.0",
"nyc": "^14.0.0",
"prettier": "^2.0.5",
Expand Down
77 changes: 0 additions & 77 deletions packages/blockchain/src/callbackify.ts

This file was deleted.

63 changes: 45 additions & 18 deletions packages/blockchain/src/dbManager.ts
Original file line number Diff line number Diff line change
@@ -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,
Copy link
Member

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

Copy link
Contributor Author

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

Expand All @@ -12,23 +14,41 @@ import {
} from './util'

import BN = require('bn.js')
import type { LevelUp } from 'levelup'
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved

const level = require('level-mem')
import { Block, BlockHeader } from '@ethereumjs/block'

/**
* @hidden
*/
export interface DBOp {
type: String
key: Buffer | String
keyEncoding: String
valueEncoding?: String
value?: Buffer | object
}

/**
* @hidden
*/
export interface GetOpts {
keyEncoding?: string
valueEncoding?: string
cache?: string
}
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Abstraction over a DB to facilitate storing/fetching blockchain-related
* data, such as blocks and headers, indices, and the head block.
* @hidden
*/
export default class DBManager {
export class DBManager {
_cache: { [k: string]: Cache<Buffer> }
_common: Common
_db: LevelUp

_common: any

_db: any

constructor(db: any, common: any) {
constructor(db: LevelUp, common: Common) {
this._db = db
this._common = common
this._cache = {
Expand All @@ -43,29 +63,33 @@ export default class DBManager {
/**
* Fetches iterator heads from the db.
*/
getHeads(): Promise<any> {
return this.get(headsKey, { valueEncoding: 'json' })
async getHeads(): Promise<{ [key: string]: Buffer }> {
const heads = await this.get(headsKey, { valueEncoding: 'json' })
Object.keys(heads).forEach((key) => {
heads[key] = Buffer.from(heads[key])
})
return heads
}

/**
* Fetches header of the head block.
*/
getHeadHeader(): Promise<any> {
async getHeadHeader(): Promise<Buffer> {
return this.get(headHeaderKey)
}

/**
* Fetches head block.
*/
getHeadBlock(): Promise<any> {
async getHeadBlock(): Promise<Buffer> {
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved
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> {
Copy link
Member

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.

Copy link
Contributor Author

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

// determine BlockTag type
if (typeof blockTag === 'number' && Number.isInteger(blockTag)) {
blockTag = new BN(blockTag)
Expand All @@ -83,11 +107,14 @@ export default class DBManager {
throw new Error('Unknown blockTag type')
}

const header: any = (await this.getHeader(hash, number)).raw
let body
const header = (await this.getHeader(hash, number)).raw
let body: any
try {
body = await this.getBody(hash, number)
} catch (e) {
} catch (error) {
if (error.type !== 'NotFoundError') {
throw error
}
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved
body = [[], []]
}

Expand Down Expand Up @@ -149,7 +176,7 @@ export default class DBManager {
* it first tries to load from cache, and on cache miss will
* try to put the fetched item on cache afterwards.
*/
async get(key: string | Buffer, opts: any = {}): Promise<any> {
async get(key: string | Buffer, opts: GetOpts = {}): Promise<any> {
const dbOpts = {
keyEncoding: opts.keyEncoding || 'binary',
valueEncoding: opts.valueEncoding || 'binary',
Expand All @@ -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[]) {
Copy link
Member

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? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh it is async, i forgot to add the keyword, i just removed the return type because it can automatically interpret it:

Screen Shot 2020-08-17 at 11 39 05 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in e6e6487

return this._db.batch(ops as any)
}
}
Loading