Make sure setters and getters cover edge cases

[mpoke]

The methods used to access the CCV state (both on provider and consumer) should be callable with all kind of params without panic.

[danwt]

IMO it's not always a good idea to program as defensively as this. Only the cases which can actually be called with nil, empty ect params should contain the checks.

Also, do we really want to not panic? There are some panics already, for example if byte or json decoding fails. It might be reasonable to panic in some places.

[danwt]

I think we should discuss this

• Decide strategy for handling errors #174

[shaspitz]

Regarding this issue, it seems that we all agree that every error should be handled in some place (linters enforce this). What's still unclear to me is if it's best to always panic when something unexpected happens within the app layer.

A good example relates to #254. We have two choices to handle the situation in which a duplicate proposal is passed (ie. a proposal for a consumer chainID that is already secured).

1. We panic and halt the chain if such a duplicate proposal was passed
2. We gracefully handle a duplicate proposal on-chain and discard it

Unless I'm missing something in my understanding of this specific case, it seems that the decision comes down to, do we want extra safety around proposal-related human errors and/or attack vectors? Or do we trust that provider validators would catch all "duplicate proposals" and reject them?

Thoughts?

[mpoke]

We panic and halt the chain if such a duplicate proposal was passed

I don't think we should panic in such a case since there is no threat to the system. Dropping the duplicated proposal is the easiest way.

[mpoke]

In general I think that

• we should panic in case of programmer errors that shouldn't happen, e.g., using MustMarshal in setters;
• we should return errors that are handled at a higher layer if it's something that can be triggered by data (i.e., user TXs), e.g., returning an error or false when looking for the channel ID for a consumer chain that was not registered.

The idea of this issue is to make sure that the code within setters / getters does not panic without us wanting that behavior, e.g., store.Get Panics on nil key (see https://github.com/cosmos/cosmos-sdk/blob/b6f867d0b674d62e56b27aa4d00f5b6042ebac9e/store/types/store.go#L197).

[shaspitz]

Got it, and thank you for the clarification of intention on this issue. Agreed on the points made here

[jtremback]

All of the panics (with the exception below) on getters and setters in consumer and provider keeper.go are related to database corruption- i.e. there is stuff in the database that should not be in there, and there is no way for our program to recover from it. However, we are inconsistent. Some database corruption errors in some getters and setters are returned. We should probably at least be consistent about it.

Exception: This panic is in AfterUnbondingInitiated which is called by the staking module and then calls back into the staking module. It is unclear to me what the proper response would be here. Panic may still be the best option, because it is due to the staking module malfunctioning somehow and we cannot recover.

	// Call back into staking to tell it to stop this op from unbonding when the unbonding period is complete
	if err := h.k.stakingKeeper.PutUnbondingOnHold(ctx, ID); err != nil {
		panic(fmt.Errorf("unbonding could not be put on hold: %w", err))
	}

[danwt]

Agree ^: we should panic iff the situation is not recoverable.

[danwt]

Closing as there is no defined closing criteria