refactor: move ica connection identifiers from port to version metadata bytestring #700
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| package keeper | ||
|
|
||
| import ( | ||
| "strings" | ||
|
|
||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
| capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" | ||
|
|
@@ -32,26 +34,25 @@ func (k Keeper) OnChanOpenInit( | |
| 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) | ||
| } | ||
|
|
||
| if counterparty.PortId != icatypes.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 err := k.validateConnectionParams(ctx, connectionHops, metadata.ControllerConnectionId, metadata.HostConnectionId); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if metadata.Version != icatypes.Version { | ||
|
There was a problem hiding this comment. Do we want to allow relayers to pass in empty string for version here? Or is this a future improvement? ref: https://github.com/cosmos/ibc/pull/629/files cc: @colin-axner This can also be implemented in a separate PR There was a problem hiding this comment. Am I missing something here? We should probably be adding a defensive check to There was a problem hiding this comment.
As far as I know, this is handled by core IBC already. You can't open a channel on a port that is not bound already. There was a problem hiding this comment. You're right, it would be handled by core here in the case where a port is not bound 👍 |
||
| return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.Version, metadata.Version) | ||
| } | ||
|
|
||
| activeChannelID, found := k.GetActiveChannelID(ctx, portID) | ||
|
|
@@ -71,21 +72,28 @@ func (k Keeper) OnChanOpenAck( | |
| counterpartyVersion string, | ||
| ) error { | ||
| if portID == icatypes.PortID { | ||
| return sdkerrors.Wrapf(icatypes.ErrInvalidControllerPort, "portID cannot be host chain port ID: %s", icatypes.PortID) | ||
| } | ||
|
|
||
| if !strings.HasPrefix(portID, icatypes.PortPrefix) { | ||
| return sdkerrors.Wrapf(icatypes.ErrInvalidControllerPort, "expected %s{owner-account-address}, got %s", icatypes.PortPrefix, portID) | ||
| } | ||
|
|
||
| var metadata icatypes.Metadata | ||
|
There was a problem hiding this comment. Would there be value in adding a There was a problem hiding this comment. I thought about this myself when I was implementing it, but I came to the conclusion that it wouldn't be so reusable as we need to carry out metadata validation in I've added There was a problem hiding this comment. I think we still need to validate connection sequences here? It is possible the host chain chooses a bad metadata, followup issue There was a problem hiding this comment. Sure, I can address this in a followup PR, with the moving of I assume retrieving the channel from state using channelKeeper, and passing along it's There was a problem hiding this comment. Figured it made more sense to move this to a |
||
| if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(counterpartyVersion), &metadata); err != nil { | ||
| return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata") | ||
| } | ||
|
|
||
| if err := icatypes.ValidateAccountAddress(metadata.Address); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if metadata.Version != icatypes.Version { | ||
| return sdkerrors.Wrapf(icatypes.ErrInvalidVersion, "expected %s, got %s", icatypes.Version, metadata.Version) | ||
| } | ||
|
|
||
| k.SetActiveChannelID(ctx, portID, channelID) | ||
| k.SetInterchainAccountAddress(ctx, portID, metadata.Address) | ||
|
|
||
| return nil | ||
| } | ||
|
|
@@ -102,31 +110,20 @@ func (k Keeper) OnChanCloseConfirm( | |
| return nil | ||
| } | ||
|
|
||
| // validateConnectionParams asserts the provided controller and host connection identifiers match that of the associated connection stored in state | ||
| func (k Keeper) validateConnectionParams(ctx sdk.Context, connectionHops []string, controllerConnectionID, hostConnectionID string) error { | ||
damiannolan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| connectionID := connectionHops[0] | ||
| connection, err := k.channelKeeper.GetConnection(ctx, connectionID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if controllerConnectionID != connectionID { | ||
| return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connectionID, controllerConnectionID) | ||
| } | ||
|
|
||
| if hostConnectionID != connection.GetCounterparty().GetConnectionID() { | ||
| return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "expected %s, got %s", connection.GetCounterparty().GetConnectionID(), hostConnectionID) | ||
| } | ||
|
|
||
| return nil | ||
|
|
||
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.
Removed
counterpartyConnectionIDas it can be retrieved using the channelKeeper below. This reduces the burden on developers implementing their own authentication modulesThere 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.
Nice change. We need to update the spec should we change this @AdityaSripal @crodriguezvega
Likewise one of us should update the ICA repo + hub teams PR (the CLI commands take two connections etc)
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.
We also need to update the spec to explain the json-encoded format of the version string, right?