Properly handle event decoding in Solidity 0.8.20

[haltman-at]

So, Solidity 0.8.20 is out, and it changes which events get listed in a contract's ABI! That means our existing event allocation code won't work with 0.8.20 contracts. Fortunately, they also added the usedEvents property to contract nodes (much like the existing usedErrors), which lets us distinguish when we're in this case, and also helps us handle it. So, now we have two event allocation pathways; one for pre-0.8.20, and one for post-0.8.20.

In addition to this, there's also one change to the event decoding return type; definedIn can now be null, as distinct from undefined (same as with errors). The use of null here means that the event was defined at file level, as opposed to undefined, which means we don't know where it was defined. Now you may say, but Solidity doesn't allow file-level events! Sure, but it will soon, and as long as I was doing this, I thought it made sense to future-proof for that. I've updated the event decoding inspector to account for this.

Anyway, back to the allocation changes -- let's break down how this affects each of the three event allocation functions.

For getEventAllocations, it basically just means that inheritance handling is bypassed if usedEvents is present. Which is nice, because it means you get to skip the confusing part of the function! Well, OK, one of the confusing parts of the function.

For getEventAllocationsForContract, it means we now have two separate allocation pathways... you'll notice this function now heavily parallels getReturndataAllocationsForContract, this is not a coincidence. :P Note that the meaning of undefined differs between the two paths, though... in the old path cases of undefined are left in for inheritance processing, while in the new path they're filtered out since there won't be any.

Finally getEventAllocation is the function whose changes you might find the most confusing, so let's go over that. First off, the function signature has changed; you can now pass in an eventNode, since now we might already know that going in. We also introduce a new variable, definedInNode; previously there was no need for this since the defined-in node was always the contract node, but now this is no longer the case! If node is defined but definedInNode is not, that's interpreted as a file-level event.

Anyway, you can see that if eventNode is not passed in, we run the old code, except that we set definedInNode equal to contractNode. By contrast, if eventNode is passed in, then we run the new code, which thankfully is much simpler. It sets node to eventNode (since we already know it, rather than having to find it like in the old case) and searches all the contracts to find the event we're looking for, setting definedInNode as appropriate (or leaving it undefined if nothing was found -- this indicates a file-level event).

(You may say, to determine if an event is file-level, shouldn't you search all the SourceUnit nodes to see if it's defined directly under one of them? Ideally, yes, but we don't have that information in the current setup and I didn't want to deal with changing that right now.)

However, due to the special handling of library events, the new code does do one more check -- if it's a library event, and this contract isn't the library that defined it, we don't allocate it. Library events are always considered to be "in play", so we leave the event alone so its defining library can allocate it; we don't want it to appear twice, after all.

But that's it! Sorry, I know I'm modifying some confusing code here, but I hope you'll find that the code paths I've added are less confusing than the older existing code paths. :P

As for tests... I wanted to get this up quickly, so I didn't test it as thoroughly as I otherwise might have. I just updated the debugger and decoder tests to 0.8.20 and checked that they still passed. Hopefully that's enough!

[haltman-at]

Side note: After writing this, I had the thought -- hey, wait, can't we get rid of the searching by making use of the scope field? Unfortunately no; it turns out that event definitions (and error definitions) don't include that. Dang.

Function definitions do, so notionally that could be used in #6050, but that would require putting them in referenceDeclarations, and that would bog down that PR in naming disputes when I want to get it merged quickly. :P

[gnidan]

Is this a breaking change?

[gnidan]

Also, does this mean we should include an update to codec-components in this PR, to handle this new null case?

[haltman-at]

Ooh, good point, you're absolutely right. I guess this is technically breaking? Bleh. And yeah I'll need to rebase this PR and update components. Thanks for catching that!

(...is this the entirety of your review or is this just a single early comment?)

[gnidan]

It was an early comment, but now I think it's also the entirety of the review. We should get this and #6050 into the same release to avoid extra minor bumps

[haltman-at]

Oh hm actually -- it looks like the current component for events doesn't use the definedIn field at all. That'll probably change at some point, but for now, this does not require updating any components after all!