Review panic strategy

[davepuchyr]

Should we use panic or return an error?

[fdymylja]

Panic, where there is a panic it means pre validity checks are failing, so there is faulty code that is allowing invalid requests to go through. So even if I returned an error I'd need to panic on top level and hence lose some parts of stack trace.

Msgs and Queries validate these things. If some things pass through we have faulty code.

I don't like the design, it's not intuitive, but if I'm checking request validity with a msg, as forced in cosmos sdk, should i repeat the same checks again while processing it? If yes, then there's a lot of logic changes to do. Cosmos SDK KVStore does not return errors, so my idea was to go through with keepers without returning errors. If an error would come out while calling a keeper then it means the request I'm processing is faulty and my pre-checks are not doing their job correctly.

[davepuchyr]

OK, so the onus is on us to test every single possible contingency and ensure that we don't encounter a panic. I don't love the strategy but I understand it given our time-to-market constraints.

[fdymylja]

Yes, as soon as @orkunkl is ready with simulations, my best guess is that we will have one single point of control to test everything.
ATM what we test is handlers behaviour, what they should do if an account is expired, does not exist and so on, every check that is done comparing the state is done in a handler.

Stateless checks are handled through Msg and Query validation, so: domain name empty, account name empty and so on.

ATM we don't have a single point to check every interaction as everything is a sparse. This is a good design as every component should behave independently from the other. The problem is that cosmos forces us to divide things that we should not divide.

Our handlers would not require panics if statelss checks were done alongside stateful ones.

So if something goes through stateless checks and makes editing state fail (trying to iterate an empty account key in an index) what should I do? If I returned an error I'd be left with partial state (which can happen during iterations)? Would the state be rolled back?

At least now we know for sure that if we panic state is cached and won't be applied so we're 100% sure that we don't 'infect' our state.
Second, with #115 we can now get the panic log and know why the daemon did not process the request.
Third, the application won't stop because of cosmos recovery middleware ( which I don't like by the way.. )

Let me know your thoughts: @davepuchyr, @orkunkl

[orkunkl]

Your implementation looks solid. Actually I checked out cosmos-sdk to see any issues related to this topic and fortunately recently someone made some improvements to the base app which we can use in the next minor updates I assume. Here it is: cosmos/cosmos-sdk#6053

[orkunkl]

We can use the recoveryMiddleware that is introduced in this PR 💥

[fdymylja]

I just wish we could edit the handler processor middleware... so we could add caches and not apply cache if handler returns an error, we could add top level logging there etc. it would solve a lot of stuff and make debugging 100000% easier... but sadly.

[davepuchyr]

@orkunkl detailed the capabilities of the simulations to me this morning. It was great news. Also, @fdymylja, it is now clear to me that you have put a lot of thought into the matter, so my confidence level is now high.

Thanks, guys.