Skip to content

chore: implement version checking for channel handshake application callbacks #6175

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.

Sign up for GitHub

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

Merged
crodriguezvega merged 11 commits into from
May 6, 2024
Merged

chore: implement version checking for channel handshake application callbacks #6175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f741403
add SupportedVersions array for different ics20 versions, add version…
charleenfei Apr 16, 2024
daa0b32
add tests
charleenfei Apr 16, 2024
40337d8
update pr review
charleenfei Apr 18, 2024
a6238ce
pr review
charleenfei Apr 22, 2024
2dbf77b
last few pr review nits
charleenfei Apr 22, 2024
ac271fa
linter
charleenfei Apr 22, 2024
1166258
add version counter proposing
charleenfei Apr 25, 2024
8bc703a
fix missing app versino
charleenfei Apr 25, 2024
e5f3d73
update code + tests to return our proposed version if counterparty ve…
charleenfei Apr 25, 2024
90cca94
remove if statement
crodriguezvega Apr 28, 2024
021e42f
address review comments: return ics20-2 if counterparty version is no…
crodriguezvega May 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 16 additions & 13 deletions modules/apps/transfer/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"fmt"
"math"
"slices"
"strings"

errorsmod "cosmossdk.io/errors"
Expand Down Expand Up @@ -85,12 +86,13 @@ func (im IBCModule) OnChanOpenInit(
return "", err
}

// default to latest supported version
if strings.TrimSpace(version) == "" {
version = types.Version
}

if version != types.Version {
return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, version)
if !slices.Contains(types.SupportedVersions, version) {
return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected one of %s, got %s", types.SupportedVersions, version)
}

// Claim channel capability passed back by IBC module
Expand All @@ -116,16 +118,16 @@ func (im IBCModule) OnChanOpenTry(
return "", err
}

if counterpartyVersion != types.Version {
return "", errorsmod.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: expected %s, got %s", types.Version, counterpartyVersion)
if !slices.Contains(types.SupportedVersions, counterpartyVersion) {
Copy link
Copy Markdown
Contributor

@AdityaSripal AdityaSripal Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: we could instead of erroring here, return our preferred version for ACK to optionally accept

Copy link
Copy Markdown
Contributor

@AdityaSripal AdityaSripal Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be changed in ics20-1 only versions of ibc-go to truly take advantage of the change.

It will also help here: #6171

Copy link
Copy Markdown
Contributor Author

@charleenfei charleenfei Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the comment from @chatton is relevant here, that relying on ics4wrapper to correctly return the transfer app version might not be the safest idea -- just adding this as a note, since we'd be relying on this as our proposed version. do you have any thoughts on that @AdityaSripal ?

Copy link
Copy Markdown
Contributor

@colin-axner colin-axner Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you link or summarize the comment from @chatton? Where is the ics4wrapper used when returning a string in the channel handshake?

Copy link
Copy Markdown
Contributor Author

@charleenfei charleenfei Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to counterpropose with our preferred version for ACK to optionally accept, we will have to get our own version from the ICS4 wrappper GetAppVersion right? since we only get the counterparty version from the params.

return types.Version, nil
}

// OpenTry must claim the channelCapability that IBC passes into the callback
if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
return "", err
}

return types.Version, nil
return counterpartyVersion, nil
}

// OnChanOpenAck implements the IBCModule interface
Expand All @@ -136,9 +138,10 @@ func (IBCModule) OnChanOpenAck(
_ string,
counterpartyVersion string,
) error {
if counterpartyVersion != types.Version {
return errorsmod.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: expected %s, got %s", types.Version, counterpartyVersion)
if !slices.Contains(types.SupportedVersions, counterpartyVersion) {
return errorsmod.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: expected one of %s, got %s", types.SupportedVersions, counterpartyVersion)
}

return nil
}

Expand Down Expand Up @@ -315,8 +318,8 @@ func (im IBCModule) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string,
return "", err
}

if proposedVersion != types.Version {
return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, proposedVersion)
if !slices.Contains(types.SupportedVersions, proposedVersion) {
return "", errorsmod.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: expected one of %s, got %s", types.SupportedVersions, proposedVersion)
}

return proposedVersion, nil
Expand All @@ -328,17 +331,17 @@ func (im IBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string,
return "", err
}

if counterpartyVersion != types.Version {
return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, counterpartyVersion)
if !slices.Contains(types.SupportedVersions, counterpartyVersion) {
return types.Version, nil
}

return counterpartyVersion, nil
}

