Make testing suite to ensure queries never alter state

[hieuvubk]

Closes: #2457

Brief Changelog

Create a func in apptesting to make sure state not change after query.
Create test in modules/client

Testing and Verifying

Added new test
All test passed

[p0mvn]

Suggested change

fmt.Println(res)

[p0mvn]

nit: Let's try to avoid these callbacks in table-driven test cases if possible please. While this test case is not complex, for bigger test cases this pattern becomes confusing. I think having a completely separate test fixture is preferable to avoid encouraging this style.

[p0mvn]

I know it's a lot to refactor so I discussed this with Matt, and he agrees with the suggestion to split it up if possible, please

[hieuvubk]

So should split each query into a separate test function?

[hieuvubk]

To avoid table-driven callbacks, I think there is another way to use queryClient.cc.Invoke and pass query path, request,...

[p0mvn]

I think that sounds reasonable. Do you mind changing one test to see how it compares?

[hieuvubk]

Sure, changed on epochs

[p0mvn]

Thanks, IMO the epoch change looks good.

[hieuvubk]

Thank you, I'll apply it to other modules

[p0mvn]

@hieuvubk thanks for the change. Please apply a state-compatibility label. These start with "V:"

[p0mvn]

LGTM.

I have some points of concern but IMO we can address them separately:

• Difficult to maintain hard-coded proto paths (I've thought about this after we discussed the approach)
• We rely upon that export genesis being correct and exporting all states. I think that the app hash verification approach might be more accurate

[czarcas7ic]

Agreed with @p0mvn assessment in terms of reliance on ExportGenesis. For now I think using exportGenesis is "good enough", but has been known to contain issues in terms of not exporting the right thing. We should both:

1. Audit exportGenesis code for all modules and
2. Look into utilizing appHash instead of exportGenesis comparison

Nice work on this though! 😄

[hieuvubk]

Agreed with @p0mvn assessment in terms of reliance on ExportGenesis. For now I think using exportGenesis is "good enough", but has been known to contain issues in terms of not exporting the right thing. We should both:

1. Audit exportGenesis code for all modules and
2. Look into utilizing appHash instead of exportGenesis comparison

Nice work on this though! 😄

Thank you, will create an issue about it

[p0mvn]

Hi @hieuvubk . Thanks for the great work here. Can you please link the newly created issue once it is ready?

I would like to merge this only after we track future work to avoid getting anything missed

[hieuvubk]

Hi @hieuvubk . Thanks for the great work here. Can you please link the newly created issue once it is ready?

I would like to merge this only after we track future work to avoid getting anything missed

Thank you, I have created 2 separate issues so that we can easily track and discuss them in the future.

[p0mvn]

Awesome, thanks!

[p0mvn]

discussed with @czarcas7ic and this looks good, going to continue improving on the approach in the created issues