Refactor the Environment dataclass#1053
Refactor the Environment dataclass#1053gurukamath wants to merge 77 commits intoethereum:forks/praguefrom
Conversation
The current spec implementaion of the point evatuation pre-compile inttoduced in cancun makes use of eth2spec (consensus-specs). This leads to potential dependency conflicts like when execution-specs wants to use a different version of py_ecc than the consensus-specs. This commit removes the need to depend on eth2spec and implements the necessary components of KZG
Implement Pectra Devnet 4 changes for requests and 7702
Extract t8n arguments from daemon query string
Fix up black and mypy in `fix-t8n-requests`
Encode requests correctly in `t8n`
src/ethereum/cancun/fork.py
Outdated
| if block.ommers != (): | ||
| raise InvalidBlock | ||
|
|
||
| env = vm.Environment( |
There was a problem hiding this comment.
I really like this change.
gurukamath
force-pushed
the
refactor-env
branch
from
b771715 to
c0bd64c
Compare
| """ | ||
|
|
||
| block_env: BlockEnvironment | ||
| tx_env: TransactionEnvironment |
There was a problem hiding this comment.
Would it make sense to introduce a third "environment" for the frame?
class BlockEnvironment:
...
class TransactionEnvironment:
...
class FrameEnvironment:
caller: Address
target: Union[Bytes0, Address]
current_target: Address
gas: Uint
value: U256
data: Bytes
code_address: Optional[Address]
code: Bytes
depth: Uint
should_transfer_value: bool
is_static: bool
accessed_addresses: Set[Address]
accessed_storage_keys: Set[Tuple[Address, Bytes32]]
parent_evm: Optional["Evm"]
class Message:
block: BlockEnvironment
transaction: TransactionEnvironment
frame: FrameEnvironmentThere was a problem hiding this comment.
This would line up the nomenclature pretty well but I have been looking at the Message dataclass serving the same purpose as the FrameEnvironment dataclass in your suggestion. I don't have any strong opinions on either choice tbh.
There was a problem hiding this comment.
It would be purely for symmetry and organizational purposes. I can imagine the docs looking like:
An EVM message contains three levels of contextual information: block level, transaction level, and call frame level.
There was a problem hiding this comment.
The symmetry argument sounds good to me but the question largely boils down to complexity since introducing FrameEnvironment could be a large change. Also, I think we can introduce FrameEnvironment in the next iteration to avoid a huge PR
There was a problem hiding this comment.
I think that makes sense.
|
Closing in favor of #1131 |
…thereum#1053) * fix(github): Features for legacy forks * docs: update changelog --------- Co-authored-by: danceratopz <danceratopz@gmail.com>
…thereum#1053) * fix(github): Features for legacy forks * docs: update changelog --------- Co-authored-by: danceratopz <danceratopz@gmail.com>
4 participants
What was wrong?
The
Environmentdata class has some transaction scope attributes likesender,originand a few others. Ideally, this data class should only have block scope attributes and the transaction scope attributes should be moved to theMessagedata class.This refactor has a few really nice benefits including
apply_bodyfunction (We can now pass just a singleenvparameter instead of a large number of individual attributes that might change with hard forks)t8nHow was it fixed?
Move transaction scope attributes to
MessageCute Animal Picture