Block: replace static constructors #3489
Conversation
|
Tests failing with this message: update fixed with commit |
ScottyPoi
added
PR state: needs review
type: refactor
package: block
breaking
target: master
There was a problem hiding this comment.
Ok, had a look, great, some things around general conventions for this kind of PRs, otherwise I guess we can also already take this in, had some closer look at the EVM changes, these are actually not that vast so I guess merging won't be as much as a problem as I anticipated.
There are 3 merge conflicts in this PR to fix though first.
| @@ -0,0 +1,442 @@ | |||
| import { RLP } from '@ethereumjs/rlp' | |||
There was a problem hiding this comment.
I would like to establish a general naming convention for this file name that we re-use throughout all libraries, so that it's just clear where to look for, same a bit as our types.ts files.
Can we take constructors.ts here in this case? That would be my pledge! 🙂
So without block (redundant, I would also pledge for just always having one file for this per library), and also using plural (constructors), since several constructors (let's always use this name, and not fall back to singular if for the moment there is only one constructor present for a library).
There was a problem hiding this comment.
(so these comments go more or less 1:1 for the other PR for blockchain)
packages/block/src/block.ts
Outdated
| const executionPayload = executionPayloadFromBeaconPayload(payload) | ||
| return Block.fromExecutionPayload(executionPayload, opts) | ||
| } | ||
|
|
||
| /** | ||
| * This constructor takes the values, validates them, assigns them and freezes the object. | ||
| * Use the static factory methods to assist in creating a Block object from varying data types and options. |
There was a problem hiding this comment.
One thing which has gotten a bit worse with this extraction is discoverability, so it has gotten harder to find/auto-complete what constructors are available.
Can we make it a habit to therefore to always provide a list with all the (linked, so clickable) static constructor methods, either here or along the class (Block) code docs (can't fully decide, where does one stumble more upon this? If one imports a Block and then hovers over the class import or if one types in new Block() and then sees the code docs for the constructor? Guess maybe the class docs?).
So that would be great and pretty helpful!
Also: can you add a @deprecated note here for this constructor as well, like we have for other constructors (tx? evm?) already?
There was a problem hiding this comment.
I agree. It is a nicer experience to type in Block. and have the constructors pop up.
There was a problem hiding this comment.
Ok, but really only in the code docs, not that we talk past each other! 🙂
So like:
/**
/* ...
/* The following constructors are available:
/* - createBlockFromBlockData() // <- This should then be clickable on mouseover in the IDE (VS Code)
/* - ...
**/
|
Ah, and one other thing: I would also find it great if we make it a habit to always post a comparable pre- and post-PR bundle size respectively the diff, so that we get a feeling about the changes and have this "measured". Won't be dramatically big here, but nevertheless. Can you maybe do this along the instructions from my presentation (slide 5 😋), and so maybe do a super-small script creating a block using one of the constructors (best the simplest to use one) with the old Guess this will generally be pretty motivating! 🏆 😄 |
ScottyPoi
force-pushed
the
block-static-methods
branch
from
0320141 to
e77d773
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
Works on #3487
Replaces all static Block methods with independently defined functions
Block constructor methods:
moved to
packages/block/src/blockConstructor.tsand renamed:Trie Root methods:
moved to
packages/block/src/helpers.tsand renamed: