Add transaction_context object to Computation #327
Conversation
lrettig
changed the title
| execution. | ||
| """ | ||
| _gas_price = None | ||
| _origin = None |
There was a problem hiding this comment.
I don't think we need the @property versions of these. Should be good just using them directly as properties.
There was a problem hiding this comment.
I copied the behavior of https://github.com/hwwhww/py-evm/blob/66a858f6ed1b4a02e35c311ed3255440499c8f9a/evm/vm/execution_context.py. Should this be different for some reason?
There was a problem hiding this comment.
The reason for why we protect fields of ExecutionContext is that we want to make sure the fields only be assigned in __init__ and won't be changed in other places.
But I'm not sure what's the use cases of TransactionContext.
There was a problem hiding this comment.
@pipermerriam originally said:
This would be an object that we treat as immutable
So it should be the same, no?
There was a problem hiding this comment.
@hwwhww @property methods don't actually provide any protection from mutation unless you create a setter which throws. I'd be good with doing that but it should probably be done with our own decorator that automatically creates the setter function. Something like a new decorator:
@immutable_property
def something(self):
...There was a problem hiding this comment.
@pipermerriam
So sorry for that I just realized I missed and haven't seen this comment(#273 (comment)) 15 days ago! Agree with you about using explicit @read_only_property or @immutable_property decorator.
There was a problem hiding this comment.
No big deal. Easy enough for us to make that cleanup now (mind opening an issue for it?)
@lrettig I'm fine with leaving current @property methods in place and cleaning them up with a stand-alone PR with the @immutable_property/read_only_property decorator.
There was a problem hiding this comment.
Sounds good, thanks for the clarification
lrettig
force-pushed
the
transaction-context
branch
2 times, most recently
from
48eddcb to
aebe522
Compare
Creates a BaseTransactionContext class and a child
FrontierTransactionContext class.
Removes properties which remain constant (gas_price, origin) from
Message and move them to the new TransactionContext class.
Generate a TransactionContext inside execute_transaction and attach it
to the Computation object. Pass this down into child Computations.
Begin reworking code which reads the (now-removed)
Message.{origin,gas_price}.
Create from instance rather than specific class in execute_transaction. Pass transaction context object into get_computation.
Update a couple of opcodes and a util method that rely on origin and gas_price
Updates a bunch of tests, adds one new test. These tests are all passing.
Bugfix: transaction_context cannot be instantiated without an origin. Use the transaction.sender when instantiating.
Errors from blockchain fixtures were being swallowed silently. Output these along with a full stack trace to make debugging easier.
lrettig
force-pushed
the
transaction-context
branch
from
aebe522 to
5602f72
Compare
| @@ -295,6 +302,8 @@ def test_state_fixtures(fixture, fixture_vm_class): | |||
| computation, _ = vm.apply_transaction(transaction) | |||
| except ValidationError as err: | |||
| transaction_error = err | |||
| LOGGER.warn("Got transaction error:") | |||
There was a problem hiding this comment.
@pipermerriam This is a little messy, not sure if this is how logging should happen inside tests, but this made my life much easier when running tests as these errors were being swallowed silently. Let me know if this is okay.
I also want to understand: can a transaction raise a TransactionError but the test still pass? Are there tests which expect an error?
There was a problem hiding this comment.
There is no TransactionError in py-evm that I know of so I'm not entirely sure what you're referring to here.
In the case of this section of code, yes, transactions can error and the tests still pass because some of the tests are designed to throw errors, testing invalid transactions do not succeed to execute in the EVM.
|
Perfect. Thanks. That's what I meant.
…On Fri, Feb 9, 2018 at 02:13 Piper Merriam ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/json-fixtures/test_state.py
<#327 (comment)>:
> @@ -295,6 +302,8 @@ def test_state_fixtures(fixture, fixture_vm_class):
computation, _ = vm.apply_transaction(transaction)
except ValidationError as err:
transaction_error = err
+ LOGGER.warn("Got transaction error:")
There is no TransactionError in py-evm that I know of so I'm not entirely
sure what you're referring to here.
In the case of this section of code, yes, transactions *can* error and
the tests still pass *because* some of the tests are designed to throw
errors, testing invalid transactions do not succeed to execute in the EVM.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#327 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADKbNGzWdqEsPjFZphXUQjDx5E9c72N2ks5tSzk0gaJpZM4RxcMt>
.
|
Rebase and update to work with ethereum#327 changes. Adds code_address to allow execution of arbitrary code in the context of an existing contract per ethereum#306 (comment).
Rebase and update to work with ethereum#327 changes. Adds code_address to allow execution of arbitrary code in the context of an existing contract per ethereum#306 (comment).
* Minimal proof of concept for exec bytecode Construct a message and pass it through apply_computation * Add missing origin param * Add transaction_context Rebase and update to work with #327 changes. Adds code_address to allow execution of arbitrary code in the context of an existing contract per #306 (comment). * Allow code_address to be None * Bugfix: pass tx_context to apply_computation * Parametrize the computation_getter Per @pipermerriam's suggestion
Ports over the changes made to master in ethereum#327. Creates a BaseTransactionContext class and a child FrontierTransactionContext class. Removes properties which remain constant (gas_price, origin) from Message and move them to the new TransactionContext class. Generate a TransactionContext inside execute_transaction and attach it to the Computation object. Pass this down into child Computations. Begin reworking code which reads the (now-removed) Message.{origin,gas_price}. Update tests. Errors from blockchain fixtures were being swallowed silently. Output these along with a full stack trace to make debugging easier.
Ports over the changes made to master in ethereum#327. Creates a BaseTransactionContext class and a child FrontierTransactionContext class. Removes properties which remain constant (gas_price, origin) from Message and move them to the new TransactionContext class. Generate a TransactionContext inside execute_transaction and attach it to the Computation object. Pass this down into child Computations. Begin reworking code which reads the (now-removed) Message.{origin,gas_price}. Update tests. Errors from blockchain fixtures were being swallowed silently. Output these along with a full stack trace to make debugging easier.
Ports over the changes made to master in ethereum#327. Creates a BaseTransactionContext class and a child FrontierTransactionContext class. Removes properties which remain constant (gas_price, origin) from Message and move them to the new TransactionContext class. Generate a TransactionContext inside execute_transaction and attach it to the Computation object. Pass this down into child Computations. Begin reworking code which reads the (now-removed) Message.{origin,gas_price}. Update tests. Errors from blockchain fixtures were being swallowed silently. Output these along with a full stack trace to make debugging easier.
Ports over the changes made to master in ethereum#327. Creates a BaseTransactionContext class and a child FrontierTransactionContext class. Removes properties which remain constant (gas_price, origin) from Message and move them to the new TransactionContext class. Generate a TransactionContext inside execute_transaction and attach it to the Computation object. Pass this down into child Computations. Begin reworking code which reads the (now-removed) Message.{origin,gas_price}. Update tests. Errors from blockchain fixtures were being swallowed silently. Output these along with a full stack trace to make debugging easier.
Ports over the changes made to master in ethereum#327. Creates a BaseTransactionContext class and a child FrontierTransactionContext class. Removes properties which remain constant (gas_price, origin) from Message and move them to the new TransactionContext class. Generate a TransactionContext inside execute_transaction and attach it to the Computation object. Pass this down into child Computations. Begin reworking code which reads the (now-removed) Message.{origin,gas_price}. Update tests. Errors from blockchain fixtures were being swallowed silently. Output these along with a full stack trace to make debugging easier.
Ports over the changes made to master in #327. Creates a BaseTransactionContext class and a child FrontierTransactionContext class. Removes properties which remain constant (gas_price, origin) from Message and move them to the new TransactionContext class. Generate a TransactionContext inside execute_transaction and attach it to the Computation object. Pass this down into child Computations. Begin reworking code which reads the (now-removed) Message.{origin,gas_price}. Update tests. Errors from blockchain fixtures were being swallowed silently. Output these along with a full stack trace to make debugging easier.
Creating a PR to track this work in progress. From #262.