-
Notifications
You must be signed in to change notification settings - Fork 644
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
Add create2 #269
Add create2 #269
Conversation
link #224 |
@@ -43,5 +44,6 @@ def _get_sharding_intrinsic_gas(transaction_data, transaction_code): | |||
return ( | |||
GAS_TX + | |||
num_zero_bytes * GAS_TXDATAZERO + | |||
num_non_zero_bytes * GAS_TXDATANONZERO | |||
num_non_zero_bytes * GAS_TXDATANONZERO + | |||
GAS_TXCREATE |
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 may miss something, could you tell me why GAS_TXCREATE
is included in every sharding tx?
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.
You are right. It should not be included in every tx.
@@ -190,3 +191,69 @@ def __call__(self, computation): | |||
if computation.msg.is_static: | |||
raise WriteProtection("Cannot modify state while inside of a STATICCALL context") | |||
return super(CreateEIP150, self).__call__(computation) | |||
|
|||
|
|||
class Create2(CreateEIP150): |
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.
Is there a reason for inheriting CreateEIP150
instead of CreateByzantium
?
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.
CreateByzantium
and Create2
both override __call__
function so I make Create2
inherit from CreateEIP150
and copy the is_static
check from CreateByzantium
.
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.
Since the shard chain doesn’t have as many JSON test cases as the non-shard chain now, add some boundary test cases might help.
Some test cases like:
- When
contract_address != transaction.to
? - When
is_collision is True
? - Assert
vm.block.header.gas_used
afterapply_transaction
tests/core/vm/test_sharding_vm.py
Outdated
simple_transfer_contract_address, | ||
simple_contract_factory_bytecode, | ||
CREATE2_contract_bytecode, | ||
CREATE2_contract_address, | ||
) | ||
|
||
|
||
def test_sharding_transaction(shard_chain_without_block_validation): # noqa: F811 |
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.
Since it's a test in test_vm_state.py
test_sharding_vm.py
, mind updating the test name to test_sharding_apply_transaction
?
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.
Sure. I will update the name. Do you mean also moving it to test_vm_state.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.
ohhhhh sorry, I meant test_sharding_vm.py
!
Tests updated. Thanks. |
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.
Some cleanup suggestions.
evm/vm/forks/sharding/opcodes.py
Outdated
} | ||
|
||
|
||
SHARDING_OPCODES = merge( | ||
copy.deepcopy(BYZANTIUM_OPCODES), | ||
NEW_OPCODES | ||
) | ||
|
||
|
||
SHARDING_OPCODES.pop(opcode_values.CREATE) |
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 popping this value out, can we do the following a few lines up.
from cytoolz import dissoc
SHARDING_OPCODES = merge(
dissoc(copy.deepcopy(BYZANTIUM_OPCODES), opcode_values.CREATE),
NEW_OPCODES
)
tests/core/vm/test_sharding_vm.py
Outdated
# Second test: contract that deploy new contract with CREATE2 | ||
second_deploy_tx = new_sharding_transaction( | ||
CREATE2_contract_address, | ||
b'', |
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 you change this to use keyword arguments so that it's clear what all of these similar parameters are.
tests/core/vm/test_sharding_vm.py
Outdated
chain = shard_chain_without_block_validation | ||
first_failed_deploy_tx = new_sharding_transaction( | ||
simple_transfer_contract_address, | ||
b'', |
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.
Same request here regarding keyword arguments.
tests/core/vm/test_sharding_vm.py
Outdated
# Next, complete deploying the contract | ||
successful_deploy_tx = new_sharding_transaction( | ||
simple_transfer_contract_address, | ||
b'', |
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.
and here
tests/core/vm/test_sharding_vm.py
Outdated
# Second case: deploy to existing account | ||
second_failed_deploy_tx = successful_deploy_tx | ||
computation = vm.apply_transaction(second_failed_deploy_tx) | ||
assert computation.is_error |
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.
Maybe add a check that isinstance(computation._error, ContractCreationCollision)
tests/core/vm/test_sharding_vm.py
Outdated
|
||
vm = chain.get_vm() | ||
computation = vm.apply_transaction(first_failed_deploy_tx) | ||
assert computation.is_error |
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.
Maybe add a check for isinstance(computation._error, IncorrectContractCreationAddress)
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.
LGTM
How was it fixed?
Add CREATE2 opcode
execute_transaction
to make suretransaction.to
is the same as computed contract addressCute Animal Picture