Common: Chain Refactor #3545
Common: Chain Refactor #3545
Conversation
…onality, adjust/simplify createCustomCommon(), remove setChain(), fix tests
holgerd77
force-pushed
the
common-chain-refactor
branch
from
bc9ee48 to
c5e696f
Compare
| @@ -27,7 +27,7 @@ type ConsensusConfig = { | |||
|
|
|||
| export interface ChainConfig { | |||
| name: string | |||
| chainId: number | bigint | |||
| chainId: number | string | |||
There was a problem hiding this comment.
This is also a change for serialization and is not yet reflected in the code base.
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. |
There was a problem hiding this comment.
@acolytec3 @jochem-brouwer thanks a lot for working through this 🙏, some additional post-merge comments, but nothing grave!
One-step-back question: are you guys generally satisfied/happy with the direction of the changes within this PR, now that you have worked your way through this?
| @@ -50,10 +50,10 @@ const genesisState: GenesisState = { | |||
| [contractAddress]: accountState, | |||
| } | |||
|
|
|||
| const common = new Common({ | |||
| // @ts-ignore PrefixedHesString type is too strict | |||
There was a problem hiding this comment.
Was this PrefixedHexString comment something one could not "fix on the fly"? 🤔
If something is "structurally" too strict, we should maybe un-stricten it?
There was a problem hiding this comment.
When we take JSON and pass it directly to the various constructors that have the PrefixedHexString type, Typescript now complains because we're passing in string s. We either need to loosen the type on the opts that can be passed to the various *Data constructors or else type check all these strings ahead of time to confirm they're prefixed correctly.
| const genesis = createBlockFromRLPSerializedBlock(genesisRlp, { common }) | ||
|
|
||
| const blockRlp = hexToBytes(testData.blocks[0].rlp as PrefixedHexString) | ||
| const blockRlp = hexToBytes(testData.default.blocks[0].rlp as PrefixedHexString) |
There was a problem hiding this comment.
Are all these now commonly applied default accesses future proof? (so: not only work in our specific current tooling setup)
There was a problem hiding this comment.
@acolytec3 maybe a final comment on this one?
There was a problem hiding this comment.
I think it's fine, though honestly not 100% sure. I think it was a knock-on effect of switching our dev setup to module in our package.json across the monorepo.
There was a problem hiding this comment.
Ok, thanks, then let's keep/take it, though I find all these kind of things honestly a bit spooky
| @@ -60,7 +60,7 @@ export class Common { | |||
| constructor(opts: CommonOpts) { | |||
| this.events = new EventEmitter() | |||
|
|
|||
| this._chainParams = JSON.parse(JSON.stringify(opts.chain)) | |||
| this._chainParams = opts.chain | |||
There was a problem hiding this comment.
This JSON.parse(... stuff was to do a deep copy of the original structure, I otherwise had the cases where the original data got modified when being modified in the Common object.
So, this e.g. concretely played out somewhere in the EVM/VM tests where a test broke when simply Mainnet or so was used as in input two times in a row to a Common object and in the first round the associated _chainParams got modified.
(I am doing a commit-by-commit review, bear with me if some of the stuff is getting fixed on later commits)
There was a problem hiding this comment.
( @jochem-brouwer even mentioned that we should do this in yet another place, can't ad hoc remember where (maybe along the copy() method))
There was a problem hiding this comment.
Btw, I learned just this week there is a better way to deep copy an object in js called structuredClone https://developer.mozilla.org/en-US/docs/Web/API/structuredClone
There was a problem hiding this comment.
That'll be a nice thing to switch to if/when NodeJS brings it out from behind the experimental flag. Looks like it's in Node 22 but not fully supported yet. https://nodejs.org/api/globals.html#structuredclonevalue-options
| .filter((hf) => hf.timestamp !== undefined) | ||
| .map((hf) => { | ||
| hf.timestamp = undefined | ||
| }) |
There was a problem hiding this comment.
I am not fully seeing through this change, but do we really need this hacky stuff here? 🤔
Or is otherwise something wrong with our typing/data structure/whatever?
|
Also, to complete here: This DOES have its desired effect on the bundle size, so Before (on 
After: 
Or simply by "looking at the box sizes" if the above is too noisy: Before: 
After: 
Slowly getting there. |
|
Very nice! |
Ok, this is a first PoC on the Common simplification with directly passing in (and only allowing) a
ChainConfigobject (so, simply:Mainnete.g.) to thechain, as discussed with @acolytec3 some time ago!This is for tree shaking, but also leads to a dramatic simplification of code! Conversion helpers, all gone, lots of typing simplifications, also a strongly streamlined
creatCustomCommon.I also killed
setChain()along the way, since we never had any use case for runtime-chain switching, as some side note.This is absurdly intrusive though regarding the code changes, hope I get this under control! 😝 Not so intrusive luckily regarding the final outer API changes.
Uff. 🤯
Ah, and the
Commontests are already passing, that's something. None of the upstream tests though yet.-- Also added
getPresetChainConfigas a reasonable replacement forgetInitializedChainssince we no longer allow multiple chain configurations in a single instance ofcommon. (note from @acolytec3)