-
Notifications
You must be signed in to change notification settings - Fork 146
Port eth2 binary main
style to eth1
#3712
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
Conversation
Making the two projects more similar makes it easier to maintain a cohesive user experience over time: * remove metrics logging - this is not viable for the amount of metrics that we have (no other project has this either) * remove `EthContext` - looks like a refactoring leftover * reuse more `nimbus_binary_common` utilities instead of homegrown versions * bump eth2
Draft until status-im/nimbus-eth2#7526 has been merged |
# noinline to keep it in stack traces | ||
proc main() {.noinline, raises: [CatchableError].} = | ||
const | ||
banner = "Nimbus verified proxy " & fullVersionStr |
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 import execution_chain/version_info
and use FullVersionStr
? proxy will remain in sync with execution_chain that way
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.
Also nimbus_portal_client
will require this eventually (fairly soon). Should be sufficient to import just FullVersionStr
from execution client version file?
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.
bette done that way because the releases will be clubbed together. UPDATE: And it makes more sense to maintain one version file rather than three
Merged. |
return false | ||
true | ||
|
||
proc getNetKeys*(rng: var HmacDrbgContext, netKey: string): Result[KeyPair, string] = |
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.
portal client has code with a similar goal but obviously differently implemented 😅 : https://github.com/status-im/nimbus-eth1/blob/master/portal/common/common_utils.nim#L81
Probably something we want to also make common in another iteration (and potentially with rework of nimbus-eth2 keystore to also use that...)
e3634f2
to
9a145b2
Compare
Making the two projects more similar makes it easier to maintain a cohesive user experience over time:
EthContext
- looks like a refactoring leftovernimbus_binary_common
utilities instead of homegrown versionsIn particular, these changes are sufficient to support #3646 .