Tests transaction receipts for reverted evm execution #184
Conversation
JoshOrndorff
changed the title
| }); | ||
| }); | ||
|
|
||
| it("should provide a tx receipt after failed deployment", async function () { |
There was a problem hiding this comment.
Ideally, those tests should be split in 2:
- should provide a tx receipt after failed deployment (verify the result is valid)
- should store the tx after reverted (verify querying the receipt returns the valid data)
| ); | ||
|
|
||
| expect( | ||
| await customRequest(context.web3, "eth_sendRawTransaction", [tx.rawTransaction]) |
There was a problem hiding this comment.
Any reason not to use context.web3.eth.sendRawTransaction ?
There was a problem hiding this comment.
I followed the style of the existing tests.
frame/ethereum/src/lib.rs
Outdated
| @@ -210,6 +210,8 @@ decl_module! { | |||
|
|
|||
| Pending::append((transaction, status, receipt)); | |||
|
|
|||
| Self::handle_exec(exit_reason)?; | |||
There was a problem hiding this comment.
We actually still cannot return error here. It must succeed, because at this stage the runtime state has already changed.
|
@JoshOrndorff Can you pull master? It should fix the |
|
Okay, so the desired behaviour is that calls to transact should succeed even if the evm execution fails? That fine, but a bit unintuitive. So I'll revert my changes to the rust code, merge master, and keep this PR open for the tests. |
JoshOrndorff
changed the title
* lots of debugging lines * Add integration test to demonstrate the issue * Revert "lots of debugging lines" This reverts commit 0d125a2. * One possible fix for the bug * Fix constants in tests * Fix warning * Fix runtime test * revert my attempt to fix
This PR adds an integration test that demonstrates missing transaction receipts when transactions are reverted as described in #178. It also makes an attempt to solve that issue, by delaying the error handling in pallet_ethereum's
transactuntil after thePendingstorage item has been written.I do have some open questions about this work:
statusin the transaction receipts (whether it is reverted or not)Pending) even when the transaction is going to fail? It seems like the correct thing in this case, but it is contrary to "normal" best practices "verify first, then write".