-
Notifications
You must be signed in to change notification settings - Fork 751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Client: Refactor Engine API #3291
Conversation
Codecov ReportAttention:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -0,0 +1,159 @@ | |||
import { Block } from '@ethereumjs/block' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this file to something like blockchainHelpers
or something? generic
seems to communicate to me that this is something to with type generics (aka something like function<T>(arg1, arg2)
)
@@ -0,0 +1,14 @@ | |||
import { bytesToHex } from '@ethereumjs/util' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any objection to including under getPayload.ts
since it and the getPayload
methods are logically connected and this doesn't have many lines of code to it.
The Engine API has grown like crazy over the last months and years and our single
engine.ts
file doesn't hold all the code any more.This is a much needed basic structural refactoring, putting things into its own files, adding (some) additional code docs and generally making the overloaded
engine.ts
file leaner and better readable again.Still WIP, will push some additional clean-ups, likely ready for review at the end of the day though.