-
Notifications
You must be signed in to change notification settings - Fork 0
Use TransitionTool.default_tool
#1
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
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.
I assume the idea is for the EELS T8N wrapper (in EELS) to subclass the TransitionTool
and set that as the default while configuring pytest. Is that the right way to understand this?
edit: nvm, I see now, you have already clarified this in the PR description. I'll get started on it.
from ethereum_test_types import Alloc, Environment, Transaction | ||
|
||
|
||
class EELST8NWrapper(TransitionTool, ABC): |
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.
I wanted to have certain constraints/guarantees on the implementation of the EELS t8n wrapper within the EEST framework. Hence I added the abstract base class. Are we ok with removing those?
evm_group.addoption( | ||
"--evm-bin", | ||
action="store", | ||
dest="evm_bin", | ||
type=Path, | ||
default="ethereum-spec-evm-resolver", | ||
default=None, |
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.
As per a previous discussion we wanted to keep the resolver the default and until we are comfortable to pull the trigger, have the need to explicitly specify that the EELS T8n entry point is to be used.
evm_group.addoption( | ||
"--evm-bin", | ||
action="store", | ||
dest="evm_bin", | ||
type=Path, | ||
default="ethereum-spec-evm-resolver", | ||
default=None, | ||
help=( | ||
"Path to an evm executable (or name of an executable in the PATH) that provides `t8n`." | ||
" Default: `ethereum-spec-evm-resolver`." |
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.
" Default: `ethereum-spec-evm-resolver`." |
With this change we need to, in https://github.com/ethereum/execution-specs/pull/1280/files#diff-a31c7ed5d35f5ed8233994868c54d625b18e6bacb6794344c4531e62bd9dde59, change
EELST8NWrapper.set_default(EELST8N)
forTransitionTool.set_default_tool(EELST8N)
, and in https://github.com/ethereum/execution-specs/pull/1280/files#diff-b573d3c72a5ca81f324c895cb2cbc3aa8fcd6978465aeb67293dd0f129c4a56a we need to rename_evaluate_eels_t8n
toevaluate
, make some small changes, and in theory it should work.