test: Add test for ICA #5506
test: Add test for ICA #5506
Conversation
Co-authored-by: Rootul P <[email protected]>
| paramsKeeper.Subspace(slashingtypes.ModuleName) | ||
| paramsKeeper.Subspace(govtypes.ModuleName) | ||
| paramsKeeper.Subspace(ibctransfertypes.ModuleName) | ||
| paramsKeeper.Subspace(ibctransfertypes.ModuleName).WithKeyTable(ibctransfertypes.ParamKeyTable()) |
There was a problem hiding this comment.
this is what was causing the icahost module to not correctly bind capabilities, I tested with a locally built image and everything works. Without it, I was getting these errors from hermes
message: "failed to execute message; message index: 1: could not retrieve module from port-id: ports/icahost: capability not found [cosmos/ibc-go/modules/[email protected]/keeper/keeper.go:452] with gas used: '117491'", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc", "x-cosmos-block-height": "66"} }
| paramsKeeper.Subspace(ibctransfertypes.ModuleName).WithKeyTable(ibctransfertypes.ParamKeyTable()) | ||
| paramsKeeper.Subspace(ibcexported.ModuleName) | ||
| paramsKeeper.Subspace(icahosttypes.SubModuleName) | ||
| paramsKeeper.Subspace(icahosttypes.SubModuleName).WithKeyTable(icahosttypes.ParamKeyTable()) |
There was a problem hiding this comment.
[no change needed] I wonder how this interacts with #5442
For context: I noticed that the ICA host params on v6 defaulted to the upstream ICA host params (a.k.a wildcard for AllowMessages) and I had to explicitly set them in the v5 -> v6 upgrade handler. But I wonder if that migration is not needed after this .WithKeyTable change.
Do you know why the icahosttypes and ibctransfertypes param subspaces need the .WithKeyTable lines but the other subspaces don't need them?
There was a problem hiding this comment.
I think your change will still be required, the WithKeyTable change makes it so that the icahost module doesn't fail silently (fails to claim capability)
I tried running
$ celestia-appd query params subspace icahost AllowMessages
param:
key: AllowMessages
subspace: icahost
It looks like here we have the key present, but it has no value, so I would imagine it defaults to the *
However we should double check that both changes work together 👀 maybe we can extend the test to query the allow messages.
I can do this in a follow up maybe as the PR is quite large as is.
| } | ||
|
|
||
| // setupIBCInfrastructure sets up IBC test infrastructure with specified app version | ||
| func (s *IBCTestSuite) setupIBCInfrastructure(appVersion uint64) { |
There was a problem hiding this comment.
this is the logic from the regular ibc test pulled out so it can be re-used in test setup
chatton
requested review from
evan-forbes,
ninabarbakadze and
rach-id
as code owners
2 participants
Overview
WIP: need to clean up a lot of this code, however the test is functional and ICA seems to be working correctly again with the changes to app.go
closes #5452