Skip to content

Commit

Permalink
chore: allow ics27 to select and return default JSON encoded metadata (…
Browse files Browse the repository at this point in the history 
…#1550)

* adding distribution logic with new registered payees

* updating ack test cases

* removing commented out test

* adding additional testcase to TestOnAcknowledgementPacket

* cleaning up distribution logic and adapting tests

* updating timeout test cases

* updating timeout logic and adding application callback failure test cases

* Apply suggestions from code review

Co-authored-by: Charly <charly@interchain.berlin>

* WIP handling empty version string for ics27

* updating testcases

* adding test with auth module modifying channel version

* adding inline comments re. ignored channel version from auth module

* updating with deduplication suggestion

Co-authored-by: Cian Hatton <cian@interchain.io>
Co-authored-by: Charly <charly@interchain.berlin>
  • Loading branch information
@damiannolan @chatton @charleenfei
3 people authored Jun 23, 2022
1 parent dbd0f77 commit 8ffa912
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 31 deletions.
3 changes: 2 additions & 1 deletion modules/apps/27-interchain-accounts/controller/ibc_middleware.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ func (im IBCMiddleware) OnChanOpenInit(
return "", types.ErrControllerSubModuleDisabled
}

if err := im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil {
version, err := im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version)
if err != nil {
return "", err
}

Expand Down
26 changes: 13 additions & 13 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,18 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
{
"success", func() {}, true,
},
{
"ICA auth module modification of channel version is ignored", func() {
// NOTE: explicitly modify the channel version via the auth module callback,
// ensuring the expected JSON encoded metadata is not modified upon return
suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
portID, channelID string, chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty, version string,
) (string, error) {
return "invalid-version", nil
}
}, true,
},
{
"controller submodule disabled", func() {
suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(suite.chainA.GetContext(), types.NewParams(false))
Expand Down Expand Up @@ -200,19 +212,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
)

if tc.expPass {
expMetadata := icatypes.NewMetadata(
icatypes.Version,
path.EndpointA.ConnectionID,
path.EndpointB.ConnectionID,
"",
icatypes.EncodingProtobuf,
icatypes.TxTypeSDKMultiMsg,
)

expBytes, err := icatypes.ModuleCdc.MarshalJSON(&expMetadata)
suite.Require().NoError(err)

suite.Require().Equal(version, string(expBytes))
suite.Require().Equal(icatypes.NewDefaultMetadataString(path.EndpointA.ConnectionID, path.EndpointB.ConnectionID), version)
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
Expand Down
29 changes: 19 additions & 10 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,26 +28,35 @@ func (k Keeper) OnChanOpenInit(
chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
) error {
) (string, error) {
if order != channeltypes.ORDERED {
return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
return "", sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
}

if !strings.HasPrefix(portID, icatypes.PortPrefix) {
return sdkerrors.Wrapf(icatypes.ErrInvalidControllerPort, "expected %s{owner-account-address}, got %s", icatypes.PortPrefix, portID)
return "", sdkerrors.Wrapf(icatypes.ErrInvalidControllerPort, "expected %s{owner-account-address}, got %s", icatypes.PortPrefix, portID)
}

if counterparty.PortId != icatypes.PortID {
return sdkerrors.Wrapf(icatypes.ErrInvalidHostPort, "expected %s, got %s", icatypes.PortID, counterparty.PortId)
return "", sdkerrors.Wrapf(icatypes.ErrInvalidHostPort, "expected %s, got %s", icatypes.PortID, counterparty.PortId)
}

var metadata icatypes.Metadata
if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(version), &metadata); err != nil {
return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
if strings.TrimSpace(version) == "" {
connection, err := k.channelKeeper.GetConnection(ctx, connectionHops[0])
if err != nil {
return "", err
}

metadata = icatypes.NewDefaultMetadata(connectionHops[0], connection.GetCounterparty().GetConnectionID())
} else {
if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(version), &metadata); err != nil {
return "", sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
}
}

if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil {
return err
return "", err
}

activeChannelID, found := k.GetActiveChannelID(ctx, connectionHops[0], portID)
Expand All @@ -58,15 +67,15 @@ func (k Keeper) OnChanOpenInit(
}

if channel.State == channeltypes.OPEN {
return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID)
return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID)
}

if !icatypes.IsPreviousMetadataEqual(channel.Version, metadata) {
return sdkerrors.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version")
return "", sdkerrors.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version")
}
}

return nil
return string(icatypes.ModuleCdc.MustMarshalJSON(&metadata)), nil
}

// OnChanOpenAck sets the active channel for the interchain account/owner pair
Expand Down
22 changes: 19 additions & 3 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
true,
},
{
"success - previous active channel closed",
"success: previous active channel closed",
func() {
suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

Expand All @@ -47,6 +47,13 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
},
true,
},
{
"success: empty channel version returns default metadata JSON string",
func() {
channel.Version = ""
},
true,
},
{
"invalid metadata - previous metadata is different",
func() {
Expand Down Expand Up @@ -138,6 +145,14 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
},
false,
},
{
"connection not found with default empty channel version",
func() {
channel.ConnectionHops = []string{"connection-10"}
channel.Version = ""
},
false,
},
{
"invalid controller connection ID",
func() {
Expand Down Expand Up @@ -214,7 +229,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
path.EndpointA.ChannelConfig.PortID = portID

// default values
metadata = icatypes.NewMetadata(icatypes.Version, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID, "", icatypes.EncodingProtobuf, icatypes.TxTypeSDKMultiMsg)
metadata = icatypes.NewMetadata(icatypes.Version, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID, "", icatypes.EncodingProtobuf, icatypes.TxTypeSDKMultiMsg)
versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)

Expand All @@ -232,12 +247,13 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {

tc.malleate() // malleate mutates test data

err = suite.chainA.GetSimApp().ICAControllerKeeper.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
version, err := suite.chainA.GetSimApp().ICAControllerKeeper.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.Version,
)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().Equal(string(versionBytes), version)
} else {
suite.Require().Error(err)
}
Expand Down
16 changes: 12 additions & 4 deletions modules/apps/27-interchain-accounts/types/metadata.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,25 @@ func NewMetadata(version, controllerConnectionID, hostConnectionID, accAddress,
}
}

// NewDefaultMetadataString creates and returns a new JSON encoded version string containing the default ICS27 Metadata values
// NewDefaultMetadata creates and returns a new ICS27 Metadata instance containing the default ICS27 Metadata values
// with the provided controller and host connection identifiers
func NewDefaultMetadataString(controllerConnectionID, hostConnectionID string) string {
func NewDefaultMetadata(connectionConnectionID, hostConnectionID string) Metadata {
metadata := Metadata{
Version: Version,
ControllerConnectionId: controllerConnectionID,
ControllerConnectionId: connectionConnectionID,
HostConnectionId: hostConnectionID,
Encoding: EncodingProtobuf,
TxType: TxTypeSDKMultiMsg,
Version: Version,
}

return metadata
}

// NewDefaultMetadataString creates and returns a new JSON encoded version string containing the default ICS27 Metadata values
// with the provided controller and host connection identifiers
func NewDefaultMetadataString(controllerConnectionID, hostConnectionID string) string {
metadata := NewDefaultMetadata(controllerConnectionID, hostConnectionID)

return string(ModuleCdc.MustMarshalJSON(&metadata))
}

Expand Down

0 comments on commit 8ffa912

Please sign in to comment.