Skip to content

Implement and add web3.eth.signTransaction test to integration tests #1277

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 15, 2019
Merged

Implement and add web3.eth.signTransaction test to integration tests #1277

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15064e2
Add web3.eth.signTransaction test to integration tests
njgheorghita Mar 13, 2019
6106bad
Add tx params to test
njgheorghita Mar 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions tests/integration/go_ethereum/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
Web3ModuleTest,
)

GETH_SIGNED_TX = b'\xf8j\x80\x85\x040\xe24\x00\x82R\x08\x94\xdcTM\x1a\xa8\x8f\xf8\xbb\xd2\xf2\xae\xc7T\xb1\xf1\xe9\x9e\x18\x12\xfd\x01\x80\x86\xee\xca\xc4f\xe1\x16\xa0\xbb7^\x1f\xf0\x03(P\x07|\x053Q\xd3M\xf1\x83\xe9\xdcp\xdc\x02\xb4\xe7`\x85\xcd\x84\xdb\xb4\xd0\xaa\xa07\x8cl\xd7\xa6R\x01\xfaW\x0e\x0f\xc1_$\xdf`\x8dO\x18\x1dC\xbc\x87\x8fud\xd2R*W\xfd4' # noqa: E501


class GoEthereumTest(Web3ModuleTest):
def _check_web3_clientVersion(self, client_version):
Expand Down Expand Up @@ -59,6 +61,9 @@ def test_eth_estimateGas_with_block(self,
web3, unlocked_account_dual_type
)

def test_eth_signTransaction(self, web3, unlocked_account):
super().test_eth_signTransaction(web3, unlocked_account, GETH_SIGNED_TX)


class GoEthereumVersionModuleTest(VersionModuleTest):
pass
Expand Down
1 change: 1 addition & 0 deletions tests/integration/test_ethereum_tester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ def func_wrapper(self, eth_tester, *args, **kwargs):

class TestEthereumTesterEthModule(EthModuleTest):
test_eth_sign = not_implemented(EthModuleTest.test_eth_sign, ValueError)
test_eth_signTransaction = not_implemented(EthModuleTest.test_eth_signTransaction, ValueError)

@disable_auto_mine
def test_eth_getTransactionReceipt_unmined(self, eth_tester, web3, unlocked_account):
Expand Down
17 changes: 17 additions & 0 deletions web3/_utils/module_testing/eth_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,23 @@ def test_eth_sign(self, web3, unlocked_account_dual_type):
)
assert new_signature != signature

def test_eth_signTransaction(self, web3, unlocked_account, geth_signed_tx=None):
txn_params = {
'from': unlocked_account,
'to': unlocked_account,
'value': 1,
'gas': 21000,
'gasPrice': web3.eth.gasPrice,
'nonce': 0,
}
COINBASE_PK = '0x58d23b55bc9cdce1f18c2500f40ff4ab7245df9a89505e9b1fa4851f623d241d'
result = web3.eth.signTransaction(txn_params)
actual = web3.eth.account.signTransaction(txn_params, COINBASE_PK)
Copy link
Collaborator

@kclowes kclowes Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a dumb question, but why do we need both web3.eth.signTransaction and web3.eth.account.signTransaction?

Copy link
Contributor Author

@njgheorghita njgheorghita Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not dumb at all, it could very well be a dumb idea. My thinking was that web3.eth.account.signTransaction (aka Account.signTransaction from eth-account does pretty much the same thing as sending a eth_signTransaction JSONRPC request to a client, which is what web3.eth.signTransaction does. Since the eth-account implementation was well-done, it seemed to me a fair test comparison to make sure that web3.eth.signTransaction is performing as expected

Copy link
Contributor

@carver carver Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, pretty much the only difference is whether your private key is managed by your python app or your node.

Copy link
Collaborator

@kclowes kclowes Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! That was the missing link 🔗. Thanks!

Copy link
Member

@pipermerriam pipermerriam Mar 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No dumb questions.... only dumb people 😈

if geth_signed_tx:
assert result['raw'] == geth_signed_tx
else:
assert result['raw'] == actual.rawTransaction
Copy link
Contributor Author

@njgheorghita njgheorghita Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carver Any thoughts on a better test for this? I'm having trouble figuring out why the eth_signTransaction call to geth is behaving differently from parity - any possible insight into that?

Also according to the spec the jsonrpc response should only include the raw signed transaction. However, both parity and geth also return the original tx params submitted in the request. What behavior do we want web3.eth.signTransaction to emulate? Just simple return the signed tx? or also include the original transaction params?

Copy link
Member

@pipermerriam pipermerriam Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is Web3.py's job to enforce spec compliance, that is going to be a separate thing that clients start testing for. For now, we should probably be quite forgiving with what data is returned (with respect to allowing these extra fields and formatting them in the expected way)

In the future, I could see it being reasonable for web3.py to even decode the transaction and populate these fields when the clients stop returning them.

Copy link
Contributor

@carver carver Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with ⬆️


def test_eth_sendTransaction_addr_checksum_required(self, web3, unlocked_account):
non_checksum_addr = unlocked_account.lower()
txn_params = {
Expand Down
1 change: 1 addition & 0 deletions web3/_utils/rpc_abi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
'eth_newFilter': FILTER_PARAMS_ABIS,
'eth_sendRawTransaction': ['bytes'],
'eth_sendTransaction': TRANSACTION_PARAMS_ABIS,
'eth_signTransaction': TRANSACTION_PARAMS_ABIS,
'eth_sign': ['address', 'bytes'],
# personal
'personal_sendTransaction': TRANSACTION_PARAMS_ABIS,
Expand Down
5 changes: 5 additions & 0 deletions web3/eth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,11 @@ def sign(self, account, data=None, hexstr=None, text=None):
"eth_sign", [account, message_hex],
)

def signTransaction(self, transaction):
return self.web3.manager.request_blocking(
"eth_signTransaction", [transaction],
)

@apply_to_return_value(HexBytes)
def call(self, transaction, block_identifier=None):
# TODO: move to middleware
Expand Down
10 changes: 10 additions & 0 deletions web3/middleware/pythonic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,15 @@ def to_hexbytes(num_bytes, val, variable_length=False):
transaction_formatter = apply_formatters_to_dict(TRANSACTION_FORMATTERS)


SIGNED_TX_FORMATTER = {
'raw': HexBytes,
'tx': transaction_formatter,
}


signed_tx_formatter = apply_formatters_to_dict(SIGNED_TX_FORMATTER)


WHISPER_LOG_FORMATTERS = {
'sig': to_hexbytes(130),
'topic': to_hexbytes(8),
Expand Down Expand Up @@ -344,6 +353,7 @@ def to_hexbytes(num_bytes, val, variable_length=False):
),
'eth_sendRawTransaction': to_hexbytes(32),
'eth_sendTransaction': to_hexbytes(32),
'eth_signTransaction': apply_formatter_if(is_not_null, signed_tx_formatter),
'eth_sign': HexBytes,
'eth_syncing': apply_formatter_if(is_not_false, syncing_formatter),
# personal
Expand Down
1 change: 1 addition & 0 deletions web3/providers/eth_tester/defaults.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ def personal_send_transaction(eth_tester, params):
)),
'getCode': call_eth_tester('get_code'),
'sign': not_implemented,
'signTransaction': not_implemented,
'sendTransaction': call_eth_tester('send_transaction'),
'sendRawTransaction': call_eth_tester('send_raw_transaction'),
'call': call_eth_tester('call'), # TODO: untested
Expand Down