feat(ica)!: support json tx encoding for interchain accounts #3796
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package keeper | ||
|
|
||
| /* | ||
| This file is to allow for unexported functions to be accessible to the testing package. | ||
| */ | ||
|
|
||
| import ( | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
|
||
| icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types" | ||
| ) | ||
|
|
||
| // GetAppMetadata is a wrapper around getAppMetadata to allow the function to be directly called in tests. | ||
| func (k Keeper) GetAppMetadata(ctx sdk.Context, portID, channelID string) (icatypes.Metadata, error) { | ||
| return k.getAppMetadata(ctx, portID, channelID) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package keeper_test | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/suite" | ||
|
|
@@ -9,6 +10,7 @@ import ( | |
| "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types" | ||
| icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types" | ||
| channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" | ||
| ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors" | ||
| ibctesting "github.com/cosmos/ibc-go/v7/testing" | ||
| ) | ||
|
|
||
|
|
@@ -27,6 +29,15 @@ var ( | |
| Encoding: icatypes.EncodingProtobuf, | ||
| TxType: icatypes.TxTypeSDKMultiMsg, | ||
| })) | ||
|
|
||
| // TestVersionWithJSONEncoding defines a reusable interchainaccounts version string that uses JSON encoding for testing purposes | ||
| TestVersionWithJSONEncoding = string(icatypes.ModuleCdc.MustMarshalJSON(&icatypes.Metadata{ | ||
| Version: icatypes.Version, | ||
| ControllerConnectionId: ibctesting.FirstConnectionID, | ||
| HostConnectionId: ibctesting.FirstConnectionID, | ||
| Encoding: icatypes.EncodingProto3JSON, | ||
| TxType: icatypes.TxTypeSDKMultiMsg, | ||
| })) | ||
| ) | ||
|
|
||
| type KeeperTestSuite struct { | ||
|
|
@@ -47,14 +58,25 @@ func (suite *KeeperTestSuite) SetupTest() { | |
| suite.chainC = suite.coordinator.GetChain(ibctesting.GetChainID(3)) | ||
| } | ||
|
|
||
| func NewICAPath(chainA, chainB *ibctesting.TestChain, encoding string) *ibctesting.Path { | ||
|
There was a problem hiding this comment. [nit] is it worth using an enum or type alias for an encoding type instead of a string? There was a problem hiding this comment. I didn't because the original proto didn't include an enum. I'll implement it if others agree There was a problem hiding this comment. I think it would be nice to make this change, previously I don't think the encoding type as a string was exposed to any public interfaces, but these changes will make that the case. Also happy with a follow up PR making only these changes. There was a problem hiding this comment. +1 for the enum type! There was a problem hiding this comment. I've looked into using enums and will probably make this change soon but here is something I've noticed:
This feels over-complicated for this functionality. Nevertheless, I'll make the change and see what happens. There was a problem hiding this comment. Actually sorry, this works: (but it looks weird due to using non-capitalized enum variants): // Encoding defines the supported codec formats for interchain account transactions
enum Encoding {
option (gogoproto.goproto_enum_prefix) = false;
// Default zero value enumeration
ENCODING_UNKNOWN_UNSPECIFIED = 0 [(gogoproto.enumvalue_customname) = "UNKNOWN"];
// proto3 defines the proto3 protobuf encoding
proto3 = 1 [(gogoproto.enumvalue_customname) = "PROTO3"];
// proto3json defines the proto3 json encoding
proto3json = 2 [(gogoproto.enumvalue_customname) = "PROTO3JSON"];
}There was a problem hiding this comment. Finally, after attempting to implement this, I realized this would require reopening of the merged PR to the main, see #3967. Which leads me to think that it is best done in another PR to main, either after this is merged, or before. So I will not push the changes here. I will discuss this briefly in the engineering call There was a problem hiding this comment. Agree it should be discussed in a separate pr. As a side note, enums are encoded differently between standard json and proto3 json. Currently ICA uses proto3 json for encoding the metadata, if it used standard json this change wouldn't be backwards compatible The enum string value for proto3 must be the same as the existing string otherwise we will break cross compatibility for opening channels I would also not backport this change |
||
| path := ibctesting.NewPath(chainA, chainB) | ||
|
|
||
| var version string | ||
| switch encoding { | ||
| case icatypes.EncodingProtobuf: | ||
| version = TestVersion | ||
| case icatypes.EncodingProto3JSON: | ||
| version = TestVersionWithJSONEncoding | ||
| default: | ||
| panic(fmt.Sprintf("unsupported encoding type: %s", encoding)) | ||
| } | ||
|
|
||
| path.EndpointA.ChannelConfig.PortID = icatypes.HostPortID | ||
| path.EndpointB.ChannelConfig.PortID = icatypes.HostPortID | ||
| path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED | ||
| path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED | ||
| path.EndpointA.ChannelConfig.Version = version | ||
| path.EndpointB.ChannelConfig.Version = version | ||
|
|
||
| return path | ||
| } | ||
|
|
@@ -85,7 +107,7 @@ func RegisterInterchainAccount(endpoint *ibctesting.Endpoint, owner string) erro | |
|
|
||
| channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext()) | ||
|
|
||
| if err := endpoint.Chain.GetSimApp().ICAControllerKeeper.RegisterInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner, endpoint.ChannelConfig.Version); err != nil { | ||
| return err | ||
| } | ||
|
|
||
|
|
@@ -106,7 +128,7 @@ func TestKeeperTestSuite(t *testing.T) { | |
| func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() { | ||
| suite.SetupTest() | ||
|
|
||
| path := NewICAPath(suite.chainA, suite.chainB, icatypes.EncodingProtobuf) | ||
| suite.coordinator.SetupConnections(path) | ||
|
|
||
| err := SetupICAPath(path, TestOwnerAddress) | ||
|
|
@@ -131,7 +153,7 @@ func (suite *KeeperTestSuite) TestGetAllActiveChannels() { | |
|
|
||
| suite.SetupTest() | ||
|
|
||
| path := NewICAPath(suite.chainA, suite.chainB, icatypes.EncodingProtobuf) | ||
| suite.coordinator.SetupConnections(path) | ||
|
|
||
| err := SetupICAPath(path, TestOwnerAddress) | ||
|
|
@@ -165,7 +187,7 @@ func (suite *KeeperTestSuite) TestGetAllInterchainAccounts() { | |
|
|
||
| suite.SetupTest() | ||
|
|
||
| path := NewICAPath(suite.chainA, suite.chainB, icatypes.EncodingProtobuf) | ||
| suite.coordinator.SetupConnections(path) | ||
|
|
||
| err := SetupICAPath(path, TestOwnerAddress) | ||
|
|
@@ -197,7 +219,7 @@ func (suite *KeeperTestSuite) TestGetAllInterchainAccounts() { | |
| func (suite *KeeperTestSuite) TestIsActiveChannel() { | ||
| suite.SetupTest() | ||
|
|
||
| path := NewICAPath(suite.chainA, suite.chainB, icatypes.EncodingProtobuf) | ||
| suite.coordinator.SetupConnections(path) | ||
|
|
||
| err := SetupICAPath(path, TestOwnerAddress) | ||
|
|
@@ -220,6 +242,17 @@ func (suite *KeeperTestSuite) TestSetInterchainAccountAddress() { | |
| suite.Require().Equal(expectedAccAddr, retrievedAddr) | ||
| } | ||
|
|
||
| func (suite *KeeperTestSuite) TestMetadataNotFound() { | ||
| var ( | ||
| invalidPortID = "invalid-port" | ||
| invalidChannelID = "invalid-channel" | ||
| ) | ||
|
|
||
| _, err := suite.chainB.GetSimApp().ICAHostKeeper.GetAppMetadata(suite.chainB.GetContext(), invalidPortID, invalidChannelID) | ||
| suite.Require().ErrorIs(err, ibcerrors.ErrNotFound) | ||
| suite.Require().Contains(err.Error(), fmt.Sprintf("app version not found for port %s and channel %s", invalidPortID, invalidChannelID)) | ||
| } | ||
|
|
||
| func (suite *KeeperTestSuite) TestParams() { | ||
| expParams := types.DefaultParams() | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not strictly necessary since the controller doesn't do JSON marshaling, what prompted this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was your comment that prompted the change: #3796 (comment)
Lmk if you want me to revert the change before I merge today