// OnChanUpgradeAck implements the IBCModule interface
func (IBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
if counterpartyVersion != types.Version {
return errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, counterpartyVersion)
if !slices.Contains(types.SupportedVersions, counterpartyVersion) {
return errorsmod.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: expected one of %s, got %s", types.SupportedVersions, counterpartyVersion)
}

return nil
Expand Down
78 changes: 45 additions & 33 deletions modules/apps/transfer/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,50 +25,56 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {
)

testCases := []struct {
name string
malleate func()
expPass bool
name string
malleate func()
expPass bool
expVersion string
}{
{
"success", func() {}, true,
"success", func() {}, true, types.Version,
},
{
// connection hops is not used in the transfer application callback,
// it is already validated in the core OnChanUpgradeInit.
"success: invalid connection hops", func() {
path.EndpointA.ConnectionID = "invalid-connection-id"
}, true,
}, true, types.Version,
},
{
"empty version string", func() {
"success: empty version string", func() {
channel.Version = ""
}, true,
}, true, types.Version,
},
{
"success: ics20-1 legacy", func() {
channel.Version = types.Version1
}, true, types.Version1,
},
{
"max channels reached", func() {
path.EndpointA.ChannelID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1)
}, false,
}, false, "",
},
{
"invalid order - ORDERED", func() {
channel.Ordering = channeltypes.ORDERED
}, false,
}, false, "",
},
{
"invalid port ID", func() {
path.EndpointA.ChannelConfig.PortID = ibctesting.MockPort
}, false,
}, false, "",
},
{
"invalid version", func() {
channel.Version = "version" //nolint:goconst
}, false,
}, false, "",
},
{
"capability already claimed", func() {
err := suite.chainA.GetSimApp().ScopedTransferKeeper.ClaimCapability(suite.chainA.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
suite.Require().NoError(err)
}, false,
}, false, "",
},
}

Expand Down Expand Up @@ -103,7 +109,7 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {

if tc.expPass {
suite.Require().NoError(err)
suite.Require().Equal(types.Version, version)
suite.Require().Equal(tc.expVersion, version)
} else {
suite.Require().Error(err)
suite.Require().Equal(version, "")
Expand All @@ -122,38 +128,44 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
)

testCases := []struct {
name string
malleate func()
expPass bool
name string
malleate func()
expPass bool
expVersion string
}{
{
"success", func() {}, true,
"success", func() {}, true, types.Version,
},
{
"success: counterparty version is legacy ics20-1", func() {
counterpartyVersion = types.Version1
}, true, types.Version1,
},
{
"success: invalid counterparty version, we use our proposed version", func() {
counterpartyVersion = "version"
}, true, types.Version,
},
{
"max channels reached", func() {
path.EndpointA.ChannelID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1)
}, false,
}, false, "",
},
{
"capability already claimed", func() {
err := suite.chainA.GetSimApp().ScopedTransferKeeper.ClaimCapability(suite.chainA.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
suite.Require().NoError(err)
}, false,
}, false, "",
},
{
"invalid order - ORDERED", func() {
channel.Ordering = channeltypes.ORDERED
}, false,
}, false, "",
},
{
"invalid port ID", func() {
path.EndpointA.ChannelConfig.PortID = ibctesting.MockPort
}, false,
},
{
"invalid counterparty version", func() {
counterpartyVersion = "version"
}, false,
}, false, "",
},
}

Expand Down Expand Up @@ -194,7 +206,7 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {

if tc.expPass {
suite.Require().NoError(err)
suite.Require().Equal(types.Version, version)
suite.Require().Equal(tc.expVersion, version)
} else {
suite.Require().Error(err)
suite.Require().Equal("", version)
Expand Down Expand Up @@ -339,18 +351,18 @@ func (suite *TransferTestSuite) TestOnChanUpgradeTry() {
nil,
},
{
"invalid upgrade ordering",
"success: invalid upgrade version from counterparty, we use our proposed version",
func() {
counterpartyUpgrade.Fields.Ordering = channeltypes.ORDERED
counterpartyUpgrade.Fields.Version = ibctesting.InvalidID
},
channeltypes.ErrInvalidChannelOrdering,
nil,
},
{
"invalid upgrade version",
"invalid upgrade ordering",
func() {
counterpartyUpgrade.Fields.Version = ibctesting.InvalidID
counterpartyUpgrade.Fields.Ordering = channeltypes.ORDERED
},
types.ErrInvalidVersion,
channeltypes.ErrInvalidChannelOrdering,
},
}

Expand Down
3 changes: 3 additions & 0 deletions modules/apps/transfer/types/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ var (
PortKey = []byte{0x01}
// DenomTraceKey defines the key to store the denomination trace info in store
DenomTraceKey = []byte{0x02}

// SupportedVersions defines all versions that are supported by the module
SupportedVersions = []string{Version, Version1}
)

// GetEscrowAddress returns the escrow address for the specified channel.
Expand Down