Handle abi decoding failures when unpacking revert reason in EVM #3419
Conversation
| var reasonCode: UInt256 | ||
| discard decode(data.toOpenArray(4, data.len() - 1), 0, 0, reasonCode) | ||
| assign(reason, panicReasons.getOrDefault(reasonCode.truncate(int))) | ||
| try: |
There was a problem hiding this comment.
I think we need to come up with our own decode functions instead of relying on web3 decoder.
We also try to eliminate exception from EVM, except for unavoidable cases. But in this case I think we can avoid exception.
There was a problem hiding this comment.
Well the abi decoding is not trivial to implement so I'd rather not spend time on writing my own abi decoder. This web3 implementation does work but it just turns out that it doesn't handle failure scenarios very well and so it can throw Defect if the input is not valid abi.
I think using this implementation is fine since nimbus-eth1 already depends on nim-web3. Now if the reason for wanting to eliminate exceptions from the EVM is performance then I'll move this outside the EVM.
There was a problem hiding this comment.
Catching Exception isn't well-defined in Nim, so can't be relied on anyway.
This needs to be fixed in nim-web3, or, yeah, nim-web3 bypassed in this case.
There was a problem hiding this comment.
Then I will offer my alternatives
There was a problem hiding this comment.
Yes agreed.
For now, I've moved the usage of unpackRevertReason outside the EVM since, as far as I'm aware, this is only needed by EVM calls used by the RPC endpoints anyway.
Also now catching a Defect to be more specific. Added a comment to explain the reasoning and also will make a note to do a separate PR in nim-web3 to improve the error handling in the library.
There was a problem hiding this comment.
it's precisely Defect that is unreliable to catch (and by extension Exception since it might be a Defect) .. ergo, it should be avoided here.
Instead of returning a decoded string reason, could return the raw bytes do the decoding elsewhere though no matter where it's done, a Defect should never be raised (ie this must be fixed by fixing the decoder not to raise a defect or by pre-checking the values)
No description provided.