Replace Object.assign calls and fix type errors

[alcuadrado]

While reviewing the codebase some calls to Object.assign caught my attention, as I suspected that they may be hiding some type errors.

I replaced them with object literals using the spread operator (i.e. { ...obj }) and normal field assignments to check if that was the case.

My suspicion turned out to be right, and I found two type errors in Interpreter#runLoop:

1. If Loop#run returned an exception, null was set as the result's gasRefund and selfdestruct fields.
2. The result's runState didn't type.

I tried to fix them, but I'm not familiar enough with the codebase as to be sure that my changes are correct.

For 1, I replaced the nulls with the default values used to initialize those fields.

For 2, I made LoopResult#runState non-optional, as I didn't find any instance of it being undefined.

[s1na]

Does the spread operator help the compiler detect errors compared to Object.assign?

Re 1.: Here are where those values are consumed:

https://github.com/ethereumjs/ethereumjs-vm/blob/3e9a91ffce924bd79ad54e09904fc30df9645ccc/lib/runTx.ts#L172-L174

https://github.com/ethereumjs/ethereumjs-vm/blob/3e9a91ffce924bd79ad54e09904fc30df9645ccc/lib/runTx.ts#L202-L204

So the null values were meant to signal that there's no need to do those computations. The default empty values would work just as fine however if that makes the code more clear.

Re 2.: runState is not available for returning e.g. when Loop returns early (when it's only a transfer, and no code to run). I'd also be more in favor of not returning it at all. We're not using it further in our code, and it's being emitted as an event, I don't see why we should return such a huge object every time.

[alcuadrado]

Does the spread operator help the compiler detect errors compared to Object.assign?

I think so. Object.assign returns the intersection (i.e. &) of their params' types. If that intersection is empty because of incompatible params, it fails silently. Here's an example:

interface A {
    n: number
    s:string
}

const a: A = Object.assign({n:123}, {s:"a"}, {s: 123})
// a.s === 213

The literal with spread operators keeps the last type for each field. This is how it behaves at runtime too, so it makes more sense and helps catch errors.

This version

const a: A = {
    ...{n:123}, 
    ...{s:"a"}, 
    ...{s: 123}
}

results in this type error:

Type '{ s: number; n: number; }' is not assignable to type 'A'.
  Types of property 's' are incompatible.
    Type 'number' is not assignable to type 'string'. ts(2322)

The default empty values would work just as fine however if that makes the code more clear.

I tried adding | null to their types, and the code got much more verbose whenever those were used.

I'd also be more in favor of not returning it at all. We're not using it further in our code, and it's being emitted as an event, I don't see why we should return such a huge object every time.

That return value is probably the most complex part of this codebase. It's pretty scary. I'm in favor of simplifying it, but I have no idea what that implies.

[s1na]

The literal with spread operators keeps the last type for each field. This is how it behaves at runtime too, so it makes more sense and helps catch errors.

That's a very good point, thanks for the explanation. I'll try to use the spread operator from now on.

That return value is probably the most complex part of this codebase. It's pretty scary. I'm in favor of simplifying it, but I have no idea what that implies.

Let's do the simplification in a future PR. For now can you please make runState optional again, and rebase so we can merge.

[alcuadrado]

Let's do the simplification in a future PR. For now can you please make runState optional again, and rebase so we can merge.

I did that and got a type error again. I think I was confused about it, and that now I fixed it correctly. Please take a look at the last commit.

[coveralls]

Coverage increased (+0.006%) to 94.997% when pulling 17c17a0 on object-assign-type-errors into c2a5cec on master.

[s1na]

Thanks for the fix, looks good to me. We should really simplify the return values though :)

[holgerd77]

Everyone is getting so creative on improvements 😄, I think we really should give it yet another 1-2 weeks until the beta release to not interrupt this flow. Doesn't make sense even to drop a beta release out if there is still so much active work ongoing on the base layer, also have some things in mind for integration.