New validation for keeper fields

[shaspitz]

Description

cosmos/gaia#2197 fixed an incorrect ordering of keeper initialization in gaia's app.go. Before Simon's fix, gaia's ccv provider keeper held a pointer reference to a zeroed (outdated) external keeper. In short, ccv keepers should never have references to nil pointers, or references to concrete types which are zero-valued for that type. This PR adds a test which validates what I've described.

This new test would have caught the mentioned evidence keeper bug, and will hopefully prevent new bugs or regressions of this same kind in the future.

To confirm test behavior, I've created two gaia branches that import this PR's ICS branch, see comparison. One branch does fail w/o Simon's fix, and the other branch passes w/ the fix ✅

Type of change

• Testing: Adds testing

Regression tests

n/a

New behavior tests

n/a

[sainoe]

Great work @smarshall-spitzbart

[MSalopek]

Thank you for raising the issue and going through all this effort!

I understand the concern, but please consider making a validator func instead of exposing private fields.

The provided tests are already checking that the fields are not zero values.

[shaspitz]

The provided tests are already checking that the fields are not zero values.

This is not true, as no e2e test caught the evidence keeper bug. All e2e tests have to be manually crafted and its easy to overlook their use, especially for external contributors.

That being said, I'm not opposed to the idea of making this a runtime check with panics instead of a test

[MSalopek]

The provided tests are already checking that the fields are not zero values.

This is not true, as no e2e test caught the evidence keeper bug. All e2e tests have to be manually crafted and its easy to overlook their use, especially for external contributors.

That being said, I'm not opposed to the idea of making this a runtime check with panics instead of a test

Meant to say:

the tests provided in this PR are only checking for zero values

Apologies for my bad english

[shaspitz]

@MSalopek thanks for the feedback! The panic design is indeed better than what I first designed, now we don't have to expose private fields. I've updated the PR accordingly, and updated the gaia branches originally mentioned in the PR description. The gaia branches pass/fail as expected 👍

[MSalopek]

@smarshall-spitzbart Thank you for updating the PR!

[glnro]

Wondering in the future if it's worth refactoring to have mustValidateFields be generic for keepers, then pass in a set of validations instead per keeper....

validations := map[string]func(Keeper) bool{
	"number of fields in provider keeper is not 13": func(keeper Keeper) bool { return reflect.ValueOf(k).NumField() != 13 },
	"cdc is zero-valued or nil": func(k Keeper) bool { return reflect.ValueOf(k.cdc).IsZero() },
	"storeKey is zero-valued or nil": func(keeper Keeper) bool { return reflect.ValueOf(k.storeKey).IsZero()},
}

...

func (k Keeper) mustValidateFields(validations map[string]func(Keeper) bool) {
	for errMessage, failedValidation := range validations {
		if failedValidation(k) {
			panic(errMessage)
		}
	}
}

[shaspitz]

This is a solid idea! Although in our repo there will only ever be 2 keepers defined, so maybe not worth adding the additional code and complexity? That sort of functionality living within the sdk repo would make the most sense to me.

Lmk if you strongly disagree and I can refactor in a future PR