More robust calldata decoding, more shrinking #887
Conversation
msooseth
force-pushed
the
fix-decoding
branch
from
9e1f750 to
d30e8db
Compare
More shrinking, fixing printing Changelog Update limit
src/EVM/ABI.hs
Outdated
| where | ||
| isDynamic t = abiKind t == Dynamic | ||
|
|
||
| decudeBufRobust :: [AbiType] -> Expr Buf -> (AbiVals, String) |
There was a problem hiding this comment.
Can we fix the existing method instead of introducing new one?
We can adjust the types to better convey the necessary information.
decodeBuff can return an Either, where Left can pass along the error string. Right can be AbiVals where we remove NoVals.
We can extend it with Maybe ByteString to return the unprocessed bytes back, if there are any.
I would prefer to leave it up to the caller to decide what to do in such case, and not decide on the message here in the callee.
I was thinking that in our use case, where we want to pretty print the Solidity function call, the extra bytes do not pose any problem and we can safely ignore them, no?
There was a problem hiding this comment.
the extra bytes do not pose any problem -> in this case, they don't seem to. But we also use this method to decode things for cheatcodes, for example, where we cannot ignore it. Either would work there, actually. Let's say that I don't want to warn the user about the trailing bytes (not my preference, but OK). I'd still need to return a Left, because some functions rely on this being exact. Should I return a Left AbiVals?
There was a problem hiding this comment.
What do you mean by "relying on this being exact"?
My idea is that the return type would be something like Either String (AbiVals, Maybe ByteString).
The callers then implement their way of handling each situation.
Which could be an error, or success with all bytes consumed, or success with some leftover bytes.
There was a problem hiding this comment.
Aaaaaah I see! OK, that make sense!
There was a problem hiding this comment.
I think it should now be how we want it to be? Let me know what you think :)_
blishko
left a comment
blishko
left a comment
There was a problem hiding this comment.
I would prefer to not decide on the error message for trailing bytes inside decodeBuf, but just return enough information for the callers to decide on their own.
That's why I was suggesting return type Either String (AbiVals, Maybe ByteString).
But I am also OK if you prefer to decide on the error string here already.
|
Sorry, I would need to think about this more to do it in a different way. I'll think about it, or you can maybe explain how you mean it? Sorry, I am not up to par for this. |
|
Let's merge this then now. We can try something different later and see what we think about it. |
Description
This PR does two things.
Firstly, it reduces the shrink limit to 32B from 1024B. This means that we now try shrinking even small buffers. This should make sure that trailing useless bytes are not shown to the user, even if the calldata is small.
Secondly, this PR makes the decoding more robust, by introducing
decodeBufFuzzythat is a bit more relaxed, allowing trailing bytes at the end of the buffer, and returning more meaningful errors/warnings to the user, which are displayed at the end of the mostly decoded calldata, such as:calldata: prove_negative_886(-42) -- with trailing bytes: [0,0,0]Test
eq-issue-with-length-cex-bug679has been updated to remove the trailingf. It's not needed. I checked with evm.codes' playground, and the two bytecodes return, respectively 0x1 and 0x0, for the generated, shorter, CEX.Fixes #886
Checklist