Chain & HF enum usage

[emersonmacro]

Second stage of work for #702. Updates usage of common package throughout monorepo to use new Chain and Hardfork enums.

My methodology was to use the global search term new Common and go package by package, so it's possible I may have missed some spots, but I tried to be as comprehensive as possible.

[codecov]

Codecov Report

Merging #1363 (c39c82a) into master (7a5af59) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	85.90% <100.00%> (-0.16%)	⬇️
blockchain	83.43% <100.00%> (ø)
client	83.97% <100.00%> (-0.04%)	⬇️
common	93.02% <ø> (+0.31%)	⬆️
devp2p	83.32% <ø> (-0.25%)	⬇️
ethash	82.83% <ø> (ø)
tx	88.36% <100.00%> (+0.11%)	⬆️
vm	79.23% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[acolytec3]

This looks good. I spotted 3 places where the chain: 'chainName' pattern is used in the tests and updated the obvious ones. A question though:
If the chain is something other than a supported network as here or here, are we just continuing to support string as an acceptable type for the chain and hardfork parameters?

[acolytec3]

This looks great though I'm a little concerned about the namespace collision between the new Chain enum in @ethereumjs/common and then the Chain class exported by @ethereumjs/client. It's easy enough to hack around by just aliasing one of them as here but I wonder if it wouldn't be better to rename the enum to something like ChainName or something? I'm not how often sure users would be instantiating a new Common instance with the client since we don't have a client package actually live on NPM yet or if client even exports chain but I could see this tripping people up down the line. Thoughts?

[ryanio]

This looks great though I'm a little concerned about the namespace collision between the new Chain enum in @ethereumjs/common and then the Chain class exported by @ethereumjs/client. It's easy enough to hack around by just aliasing one of them as here but I wonder if it wouldn't be better to rename the enum to something like ChainName or something? I'm not how often sure users would be instantiating a new Common instance with the client since we don't have a client package actually live on NPM yet or if client even exports chain but I could see this tripping people up down the line. Thoughts?

thanks for contributing to the PR!

good point about the namespace collision, but you also make a good point that not many people will likely be developing in the client and be instantiating commons so I don't think it requires renaming the whole enum. I think having the simpler enum name of Chain is more useful to have. If importing both, one could also alias the client's Chain as ClientChain or something. Thinking more about it, I think the client's Chain class itself could maybe have a better name, since it almost seems like a nickname and doesn't help to clarify what it exactly is (my understanding is it's the client's blockchain helper class that interfaces with the underlying @ethereumjs/blockchain).

[ryanio]

thanks @emersonmacro, this looks great! 🎉

[emersonmacro]

@ryanio thanks for updating and merging 🎉

@acolytec3 thanks for taking a look as well, and definitely valid points. i kept string as an acceptable type based on the discussion here #702 (comment), and i think it provides some nice flexibility. the namespace collision is unfortunate, but the more time i spent looking at usage in the codebase, the more i started leaning toward what Ryan said above. the Chain enum in common works pretty well, and i do think the Chain class in client could be a candidate for a better name (just not sure what yet).