Common: Rename/delete ambiguous active* methods

[holgerd77]

There currently is some ambiguity around methods named with "active*" in the Common library, see index.ts implementation (best take the version from the develop branch, since there has already been some additional bloat along "active HFs" being removed by #1649 so this is easier to digest).

So there is basically the meaning of "active" in meaning that a HF respectively its rules are activated (e.g. on a certain block number). I guess this is the intuitive and expected meaning respectively semantics. So the associated method names should be kept:

• hardforkIsActiveOnBlock()
• activeOnBlock()

The second used version for "active" in method names is rather meaning: "is generally included (but not necessarily: active 😋) on the chain.

These should be renamed for clarity, this ambiguity has already let to wrong usage patterns in the past.

Following suggestings:

• hardforkIsActiveOnChain -> isIncludedHardfork
• activeHardforks -> includedHardforks
• activeHardfork -> lastestIncludedHardfork

Another thing I noticed together with an alternative solution:

All these three methods are extremely little used. For our monorepo:

1. hardforkIsActiveOnChain -> One usage in block header
2. activeHardforks -> No external usage, several Common internal usages
3. activeHardfork -> One usage in block header

Both usages are actually even suboptimal usages of the Common API and can be replaced by other Common functions. For 1. all the four lines or so of various checks can be replaced with a single hardforkIsActiveOnBlock() call if I am not mistaken. For 3. a getHardforkByBlockNumber() would be the replacement and - furthermore - this whole _getHardfork() function in block is generally misplaced and should be removed entirely (not sure if this would lead to some necessary tweaks), since normally the hardfork should directly be retrieved by Common, this is some sort of strange functionality "doubling".

So I wonder if we should rather - at least for 1. and 3. - remove these functions at all. 🤔

I think this whole semantics is somewhat confusing, regardless of the name.

For 2. we might consider to rather make an internal private function (since this is a helpful utility function inside of Common) or maybe we keep this single one (but then also do the renaming).

Thoughts?

[holgerd77]

Short note: have done several post corrections, please read on site! 🙂

[holgerd77]

Ok, had another deeper look here: even the activeHardforks() function is just used in one additional place in Common, in paramByBlock(), and there it can actually be replaced by a getHardforkByBlockNumber() call, which would actually be even better, because then the paramByBlock() method can also be adopted to take an optional td paramter and then forward this to getHardforkByBlockNumber(), then the method will also work in a merge-context (eventually then also update the upstream usages as well).

With this I would really pledge to delete all three messages. Actually I guess it is safe to execute on this if there are 24 hours or so more with no objections raised here.

So - as a summary - "action items" on this would be:

1. Remove hardforkIsActiveOnChain and adopt the one upstream usage in Block (see above)
2. Remove activeHardforks and adopt usage in Common paramByBlock()
3. Adopt paramByBlock() to take optional td, update upstream usages (if possible)
4. Remove activeHardfork and adopt the upstream usage in Block (see above)
5. Eventually combined with 4. or separate: remove _getHardfork() usage in Block

[emersonmacro]

@holgerd77 I was able to adapt paramByBlock to take an optional td value and then remove activeHardfork and activeHardforks successfully.

I ran into an issue in your suggestion for removing hardforkIsActiveOnChain, in that hardforkIsActiveOnBlock throws an error if the hardfork is not defined on the chain (whereas hardforkIsActiveOnChain would just return false in that case), so tests were failing where the Dao hardfork isn't defined for the chain (e.g. Rinkeby, Ropsten). So I went with renaming hardforkIsActiveOnChain to isIncludedHardfork, but wondering if you have any thoughts or suggestions.

[holgerd77]

@emersonmacro Ok, I would cautiously say here (if someone wants to correct feel free, otherwise you can execute I guess): this internal _getHardfork() function shouldn't throw in the first place but we should change this to return undefined instead. This semantical differentiation between "hardfork which lays somewhat in the future" and "hardfork which is just not there" doesn't make any sense.

I have looked at the 3-4 usages of this function, all should remain safe to execute. You need to adopt the following boolean checks though a bit and first check for the respective _getHardfork() result (when called from another Common function) and only then check for (access) the properties.

Just thinking a bit, maybe you want to add one respective additional test (if not already existing) per _getHardfork()-using function - in particular for hardforkBlock() - testing with a I guess best random HF string "thisHardforkDoesNotExist" or so - and check that the function nevertheless works (return null).

And, then - to make things complete - hardforkIsActiveOnBlock should also work as expected and not throw any more and it should be possible to use as a replacement function for block header and remove the hardforkIsActiveOnChain() function.

Would also be nice if you collect the change semantics for functions a bit together we have made through all of this and post as a list or so in your initial PR comment, so that we have this a bit together for the release notes.

Thanks a lot! 😄

[holgerd77]

Closed by #1753