Add support for decoding EIP 838 revert reasons #188
Conversation
|
I might be missing something, but it doesn't look like the CI failure is due to anything I changed. |
|
I’m fixing the CI issues right now; some dependency for process/browser.js nextTick is failing, so I need to lock in the package-lock, I think... It only affects the test cases in phantomjs. I’ll update this ticket soon. Thanks! |
|
It's been a while. Any feedback? |
|
This is next on my list. I’m just polishing up some of the last odds and ends on the TypeScript implementation, which is a major version change, so we can have some non-backward compatible changes and make the API smooth. I will be testing the revert reasons over the next few days. :) |
|
I've been playing with this in the TypeScript branch, and just to make sure I understand, this only works for call, not transactions, right? I was thinking there might be more data in the receipt now, but I see now it is just calls that provide the reason. |
|
@ricmoo you are correct and it only works for calls (at least this is true for Geth; I haven't tested with other clients). In theory it should be possible for clients to return the revert reason in the transaction receipt, but it doesn't look like anyone is doing this right now. |
|
This now works in the v4 branch. Let me know if you have any issues with it. :) |
We use in-process ganache which throws on an RPC error. This means all of our tests get a nice revert error thrown when testing against ganache. This is not possible with RPC providers and a revert with reason result is returned. Our callAsync doesn't handle this and attempts to decode the revert with reason error log as a successful log, which results in an error while decoding. This only works with our fork of ethers ethers-io/ethers.js#188 and will need to be re-worked when updating to Ethers.js 4
We use in-process ganache which throws on an RPC error. This means all of our tests get a nice revert error thrown when testing against ganache. This is not possible with RPC providers and a revert with reason result is returned. Our callAsync doesn't handle this and attempts to decode the revert with reason error log as a successful log, which results in an error while decoding. This only works with our fork of ethers ethers-io/ethers.js#188 and will need to be re-worked when updating to Ethers.js 4
We use in-process ganache which throws on an RPC error. This means all of our tests get a nice revert error thrown when testing against ganache. This is not possible with RPC providers and a revert with reason result is returned. Our callAsync doesn't handle this and attempts to decode the revert with reason error log as a successful log, which results in an error while decoding. This only works with our fork of ethers ethers-io/ethers.js#188 and will need to be re-worked when updating to Ethers.js 4
EIP 838 introduces revert reasons. Revert reasons have been implemented in Solidity, Geth, Parity, and probably many other projects within the ecosystem. This PR introduces support for decoding them.
For some background, we have a test case in which we expect a contract call to fail. Normally, it returns an address, but in the case of failure it reverts with a reason. Prior to this change, the reason was being parsed as if it were an address, which resulted in an invalid address of all 0's. With this change in place, ethers.js throws with an error message that includes the reason.
Currently, both Geth and Parity (and possibly other implementations) are following the convention of encoding these revert reasons as
08c379a0followed by an ABI encoded string.