Add sharding transaction test #252

NIC619 · 2018-01-08T09:46:25Z

How was it fixed?

Add sharing transaction test.

Steps are:

calculate contract address to be deployed
initiate a shard chain(without block validation) with the address derived above funded
deploy the contract
send transaction to the contract

I'm working on the contract code right now which is similar to the user account contract but simpler(parse the transaction data and transfer amount of ether to recipient).

Cute Animal Picture

…w_sharding_transaction

hwwhww · 2018-01-08T15:14:03Z

As the discussion in #238, if we are going to create a Shard class which mirrors the Chain class and a ShardVM which mirrors the VM class, how about creating a new test_shard_vm.py file for these tests?

pipermerriam · 2018-01-08T15:43:32Z

tests/core/vm/test_vm.py

+    # specified amount of ether to recipient.
+    # TODO: update contract_deployment_code
+    contract_deployment_code = b''
+    contract_addr = generate_create2_contract_address(b'', contract_deployment_code)


What do you think about changing the name to generate_CREATE2_contract_address to make it a bit clearer that we're referring to the CREATE2 opcode.

I will update it in CREATE2 PR.

pipermerriam

Looks well formed. A few minor cleanup suggestions.

pipermerriam · 2018-01-08T15:55:12Z

tests/core/fixtures.py

+        }
+    }
+    chain = klass.from_genesis(shard_chaindb, genesis_params, genesis_state)
+    chain.funded_address = funded_addr


Instead of storing this on the chain object, can we just use the funded_address fixture in any place where we need access to this address. I'm worried that setting a precedent for tacking values onto objects will lead to confusing tests down the road when it's hard for someone to differentiate what is a formal Chain API and what is just part of the test setup.

pipermerriam · 2018-01-08T15:56:25Z

tests/core/vm/test_vm.py

+    transfer_tx = new_sharding_transaction(tx_initiator, recipient, amount, b'', b'', b'')
+    computation = vm.execute_transaction(transfer_tx)
+    assert not computation.is_error
+    with vm.state.state_db(read_only=True) as state_db:


Just had the thought that we could provide access to a read-only statedb without the context manager as part of the state object, and have the context manager API be only necessary for when write operations are necessary.

That makes sense. Just to make sure, context manager's job here is to (1) double check no state were written in read_only mode and (2) decommission the state to prevent further modification to db, correct?
Does it involve managing(opening/closing/cleaning) the actual data base file?

…d make shard chain contract fixture

…ion data in ShardingTransaction

jannikluhn · 2018-01-11T15:58:30Z

The simple_transfer_contract is intended just for testing, correct? If so I'd not put it into evm/auxiliary but move it somewhere to the tests subdirectory.

NIC619 · 2018-01-11T16:10:37Z

@pipermerriam I made some updates and it is ready for review/merge, please have a look, thank you.

Updates:

Switch to NestedTrieBackend for shard chain testing for now to circumvent account data access restriction. Will update it once access list generating helper function is out.
Update contract code fixture for testing. The contract is a simple contract which takes in destination, value, each padded to 32 bytes, and transfer value amount of ether to destination. Steps are, firstly, fund the contract address in genesis, then deploy the contract and finally execute the transfer transaction.

NIC619 · 2018-01-11T16:11:42Z

@jannikluhn ahh you are right. I will move them to tests directory.

pipermerriam · 2018-01-11T17:20:49Z

tests/core/fixtures.py

