Common File Layout Alignment #1915
Conversation
acolytec3
force-pushed
the
common-refactor
branch
from
e5d7b8e to
9e4d077
Compare
|
Barring something I missed somewhere in an obscure test, this should be ready for a first round of review. The important bits are the |
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Okay, now it's really ready for review. |
packages/block/test/block.spec.ts
Outdated
| @@ -20,7 +20,7 @@ import util from 'util' | |||
|
|
|||
| tape('[Block]: block functions', function (t) { | |||
| t.test('should test block initialization', function (st) { | |||
| const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.Chainstart }) | |||
| const common = new Common({ chain: ChainId.Mainnet, hardfork: HardforkName.Chainstart }) | |||
There was a problem hiding this comment.
Ah, sorry, but I find both these renaming very unfortunate, I didn't think that you would change these actually.
These are all over the place, we settled on the short names a long time ago (and I honestly also like these a lot) and this is also used by the Community on all their instantiations, so this would cause a lot of unnecessary hazzle.
Can you please name back? If this conflicts with something else, then please rename the "else" part, but this should really stay stable and untouched.
There was a problem hiding this comment.
Sure, though alternatively can we rename the corresponding Hardfork and Chain interfaces? It makes no sense to me to have both an enum and an interface with the same name and produces all sorts of naming collisions.
There was a problem hiding this comment.
Yes, these are a lot more uncritical, since close to no one will use these directly. So maybe we can do HardforkConfig and ChainConfig for these?
There was a problem hiding this comment.
Cool, I'd already started down this path so will revert all the other commits.
There was a problem hiding this comment.
(not sure if we should then rename to GenesisBlockConfig and BootstrapNodeConfig as well for consistency? A bit undecided)
There was a problem hiding this comment.
Can you also do:
- Remove this
eipsTypecompletely and type directly at the one application point with[key: number]: any. This is just polluting thetypesfile and it should be avoided that these kind of super-trivial non-relevant types are exported and eventually used directly by users. - Make sure all types/interfaces/... start with capital letters
genesisStatesTypeseems not be used at all anywhere, delete?- Rename
ChainsTypeto something more consistent (we never haveTypein the name somewhere else, at least if it is meant in the sense of "TypeScript Type" (so we do haveConsensusType, but that has another semantics)), I would suggestChainsConfig
There was a problem hiding this comment.
I believe I've addressed all the feedback here. I know @ryanio mentioned combining the types/enums into one file but I kind of like them separated. Just makes things more readable to my way of thinking but could be persuaded otherwise.
There was a problem hiding this comment.
nice thanks, just took a look, yeah i think it is kind of nice to have them separated actually :)
acolytec3
force-pushed
the
common-refactor
branch
from
21d8ec0 to
28b8b5c
Compare
* common: create new common.ts * common: finish reorganize/rename in common * Address comments
* common: create new common.ts * common: finish reorganize/rename in common * Address comments
* common: create new common.ts * common: finish reorganize/rename in common * Address comments
* common: create new common.ts * common: finish reorganize/rename in common * Address comments
* common: create new common.ts * common: finish reorganize/rename in common * Address comments
Fixes #1913