-
Notifications
You must be signed in to change notification settings - Fork 625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ica)!: support json tx encoding for interchain accounts #3796
Changes from 136 commits
c9d13c7
fc7b2d1
65bca55
3b19ac3
ba73e12
d5f02cf
836dbb7
57dfd22
90907d7
4af1a83
e16a575
c4b246a
d08d066
882fa52
2eb410a
19c7dc5
66fe43d
345febf
3d5f1c0
e0cde18
9834a20
318f591
b8208fd
f7ccaf6
cc3e293
ed1edf9
ae13295
8c34e7d
88fe02b
a42145b
200b282
9602b47
97de732
f3d9879
a0485f2
7bc2f41
8869b7a
7987739
fb15e55
cb18323
fef3871
4b8c62f
9ed13d8
e5d6c68
483937d
99fb8e8
1ee1fa7
550a314
53aeadd
3aee5f0
2dc24bd
f033903
509b625
3daf6e2
1e8cbe3
2d50034
86e5407
da2801a
5785d02
3f389ea
e5d982e
3bde3dc
db4fd5b
14b3f0d
12b20ec
e2ad1a1
437fd25
f732702
95ccad3
ea3792d
4f3e7d6
32e91e9
f9bab4c
6bd1b47
f0ca07d
e7eabe5
67d02f0
ae8aa60
2d01c88
e331980
1bd9af7
59d9c2e
e657656
92648d8
d52962d
4749442
172879d
4e18bad
3b5b872
ade40de
12b556c
5e530b8
575220c
dbaed63
b8b41ac
6eff38c
2e3914c
4597a8b
1767dcc
b169aa7
5cd75c9
48aa851
936e7a9
5123fba
e893d21
2581cc1
a06a882
1bb2c9f
954f12e
0dca4ae
5a0cd81
10d79db
2955eb9
3ae45fa
0813c59
6b7ac18
30dfe0f
f4112e7
45bb7a7
9a87cee
048bedd
704a99f
635a878
cb5d44d
4408877
7a66beb
85eb946
ab72e95
3b4e389
5b7c709
b4cbe28
7e2b841
985632f
b771d3f
68db856
d0b2483
f6abbb1
6a7c8f9
9010228
9c3ef7e
e81ac1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ import ( | |
"fmt" | ||
"strings" | ||
|
||
errorsmod "cosmossdk.io/errors" | ||
|
||
"github.com/cosmos/cosmos-sdk/codec" | ||
storetypes "github.com/cosmos/cosmos-sdk/store/types" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
@@ -18,6 +20,7 @@ import ( | |
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" | ||
porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types" | ||
host "github.com/cosmos/ibc-go/v7/modules/core/24-host" | ||
ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors" | ||
"github.com/cosmos/ibc-go/v7/modules/core/exported" | ||
) | ||
|
||
|
@@ -106,6 +109,22 @@ func (k Keeper) GetAppVersion(ctx sdk.Context, portID, channelID string) (string | |
return k.ics4Wrapper.GetAppVersion(ctx, portID, channelID) | ||
} | ||
|
||
// GetAppMetadata retrieves the interchain accounts metadata from the store associated with the provided portID and channelID | ||
srdtrk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
func (k Keeper) GetAppMetadata(ctx sdk.Context, portID, channelID string) (icatypes.Metadata, error) { | ||
srdtrk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
appVersion, found := k.GetAppVersion(ctx, portID, channelID) | ||
if !found { | ||
return icatypes.Metadata{}, errorsmod.Wrapf(ibcerrors.ErrNotFound, "app version not found for port %s and channel %s", portID, channelID) | ||
} | ||
|
||
var metadata icatypes.Metadata | ||
if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(appVersion), &metadata); err != nil { | ||
// UnmarshalJSON errors are indeterminate and therefore are not wrapped and included in failed acks | ||
return icatypes.Metadata{}, errorsmod.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata") | ||
} | ||
Comment on lines
+121
to
+123
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess one slight consideration here is that the error code changes depending on whether you wrap the error or not (the error code is included in the acknowledgement). I don't have a preference one way or another, but given that we do have a strong guarantee that we will wipe non-deterministic info from an error before creating a error acknowledgement, I could be okay with wrapping the error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't wrapped them since this is what we agreed in one of the calls. If you don't have a preference, I'm happy to keep it as is. |
||
|
||
return metadata, nil | ||
} | ||
|
||
// GetActiveChannelID retrieves the active channelID from the store keyed by the provided connectionID and portID | ||
func (k Keeper) GetActiveChannelID(ctx sdk.Context, connectionID, portID string) (string, bool) { | ||
store := ctx.KVStore(k.storeKey) | ||
|
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) *ibctesting.Path { | ||
func NewICAPath(chainA, chainB *ibctesting.TestChain, encoding string) *ibctesting.Path { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for the enum type! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 = TestVersion | ||
path.EndpointB.ChannelConfig.Version = TestVersion | ||
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, TestVersion); err != nil { | ||
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) | ||
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) | ||
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) | ||
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) | ||
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() | ||
|
||
|
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