+    """
+    # TODO: Comment this out to use NestedTrieBackend to prevent access list validation.
+    # Uncomment this to use FlatTrieBackend once access list generation is implemented.
+    # shard_chaindb = BaseChainDB(get_db_backend(), state_backend_class=FlatTrieBackend)


I'm not sure if this is what you want, but assuming you want to run this using both state backends you can do that in a very clean way as follows.

@pytest.fixture(params=(FlatTrieBackend, NestedTrieBackend)) def shard_chain_without_block_validation(request): shard_chaindb = BaseChainDB(get_db_backend(), state_backend_class=request.param) ...

Sorry my comment was misleading. What I mean is to comment out FlatTrieBackend and use NestedTrieBackend instead for now. After the helper function which generates access list for a transaction is implemented, we can uncomment it and use FlatTrieBackend. I will update this part.

pipermerriam · 2018-01-11T17:21:14Z

tests/core/fixtures.py

+        'extra_data': constants.GENESIS_EXTRA_DATA,
+        'timestamp': 1501851927,
+        # 'state_root': decode_hex(
+        #     '0x9d354f9b5ba851a35eced279ef377111387197581429cfcc7f744ef89a30b5d4')


Reason for this being commented out? Can it be removed?

You are right. This can be removed.

pipermerriam · 2018-01-11T17:23:34Z

tests/core/vm/contract_fixture.py

+contract_bytecode = b'0x61003e567401000000000000000000000000000000000000000060205260003560205181101558575060006000600060006020356000356000f1155857005b61000461003e0361000460003961000461003e036000f3'  # noqa: E501
+
+# address where this contract will be deployed
+contract_address = b'\xdb\xcd\xfc\xf2\xea!\xcd\x0c\xa5d\xaay\x8b;\xf0\xe4\xe2\x98S\xca'


I assume this value is based on the contract bytecode? If so, it seems like this value is likely to end up out-of-sync if the bytecode changes. I'd suggest one of:

make this a computed value.

put a big loud comment here indicating how to compute this value and at the bytecode above indicating that if the bytecode is changed, then this value needs to be changed also.

Will update it to a computed value.

…ontract_address computed value

NIC619 added 2 commits January 8, 2018 17:08

add two helper functions: shard_chain_without_block_validation and ne…

dc3f778

…w_sharding_transaction

add sharding transaction test

f68e279

NIC619 added the PR state: WIP label Jan 8, 2018

NIC619 requested a review from pipermerriam January 8, 2018 09:46

pipermerriam reviewed Jan 8, 2018

View reviewed changes

pipermerriam approved these changes Jan 8, 2018

View reviewed changes

hwwhww added the eth2.0 label Jan 9, 2018

NIC619 added 3 commits January 10, 2018 13:36

comment out FlatTrieBackend, comment out state root in shard chain an…

ebf40d3

…d make shard chain contract fixture

remove target in ShardingTransaction, remove rlp encoding of transact…

b72351f

…ion data in ShardingTransaction

update testing contract code fixture

94021a4

NIC619 added Needs Review and removed PR state: WIP labels Jan 11, 2018

move test_sharding_transaction to test_sharding_vm.py

b32eaf6

move contract_fixture to tests/core/vm

35608a2

pipermerriam approved these changes Jan 11, 2018

View reviewed changes

pipermerriam added PR state: merge ready and removed Needs Review labels Jan 11, 2018

update get_db_backend comment, remove commented state_root and make c…

2d3d894

…ontract_address computed value

NIC619 merged commit 2f5c87e into ethereum:sharding Jan 14, 2018

NIC619 deleted the add_sharding_transaction_test branch January 14, 2018 04:53

NIC619 restored the add_sharding_transaction_test branch January 14, 2018 05:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sharding transaction test #252

Add sharding transaction test #252

NIC619 commented Jan 8, 2018

hwwhww commented Jan 8, 2018

pipermerriam Jan 8, 2018

NIC619 Jan 9, 2018

pipermerriam left a comment

pipermerriam Jan 8, 2018

pipermerriam Jan 8, 2018

NIC619 Jan 9, 2018

jannikluhn commented Jan 11, 2018

NIC619 commented Jan 11, 2018

NIC619 commented Jan 11, 2018

pipermerriam Jan 11, 2018

NIC619 Jan 12, 2018

pipermerriam Jan 11, 2018

NIC619 Jan 12, 2018

pipermerriam Jan 11, 2018

NIC619 Jan 12, 2018

Add sharding transaction test #252

Add sharding transaction test #252

Conversation

NIC619 commented Jan 8, 2018

How was it fixed?

Cute Animal Picture

hwwhww commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannikluhn commented Jan 11, 2018

NIC619 commented Jan 11, 2018

NIC619 commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment