Skip to content
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

Jump to bottom

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

Merged
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 2 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
35 changes: 21 additions & 14 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 @@ -89,16 +90,21 @@ func (im IBCModule) OnChanOpenInit(
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, "invalid version: expected %s or %s, got %s", types.Version1, types.Version, version)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}

// Claim channel capability passed back by IBC module
if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
return "", err
}

return version, nil
if version == types.Version1 {
return types.Version1, nil
}
charleenfei marked this conversation as resolved.
Show resolved Hide resolved

// default to latest supported version
return types.Version, nil
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}

// OnChanOpenTry implements the IBCModule interface.
Expand All @@ -116,16 +122,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
Member

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
Member

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
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
Contributor

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
Contributor Author

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 "", errorsmod.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: expected %s or %s, got %s", types.Version1, types.Version, counterpartyVersion)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}

// 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 +142,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 %s or %s, got %s", types.Version1, types.Version, counterpartyVersion)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}

return nil
}

Expand Down Expand Up @@ -315,8 +322,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 proposed version: expected %s or %s, got %s", types.Version1, types.Version, proposedVersion)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}

return proposedVersion, nil
Expand All @@ -328,17 +335,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 "", errorsmod.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: expected %s or %s, got %s", types.Version1, types.Version, counterpartyVersion)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}

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 %s or %s, got %s", types.Version1, types.Version, counterpartyVersion)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}

return nil
Expand Down
28 changes: 26 additions & 2 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 @@ -22,6 +22,7 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {
path *ibctesting.Path
chanCap *capabilitytypes.Capability
counterparty channeltypes.Counterparty
v1 bool
)

testCases := []struct {
Expand All @@ -44,6 +45,12 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {
channel.Version = ""
}, true,
},
{
"ics20-1 version string", func() {
v1 = true
channel.Version = "ics20-1"
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}, true,
},
{
"max channels reached", func() {
path.EndpointA.ChannelID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1)
Expand Down Expand Up @@ -89,6 +96,7 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {
ConnectionHops: []string{path.EndpointA.ConnectionID},
Version: types.Version,
}
v1 = false
charleenfei marked this conversation as resolved.
Show resolved Hide resolved

var err error
chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(ibctesting.TransferPort, path.EndpointA.ChannelID))
Expand All @@ -103,7 +111,11 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {

if tc.expPass {
suite.Require().NoError(err)
suite.Require().Equal(types.Version, version)
if v1 {
suite.Require().Equal("ics20-1", version)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
} else {
suite.Require().Equal(types.Version, version)
}
} else {
suite.Require().Error(err)
suite.Require().Equal(version, "")
Expand All @@ -119,6 +131,7 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
path *ibctesting.Path
counterparty channeltypes.Counterparty
counterpartyVersion string
v1 bool
)

testCases := []struct {
Expand All @@ -129,6 +142,12 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
{
"success", func() {}, true,
},
{
"counterparty version is ics20-1", func() {
v1 = true
counterpartyVersion = "ics20-1"
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
}, true,
},
{
"max channels reached", func() {
path.EndpointA.ChannelID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1)
Expand Down Expand Up @@ -176,6 +195,7 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
Version: types.Version,
}
counterpartyVersion = types.Version
v1 = false

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)
Expand All @@ -194,7 +214,11 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {

if tc.expPass {
suite.Require().NoError(err)
suite.Require().Equal(types.Version, version)
if v1 {
suite.Require().Equal("ics20-1", version)
} else {
suite.Require().Equal(types.Version, version)
}
} else {
suite.Require().Error(err)
suite.Require().Equal("", version)
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
Loading