feat(cheatcodes): add negative assertions for expectEmit and expectRevert using count syntax

[fubhy]

Negative assertions, e.g. asserting that a certain call was not performed or a certain event was not emited are not very useful on their own (because they might pass for the wrong reasons, e.g. a negative assertion always passes if you simply don't do anything of course).

However, when paired with positive assertions (e.g. asserting that the tested functionality actually performed the desired state mutation), they can be very useful to assert that a certain code path was skipped in doing so. This can be useful to test e.g. that a certain conditional was skipped.

I realize that this can already be achieved at the test level with the testFail prefix, but that comes with different ergonomics vs. negating individual assertions.

[gakonst]

Good idea. Does any other project do it, so that we can have some example? What would be your ideal syntax?

(cc @brockelmore @mds1 @transmissions11)

[mds1]

I like this idea, agreed it'd be useful, but I don't have any examples offhand. It's easy to extend DSTest to add an assertNeq method so I think we can ignore that for the scope of this issue. Then we have expectEmit, expectCall, and expectRevert. Inverting expectRevert isn't necessary, so we're down to just calls and events, but let me know if there's anything we might be missing here.

It would also be useful to test that you don't jump to a certain function within your contract (and similarly have an expectJump), though I'd guess this is currently difficult for the same reasons as #432

Some syntax ideas:

1. a vm.expectInverse() cheat code that watches for the next vm.expect*() and inverts it. With an approach like this I worry we're cluttering up tests a bit, e.g. you now need 4 lines to test an event is not emitted, and because of the order required you can't wrap these into a helper method
2. similar to 1 but instead called as vm.expectInverse(vm.expect*()), which saves a line, but I think we'd have to abi encode this without a preprocessor—also this then becomes the only cheatcode that takes another cheat code as input, but maybe that's ok

There's probably another question of how topics should be considered when expecting an event not to be emitted. I'd guess most or all use cases only are really interested in only ensuring that topic0 is not found, as opposed to ensuring topic0 is emitted but that topic1/data does not have some value. The easiest behavior is to just invert the result of expectEmit, but then everyone would be calling vm.expectEmit() with 4 unused input args every time they want to assert an event isn't emitted which feels a bit gross

[fubhy]

@mds1 Agreed wrt 1 being a bit verbose / cluttered. I like 2, but maybe we can make that a bit shorter: What about vm.not(vm.expectCall(...)) ?

Slightly off topic, but for events in particular, wouldn't it make sense to also have a more ergonomic version that just tests for topic0 via vm.expectEmit("Transfer(address, address, uint)").

[mds1]

Slightly off topic, but for events in particular, wouldn't it make sense to also have a more ergonomic version that just tests for topic0 via vm.expectEmit("Transfer(address, address, uint)")

That'd be a useful overload here, because then we can shorten it to s single line vm.not(vm.expectEmit("Transfer(address, address, uint)")). IIRC we discussed an expectEmit syntax like that but didn't implement it since it doesn't extend nicely to taking input args of the expected params, though I may be misremembering / @brockelmore might recall

[odyslam]

+1 for vm.not before expectNotEmit. For some protocols (like Nomad), where the functionality is heavily based on event emission, some times there are no other signals (or variables to be read) that can verify that something did NOT happen, except the NOT emission of an event.

[mattsse]

this is definitely useful and we should add this imo.

otherwise, you can't test the inverse of expectEmit.

flipping the impl and adding expectNotEmit should be easy.

will try to squeeze this in

[ChainsightLabs]

+1

[DanTheDev-il]

def a +1 from my side for this change, too.

Hardhat has a similar functionality that I found really easy and handy to use:

expect(<call).to.not.emit(, ).withArgs(....)

the ".withArgs" part is optional. So either I check only if an event was emitted or not or I can also check if the args match (or dont).

[0xDEnYO]

+1

[reubenr0d]

+1

Another way to go about this would be with cheatcodes that count the number of times a function is called or event emitted. That way we can not only assert that call/emit was made multiple times, but also if it was not made at all using the same cheat.

There is an issue for expectCallCount (#4513), but we could also do something similar for events as well.
Eg. expectEmit(bool,bool,bool,bool, uint256 count)

[mds1]

I'm supportive of adding overloads with a count arg for both the expectEmit and expectCall cheats, since that accomplishes both this issue and #4513 with the same UX/set of cheats. I saw in the support telegram that you'd be interested in working on this, should I assign to you?

Also if anyone has objections to that syntax and has a reason to prefer e.g. expectNoEmit please share 🙂

[reubenr0d]

Yep, you can assign it to me @mds1

[mattsse]

woops this one slipped through... sorry about that

nice @reubenr0d !

lmk if you have questions/need some pointers

[Evalir]

cc @mds1 i guess we went for the count syntax arg (which needs to be fixed)—do we still wanna implement negative assertions?

[mds1]

Yea the count approach is how we'll implement negative assertions. We have it for expectCall but still need it for expectEmit and expectRevert

[zerosnacks]

cc @grandizzy possibly relevant to recent cheatcode additions

[grandizzy]

cc @grandizzy possibly relevant to recent cheatcode additions

yep, is on my cheatcode expect* list to tackle