Enhancement: Add decoding of multicalls to @truffle/decoder

[haltman-at]

Addresses #4133. This PR adds interpretations to decodings returned by @truffle/decoder, with one interpretation currently existing: multicall.

This is mostly pretty straightforward -- the decoder, after decoding a transaction, will check whether it's a multicall (as in: name multicall, arguments a single bytes[]), and, if it is, will then recurse over the subcalls and decode them, sticking the result in interpretations.multicall. There's not much to say about the logic here! I also added a test of this functionality.

I also updated CalldataDecodingInspector so that it will check for interpretations.multicall, and, if it finds it present, print out the the calls made rather than the raw bytestrings for each entry. In the process I found that the existing logic for formatFunctionLike wasn't really working like I wanted so I fixed that up a bit too.

Really, the thing on this PR that really needs looking over is the types. Here are the questions I have for @gnidan:

1. I made interpretations optional, and only included it when there's something to put in there. Is this right? Should it be required? (Note that if we make it required, and then later also make it required on individual values when we do reverse ENS resolution, I'm going to have to go updating all the wrap functions... not that that'd actually be hard, just annoying, but...)
2. You'll notice that multicall is not CalldataDecoding[], but rather (CalldataDecoding | null)[]. This is because, what if one of the individual bytes decodes to an error? Yeah, that shouldn't normally happen, but, like, we should be prepared for the possibility. I realize this isn't ideal but I couldn't think of a better way of handling this. Like, if you want the actual error, you can always look in arguments! In the inspector, I just had these nulls turn into a "could not decode" message... should I perhaps have it actually look in arguments so I can have it display the actual error message in case of an error?

But yeah, mostly nothing complicated here!

[gnidan]

1. I made interpretations optional, and only included it when there's something to put in there. Is this right?

I'm leaning towards thinking it should be required and empty when there's nothing. This way it will be in front of people's faces when they integrate the format... if it's optional, it feels a bit too surprise-y when it does show up. But I could be convinced otherwise.

2. You'll notice that multicall is not CalldataDecoding[], but rather (CalldataDecoding | null)[]. This is because, what if one of the individual bytes decodes to an error? Yeah, that shouldn't normally happen, but, like, we should be prepared for the possibility. I realize this isn't ideal but I couldn't think of a better way of handling this. Like, if you want the actual error, you can always look in arguments! In the inspector, I just had these nulls turn into a "could not decode" message... should I perhaps have it actually look in arguments so I can have it display the actual error message in case of an error?

Hm, we could make it (CalldataDecoding | Error)[] or provide some other representation that can communicate error cases.

Alternatively... maybe we should treat it as a bad interpretation altogether, and indicate the error in the interpretation itself... like CalldataDecoding[] | MulticallDecodingError.

All of these sound kinda clunky, though. Maybe errors won't be so common and we can just stick with your "or null"... not sure.

[haltman-at]

I'm leaning towards thinking it should be required and empty when there's nothing. This way it will be in front of people's faces when they integrate the format... if it's optional, it feels a bit too surprise-y when it does show up. But I could be convinced otherwise.

OK, I've made it required.

[haltman-at]

Hm, we could make it (CalldataDecoding | Error)[] or provide some other representation that can communicate error cases.

Alternatively... maybe we should treat it as a bad interpretation altogether, and indicate the error in the interpretation itself... like CalldataDecoding[] | MulticallDecodingError.

Yeah, the problem with this is that our error system is set up for reporting errors in decoding of particular variables, not of transactions as a whole. So we'd either have to mash together different systems that weren't meant to be used that way (something you've objected to in the past before and which I would definitely object to here), or make a whole new system for this, and it just doesn't seem worth it.

All of these sound kinda clunky, though. Maybe errors won't be so common and we can just stick with your "or null"... not sure.

Yeah errors occuring in that particular context should be pretty rare. I think most decoding problems would result in multicall simply not being attached as an interpretation at all; to get these nulls you'd have to be doing something pretty weird. Moreover, as I mentioned, you can always look outside the interpretations field if you really want to know about the errors.

All this still leaves the question though @gnidan: Should I have the inspector do just that, to handle such errors better?

[gnidan]

Additional point: my recent Solidity Summit demo featured this PR's behavior prominently, and in doing so I realized a gap in what this supports... namely:

• multicall v2 (see repo). there are a couple different forms within this v2, so you might want to check for all those more comprehensively (like how they define aggregate and tryAggregate separately)
• Uniswap v3's "nonstandard" multicall, which defines a function that takes a preceding deadline argument (see IMulticallExtended)

I did some hacking to add these in this commit, but the variety of forms here suggests to me that we want to handle the differences more robustly.

(Is it possible to be less strict when detecting? Like can we say "use the interpretation if it looks like it might be a multicall?" I'm guessing this is hard)

[haltman-at]

I did some hacking to add these in this commit, but the variety of forms here suggests to me that we want to handle the differences more robustly.

I mean, these could just be different interpretations and we could update it as need be...? :-/ Yeah I dunno I don't really have a better idea offhand.

(Is it possible to be less strict when detecting? Like can we say "use the interpretation if it looks like it might be a multicall?" I'm guessing this is hard)

Yeah I don't really have any idea how that would work.

[haltman-at]

OK, I've added the additional multicall interpretations (together with tests for most of them and updates to the inspector). Notes on these:

1. I grouped blockAndAggregate into aggregate and tryBlockAndAggregate into tryAggregate rather than giving these separate interpretations, since they have the same arguments, and only the return values differ. If you want to know which this one is, well, again, the full decoding is still there; you can just look at decoding.abi.name.
2. Not sure what you'll think of the format; hoping it's OK. Like e.g. I included requireSuccess as a direct boolean, and deadline as a BN.
3. The way I handled aggregate in the inspectors is a little hacky. Oh well? :-/

So uh yeah hope this is good?

[gnidan]

Moreover, as I mentioned, you can always look outside the interpretations field if you really want to know about the errors.

All this still leaves the question though @gnidan: Should I have the inspector do just that, to handle such errors better?

This is probably worth doing, but if you want to open an issue for it, I don't think it needs to be this PR

[gnidan]

This looks good. I think the additional interpretation is probably the right call. Thank you!