-
Notifications
You must be signed in to change notification settings - Fork 329
utilize pydantic models from eest on t8n #1359
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
base: forks/osaka
Are you sure you want to change the base?
utilize pydantic models from eest on t8n #1359
Conversation
k: v if (isinstance(v, str) and v.startswith("0x")) else hex(v) | ||
for k, v in acc["storage"].items() | ||
} | ||
state = t8n.json_to_state(alloc_json) |
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.
This could be made more efficient without having to convert from pydantic to json and then json to state
json_output["alloc"] = json_state | ||
json_output["result"] = json_result | ||
output: TransitionToolOutput = TransitionToolOutput.model_validate( | ||
json_output, context={"exception_mapper": self.exception_mapper} |
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.
This could potentially also be simplified without having to convert json and then json to pydantic
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 think now a direct conversion or individually validating each part of TransitionToolOutput
wouldn't really add any efficiency gains here
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.
So my review amounts basically to just a general Python review.
@gurukamath would you mind giving this a review with an eye specifically to design/architecture, since you've been working in this area lately?
@@ -80,26 +82,18 @@ class T8N(Load): | |||
"""The class that carries out the transition""" | |||
|
|||
def __init__( | |||
self, options: Any, out_file: TextIO, in_file: TextIO | |||
self, options: Any, out_file: TextIO, in_file: TransitionToolRequest |
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.
Does this require changes in the daemon?
output: TransitionToolOutput = TransitionToolOutput.model_validate( | ||
json_output, context={"exception_mapper": self.exception_mapper} | ||
) | ||
return output |
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.
Hm, so this change would break the command-line output? Is that what we want?
def to_canonical_withdrawal(raw): | ||
return t8n.fork.Withdrawal( | ||
index=U64(raw.index), | ||
validator_index=U64(raw.validator_index), | ||
address=raw.address, | ||
amount=U256(raw.amount), | ||
) |
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.
Should probably hoist this out of read_withdrawals
to avoid creating a bunch of closures that aren't used.
if AuthorizationCls is None: | ||
from ethereum.osaka.fork_types import Authorization as AuthorizationCls |
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.
If the fork doesn't have an Authorization
type, but the test transaction does, we should probably error, no? Shoving an osaka type into a frontier block is going to lead to at least an exception somewhere.
if isinstance(val, (bytes, bytearray)) and len(val) == 20: | ||
return Bytes20(val) | ||
if isinstance(val, (bytes, bytearray)) and len(val) == 0: | ||
return Bytes20(b"\x00" * 20) | ||
if isinstance(val, (bytes, bytearray)): | ||
return Bytes20(val.rjust(20, b"\x00")) |
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.
Are these special cases necessary or does the rjust
handle them all properly?
return Bytes20(bytes(val).rjust(20, b"\x00")) | ||
|
||
# build the canonical transaction | ||
if tx_cls.__name__ == "FeeMarketTransaction": |
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 use issubclass
here instead?
# build the canonical transaction | ||
if tx_cls.__name__ == "FeeMarketTransaction": | ||
return tx_cls( | ||
chain_id=U64(tx.chain_id), |
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.
Instead of repeating most of the arguments in every branch, would it make sense to do something like:
arguments = {
'nonce': U256(tx.nonce),
# The rest of the common items...
}
if issubclass(tx_cls, AccessListTransaction):
arguments["access_list"] = convert_access_list(tx.access_list)
elif issubclass(tx_cls, BlobTransaction):
...
return tx_cls(**arguments)
# output_dict = json.loads(out_stream.getvalue()) | ||
# output: TransitionToolOutput = TransitionToolOutput.model_validate( | ||
# output_dict, context={"exception_mapper": self.exception_mapper} | ||
# ) |
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.
Remove code instead of commenting it out.
5479b3c
to
5a17bd9
Compare
) -> None: | ||
self.out_file = out_file | ||
self.in_file = in_file | ||
self.options = options | ||
self.forks = Hardfork.discover() | ||
|
||
if "stdin" in ( |
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.
Would we still be able to consume json files if needed? We will need to continue supporting that use case.
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.
yes! with this commit dc31f96 I added the dual input option where we preserve the older json input route
) | ||
|
||
self.chain_id = parse_hex_or_int(options.state_chainid, U64) | ||
self.alloc = Alloc(self, in_file.input.alloc) |
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.
Perhaps we should re-use the Alloc, Env and Txs types defined by EEST. And if the json inputs are provided, we could just serialize/de-serialize using pydantic. That way, we can get of a lot of redundant code in env.py
and t8n_types.py
.
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.
right! :) The first thing i tried was to re-use the types (pydantic models) from EEST (to not have these intermediary classes at all) But seems like our execution-spec code isn't directly able to work with those (without some translations/conversions)? To be able to utilize the EEST types directly would definitely be the most efficient - any suggestions?
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.
Would something like this work?
- If EEST types are the input: EEST types -> EELS BlockEnvironment -> run the t8n tool
- If json input: json input -> EEST types -> EELS BlockEnvironment -> run the t8n tool
You might have to transform the EEST Alloc
to the relevant fork's State
dataclass but I think this is still much cleaner than what we currently have.
What was wrong?
Related to Issue ##1124
How was it fixed?
Cute Animal Picture