Common: Return BigInt for param() functionality

[holgerd77]

There are four param-retrieving functions in Common:

• param()
• paramByEIP()
• paramByHardfork()
• paramByBlock()

These functions currently return a number if available, otherwise null (needs confirmation). When doing a search on usages of the functions it turns out that on 80%+ of the occurrences, the value is used in a BigInt context and therefore transformed to BigInt with a BigInt() constructor. In many of the remaining 20% cases there is an indirect need of a BigInt result (e.g. at the end of some calculation).

It would therefore make a lot of sense to directly return a BigInt on these function calls.

In addition all these functions have an any return type. This is highly suboptimal and the functions should be properly typed along with this transition work.

[jochem-brouwer]

I vaguely remember that we have some values which are of type string? (Not entirely sure though)

[holgerd77]

Ah, you are right, just found minerReward in byzantium, being "3000000000000000000".

So it seems this is just for big numbers, so this would indeed even some additional argument to go BigInt here. 🙂

[acolytec3]

I've started work on this and have a question. I don't think returning null if a param isn't found is a useful return value since Javascript implicitly coerces null to 0 when you apply mathematical operations to it. So in all the places where we might return null, that return value is mostly just used in some arithmetic operation so implicitly is a 0. When we switch the return value to bigint this implicit coercion no longer works and we get all these errors about a return value possibly being 0. To my way of thinking, we should just return either the parameter value or 0 since that's what's implicitly being used across our codebase already. Any thoughts?

[ryanio]

another option would be to throw if it isn't found. it might be safer, and encourage ensuring only valid parameters are accessed. i imagine it would be more difficult to debug certain situations if a param was being returned as zero rather than alerting that it wasn't found.

[acolytec3]

Possibly and I'll try it out locally and see what side affects it has. It seems like it could cause a much bigger set of needed rewrites to implement try/catch logic across the monorepo since this code is used in lots of places on the assumption it's returning either null or a number already. Will do some more looking.

[ryanio]

yeah, well if null is handled like that then maybe that achieves the desired affect, but i see your point about null getting coerced to zero. i think handling null is easier than handling a thrown error though. i would think the code could be guarded (e.g. a common if eip enabled check) before accessing params that are only available after an upgrade.

[acolytec3]

Here's a good example of what I'm talking about. The test expects the data fee returned to be zero but two layers down in the call stack, you have this call that expects a return value or 0 (not an error if the specific parameter doesn't exist). It's easy enough to fix this particular instance but I just don't have a sense for if throwing an error really the right solution here. If this example and however many others like it were already working with null being coerced to zero, should we intentionally change the behavior of something to require a try/catch in all use cases. I'll push my commits so far to a PR so we can look at the diff and think through the right approach here.

[holgerd77]

Yeah, I would agree to think that throwing here is overblown. Can't we just return undefined (as it is) instead of null? This generally seems cleaner to me. This might need some code adoptions too, but these then actually should be in place since this is just the reality: a value can be present or not.

Returning 0 seems unclean and even dangerous to me since 0 also is an explicit value statement. In arithmetics this might have no side effects (but there it can very well have, e.g. for division cases), it others it might (will) have side effects in unlucky situations I would assume (even if it didn't show side effects in our existing code base, seems rather pure luck to me than "by-clean-structural-setup").

[holgerd77]

Update: just some additional thought: I guess 0 values are sometimes (often?) just the - somewhat implicit - default values before something is set to a value greater than 0.

It might also help on these situations to add an explicit value 0 to chainstart.

Just as some thought, this might also help to get to an overall consistent solution.

[holgerd77]

Closed by #1854