fix: skip logging during ante simulation

[milapsheth]

Description

Todos

• Unit tests
• Manual tests
• Documentation
• Connect epics/issues
• Tag type of change

Steps to Test

Expected Behaviour

Other Notes

[fish-sammy]

if simulate { return...} to avoid unnecessary nested if blocks

[milapsheth]

That would duplicate the next call at the end though

[fish-sammy]

which is fine

[milapsheth]

Code duplication seems worse (especially related to ante handle checks) compared to a bit more nesting

[fish-sammy]

I strongly disagree, especially in golang. Do you wanna start seeing?!

    if err := ...; err == nil {
        if err := ...; err == nil {
            if err := ...; err == nil {
                ...
            }
        }
    }

    return ...

[fish-sammy]

What exactly makes you think this kind of duplicate is bad? What makes ante handler special in this case?

[fish-sammy]

nested-ifs compared to early exit creates the very problem that when someone reads it, he has to figure out what happens if simulate is true by scrolling down the entire block to just find out nothing is gonna happen. It's confusing and unreadable. When one sees if simulate { return ... }, it's clear immediately that this function isn't gonna do anything when simulate is set to true (no logging, no mutation to ctx, no nothing) and also makes a lot of sense since logging shouldn't happen if in the simulation mode.

[milapsheth]

Updated it. I get the benefit of early returns on failure cases. But in this case, it's not a failure, so requires duplication of logic. My concern is if someone changes the logic of next at the end, they need to remember to do this for the early exit too. This is specifically concerning for ante handle checks since they perform some authentication and are critical. The vote module had a duplication related bug like this if I recall. But anyways, up to you, since I haven't coded in Go much.

[fish-sammy]

They wouldn't need to remember to do anything to the early exit. The whole point here is that we wanna make sure NOTHING should ever happen in the logging ante handler if simuate is true. If they have to do something to the early exit, I can see a lot of stuff being questionable about what that person is doing.

[fish-sammy]

ante handler isn't anything special. It's just like middlewares in express.js or interceptors in gRPC.