Implement and add web3.eth.signTransaction test to integration tests #1277
Conversation
njgheorghita
force-pushed
the
implement-eth-signTransaction
branch
19 times, most recently
from
a168543
to
60e7c9a
Compare
njgheorghita
changed the title
njgheorghita
force-pushed
the
implement-eth-signTransaction
branch
9 times, most recently
from
ad85d35
to
4dc2a94
Compare
njgheorghita
force-pushed
the
implement-eth-signTransaction
branch
from
4dc2a94
to
15064e2
Compare
| if geth_signed_tx: | ||
| assert result['raw'] == geth_signed_tx | ||
| else: | ||
| assert result['raw'] == actual.rawTransaction |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
njgheorghita
changed the title
| } | ||
| COINBASE_PK = '0x58d23b55bc9cdce1f18c2500f40ff4ab7245df9a89505e9b1fa4851f623d241d' | ||
| result = web3.eth.signTransaction(txn_params) | ||
| actual = web3.eth.account.signTransaction(txn_params, COINBASE_PK) |
There was a problem hiding this comment.
This may be a dumb question, but why do we need both web3.eth.signTransaction and web3.eth.account.signTransaction?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yup, pretty much the only difference is whether your private key is managed by your python app or your node.
There was a problem hiding this comment.
Got it! That was the missing link 🔗. Thanks!
There was a problem hiding this comment.
No dumb questions.... only dumb people 😈
njgheorghita
force-pushed
the
implement-eth-signTransaction
branch
from
af4eda4
to
57ff10c
Compare
njgheorghita
force-pushed
the
implement-eth-signTransaction
branch
from
57ff10c
to
6106bad
Compare
|
Ready for final review |
| @@ -282,7 +282,7 @@ The following methods are available on the ``web3.eth`` namespace. | |||
| 'gasLimit': '0x2fefd8', | |||
| 'gasUsed': '0x0', | |||
| 'hash': '0xc78c35720d930f9ef34b4e6fb9d02ffec936f9b02a8f0fa858456e4afd4d5614', | |||
| 'logsBloom':'0x000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000, | |||
| 'logsBloom':'0x000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000', | |||
There was a problem hiding this comment.
Nice catch! How do you feel about turning these into doctests so that we catch errors like this in the future? If it's too much for this PR, I'll add a new issue. I think it is a good idea to start moving that direction for all of our docs.
There was a problem hiding this comment.
Agreed, I'm all for doctest and autodoc, but I think it's a somewhat hefty task and deserves it's own issue / pr(s).
What was wrong?
eth_signTransactionas defined in EIP 1474 is not implemented yet in web3.How was it fixed?
wrote
web3.eth.signTransactionand added integration tests for Parity & GethCute Animal Picture