fix: make expect4337Revert dynamic to allow optional msg

[mehtaculous]

https://github.com/rhinestonewtf/modulekit/issues/71

expect4337Revert now takes in a string value as a parameter

this PR likely needs to be updated since it was a bit unclear why the integer value was hard-coded

[kopy-kat]

Thank you for working on this, I think the issue was a bit vague, but the idea is that a dev could use expect4337Revert as is (ie without msg) or with one, similar to how foundry does this: https://book.getfoundry.sh/cheatcodes/expect-revert#expectrevert - would you be able to implement it in that way?

[mehtaculous]

that makes more sense, so I left the current expect4337Revert as is so that it just writes 1 when an error is expected with no message

I also added 2 additional functions that either take in bytes4 or bytes values, similar to the foundry doc. since the data types are different, I had to implement new write and read functions for each one inside Log.sol. do I need to have separate storage slots for each error type or can they just all use keccak256("ModuleKit.ExpectSlot") ?

I'm not entirely sure if that is what is being asked here but if it looks good, I can go ahead and add a test for those checks

[kopy-kat]

I think it would make sense to just have them in one slot since only one expect revert will be active at one time and then when parsing this, there is a way to determine if it should check the revert reason/bytes4 or to not check it - does that make sense

[kopy-kat]

btw lmk when you want me to take a look at this again

[mehtaculous]

I think I'm still a bit lost here..right now, the way it's set up is that there are 3 separate read functions: getExpectRevert, getExpectRevertBytes4 and getExpectRevertBytes which all return different data types. how exactly would you know which function to call in order to check for the revert reason/bytes4 or not ?

Also, is the testexec__RevertWhen__UserOperationFails test complete? It doesn't assert any values and when I console log the expectRevert value after the instance.exec call, I get 0 instead of 1..

might be helpful if you take a look at the current code

[kopy-kat]

yep will check out the implementation now. Regarding the testexec__RevertWhen__UserOperationFails this test should be complete in that it should only pass if the op fails - this is whats being tested. Yep, after a userOp execution, the expectRevert value is reset so that it only takes effect for the next userOp and not all following ones too

[mehtaculous]

yep will check out the implementation now. Regarding the testexec__RevertWhen__UserOperationFails this test should be complete in that it should only pass if the op fails - this is whats being tested. Yep, after a userOp execution, the expectRevert value is reset so that it only takes effect for the next userOp and not all following ones too

I think the test name is a bit misleading, was expecting it to actually revert. rather it just writes to the ExpectSlot and then makes a call to exec..

[kopy-kat]

Well, the test still has to pass so we expect the exec call to revert. This naming is actually pretty common and you can also find it in the foundry book testing best practices