Migrate Loop to typescript #505
Conversation
| @@ -522,11 +540,6 @@ export default class EEI { | |||
| // push the created address to the stack | |||
| return new BN(results.createdAddress) | |||
| } | |||
| } else { | |||
| // creation failed so don't increment the nonce | |||
| if (results.vm.createdAddress) { | |||
There was a problem hiding this comment.
results.vm.createdAddress is always undefined, as createdAddress is set on results itself. I fixed this, which led to some test failing. Had a look at geth and the contract creator's nonce is incremented, but not decremented on failure.
There was a problem hiding this comment.
Some questions for my own understanding, but no real blockers, looks good!
With the TypeScript benefits sounds really great, can't wait for a time when I'll have time again to dig deeper and get my hands more directly on the code again and experience this myself. 😄
| origin: Buffer | ||
| block: any | ||
| contract: Account | ||
| } |
There was a problem hiding this comment.
Phew, the transparency of this is mind blowing. Really neat!
| @@ -36,7 +54,7 @@ export default class EEI { | |||
| } | |||
|
|
|||
| refundGas (amount: BN): void { | |||
| this._result.gasRefund.iaddn(amount) | |||
| this._result.gasRefund.iadd(amount) | |||
| } | |||
There was a problem hiding this comment.
Uh, this didn't cause a bug before?
There was a problem hiding this comment.
It wasn't a bug because refundGas was called with a number always (see changes in opFns). I changed them to pass a BN instead of a normal number.
| @@ -554,7 +567,7 @@ export default class EEI { | |||
| } | |||
| } | |||
|
|
|||
| function trap (err: string) { | |||
| function trap (err: ERROR) { | |||
There was a problem hiding this comment.
Yeah, seems that these kind of typings are also very much a driver for consistency. Great to have this error cleanup.
| message: Message | ||
| txContext: TxContext | ||
| pc?: number | ||
| } |
There was a problem hiding this comment.
Program counter can be optimal sometimes?
There was a problem hiding this comment.
Ah, just for the options, self-answered. 😀
| stateManager: StateManager | ||
| storageReader: StorageReader | ||
| eei?: EEI | ||
| } |
There was a problem hiding this comment.
Again here: this is wonderful.
| runState: Object.assign({}, this._runState, this._result, this._runState.eei._env), | ||
| exception: err ? 0 : 1, | ||
| runState: Object.assign({}, this._runState, this._result, this._runState.eei!._env), | ||
| exception: err ? 0 as IsException : 1 as IsException, | ||
| exceptionError: err, |
There was a problem hiding this comment.
What's happening here with eei! ?
There was a problem hiding this comment.
Because eei is optional, typescript would give an error that it could be undefined, the ! tells the compiler I know it isn't undefined 😄 (because it's initialized in initRunState). Agree this is something that can be improved.
|
More strict options and results sound great, very much in favor. |
I already noticed the positive effects of typing everything, and the effects become more obvious the more everything is typed! It led to a few minor fixes (some just type or doc annotations, and one possible bug). I also removed
vmErrorin loop's result as it was redundant.Two points I hope we can improve in next PRs:
exceptionfrom the results, as an optionalexceptionErrorsuffices), and always return aVmError(not the error string directly).(To be merged into #479)