BN support in Common

[jochem-brouwer]

Lots of methods in Common have to do with numbers. We sometimes convert BNs to numbers (for instance block numbers) and feed those to Common. Common should support inputs where this input is of type BN and should also handle arithmetic as BN. We can keep supporting numbers by internally converting it to BN and then feeding it to the same method which handles the input as BN.

[jochem-brouwer]

Bumping this to important. We convert BNs to numbers at some places, where this is illegal if we use very high values (such as chain id, very big block numbers, etc.)

[holgerd77]

Also the question here if we want to use an (eventually expanded with BigInt) BNLike type for this.

[ryanio]

I have taken a look at all existing Common type signatures with number that we may want to upgrade:

input type containing a number

getHardforkByBlockNumber(blockNumber: number): string

setHardforkByBlockNumber(blockNumber: number): string

paramByBlock(topic: string, name: string, blockNumber: number): any

isNextHardforkBlock(blockNumber: number, hardfork?: string): boolean

activeOnBlock(blockNumber: number, opts?: hardforkOptions): boolean

activeHardforks(blockNumber?: number | null, opts?: hardforkOptions): Array<any>

activeHardfork(blockNumber?: number | null, opts?: hardforkOptions): string

isHardforkBlock(blockNumber: number, hardfork?: string): boolean

hardforkIsActiveOnBlock(hardfork: string | null, blockNumber: number, opts: hardforkOptions): boolean

output type containing a number

hardforkBlock(hardfork?: string): number

nextHardforkBlock(hardfork?: string): number | null

chainId(): number

networkId(): number

I'm not sure yet how we can return these as BNs without a breaking change, so please feel free to leave any ideas.

[holgerd77]

@ryanio thanks for bringing these together. 😄 Yes, I also had a short look couple of days ago. The only solution I can think of for these return-number messages is doing temporary e.g. nextHardforkBlockBN() methods and then (important!) add to the v6 breaking notes that we will want to replace/unify these methods again.

We might also want to give a short note on the code docs of the methods themselves.

[ryanio]

@holgerd77 no problem. another idea I had today is what if we added an extra optional param returnAsBN?

e.g.

chainId(returnAsBN?: true): BN
chainId(returnAsBN? = false): number

so current usage would still work as expected without any breaking change.

I think I prefer just adding a new method chainIdBN though, otherwise the params might get confusing in the future if we add other options and have to juggle the returnAsBN, or if we remove it and then people are left with chainId(false) and chainId(true) with no clear reasoning why.

On another note, can we do a small release on ethereumjs-util so I can get access to the new toType method? Then I can start on this PR. I could do without, but I like the Number.isSafeInteger checks already included on the input and output.

[holgerd77]

Yeah, if it wouldn't be for your explanation I guess I would have also agreed to the parameter version, but these extra methods seems to be the less invasive variant.

Some other aspects: I was thinking a bit about a (not so far in the) future BigInt transition. My current idea/suggestion on how to do this (at least regarding the methods we are touching here and with the other BNLike updates):

1. At some point we add BigInt as a possible input type for BNLike (I guess we can do this already? We should nevertheless open a dedicated issue and discuss on potential side effects - eventually just being on the "social" side (encouraging people to unreflectedly use BigInt and therefore break their libraries unintentionally for a still somewhat large set of users)
2. At some later point we remove BN.js from there
3. For people who don't want to worry about this at all we recommend to just use Buffer directly (and we might want to do ourselves, not sure, maybe also depends on the context)

With this in consideration I guess it does make sense here (for the input parameters) to rather broaden and go for BNLike. Then these methods are already prepared for this somewhat smoother transition process.

For the output methods it might rather make more sense to go for Buffer instead of BN?