Promisify public API #546
Promisify public API #546
Conversation
|
There is a WASM file in here. ? 😋 |
Regenerate docs Rm mistakenly pushed wasm file
s1na
force-pushed
the
promisify-runBlockchain
branch
from
9658fa3
to
4060c43
Compare
|
Oops 😄 Removed and rebased |
| runBlockchain(blockchain: any, cb: any): void { | ||
| runBlockchain.bind(this)(blockchain, cb) | ||
| runBlockchain(blockchain: any): Promise<void> { | ||
| return promisify(runBlockchain.bind(this))(blockchain) | ||
| } |
There was a problem hiding this comment.
This seems to be so explicitly done to use promisify here and not make runBlockchain itself async that I'm sure you have a reason, so I won't make this a review blocker.
Can you nevertheless give a short (eventually) post-merge explanation on this please?
There was a problem hiding this comment.
Update: explanation here, no need to give an answer.
There was a problem hiding this comment.
explanation here, no need to give an answer.
Yeah that was the reason, but on the other hand what I could have done is to return a promise in runBlockchain without using promises within it internally. I did this now and pushed, so we won't need to promisify in the VM.
| @@ -219,7 +200,7 @@ async function applyTransactions(this: VM, block: any) { | |||
| } | |||
|
|
|||
| // Run the tx through the VM | |||
| const txRes = await promisify(this.runTx).bind(this)({ | |||
| const txRes = await this.runTx({ | |||
| tx: tx, | |||
| block: block, | |||
| }) | |||
There was a problem hiding this comment.
Looks good, also had a look at the whole file to get some broader picture.
| blockchain.delBlock(block.header.hash(), function() { | ||
| cb(err) | ||
| }) | ||
| }) | ||
| } | ||
| } | ||
| } |
| } catch (e) { | ||
| await state.revert() | ||
| throw e | ||
| } |
| } catch (e) { | ||
| await state.revert() | ||
| throw e | ||
| } | ||
| } | ||
|
|
||
| async function _runTx(this: VM, opts: RunTxOpts): Promise<RunTxResult> { |
There was a problem hiding this comment.
Isn't it possible here to eliminate this inner _runTx method similar like in runBlock? Won't make this a blocker either, just please give a short explanation (or update if just forgotten).
There was a problem hiding this comment.
I thought it's a clean separation that the outer function performs checkpointing, commit on success and revert on any error. And the internal method modifies the state without being aware of this.
I can change this however if it doesn't make sense
There was a problem hiding this comment.
Ah, no, that's a good reasoning, thanks.
| interpreter | ||
| .executeMessage(message) | ||
| .then(results => cb(null, results)) | ||
| .catch(err => cb(err, null)) |
There was a problem hiding this comment.
Ok, the result from executeMessage is now returned directly.
| .runLoop(opts.message, { pc: opts.pc }) | ||
| .then(res => cb(null, res)) | ||
| .catch(err => cb(err, null)) | ||
| return interpreter.runLoop(opts.message, { pc: opts.pc }) | ||
| } |
There was a problem hiding this comment.
Ok, similar here, runLoop result is directly returned.
| } | ||
| t.comment(JSON.stringify(stateRoot)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
This is some cleaning up of conditional expression boxing if I get this correctly.
| } else { | ||
| done() | ||
| } | ||
| }) |
There was a problem hiding this comment.
Ok, different result handling due to promisified API.
| st.end() | ||
| }) | ||
| st.assert(!err) | ||
| st.end() |
There was a problem hiding this comment.
Post-review comment: this is also a nice simplification.
I'd appreciate having a broader look here to see I haven't missed anything.
As per discussion in #455 and #544