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 3 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, "expected one of [%s, %s], got %s", types.Version1, types.Version, version)
}

// 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
}

// default to latest supported version
return types.Version, nil
}

// 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
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 "", errorsmod.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: expected %s or %s, got %s", types.Version1, types.Version, counterpartyVersion)
}

// 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 one of [%s, %s], got %s", types.Version1, types.Version, counterpartyVersion)
}

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 one of [%s, %s], got %s", types.Version1, types.Version, proposedVersion)
Copy link
Copy Markdown
Contributor

@damiannolan damiannolan Apr 22, 2024

Choose a reason for hiding this comment

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

ditto here for error formatting, but should we be supporting upgrading a transfer channel where the transfer version stays as ics20-1? Is there a use-case where you'd want to enable ics29 fees for example but not upgrade to ics20-2?

To me if I was going to upgrade the channel I'd do both I guess.

Copy link
Copy Markdown
Contributor Author

@charleenfei charleenfei Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

not sure what the general use case will end up looking like between those options, but i dont think this method needs to be opinionated on whether or not ics29 needs to be used with ics20-2/channel upgrades to add mw need to also update ics20 version

Copy link
Copy Markdown
Contributor

@damiannolan damiannolan Apr 22, 2024

Choose a reason for hiding this comment

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

Yeah, enabling ics29 is just an example.

I suppose supporting upgrading the channel to switch the connection and stay on ics20-1 is potentially a valid use-case that should be supported then

}

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 one of [%s, %s], got %s", types.Version1, types.Version, counterpartyVersion)
}

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, %s], got %s", types.Version1, types.Version, counterpartyVersion)
}

return nil
Expand Down
26 changes: 24 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 = types.Version1
}, true,
},
{
"max channels reached", func() {
path.EndpointA.ChannelID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1)
Expand Down Expand Up @@ -103,7 +110,11 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() {

if tc.expPass {
suite.Require().NoError(err)
suite.Require().Equal(types.Version, version)
if v1 {
suite.Require().Equal(types.Version1, version)
} else {
suite.Require().Equal(types.Version, version)
}
} else {
suite.Require().Error(err)
suite.Require().Equal(version, "")
Expand All @@ -119,6 +130,7 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
path *ibctesting.Path
counterparty channeltypes.Counterparty
counterpartyVersion string
v1 bool
)

testCases := []struct {
Expand All @@ -129,6 +141,12 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
{
"success", func() {}, true,
},
{
"counterparty version is ics20-1", func() {
v1 = true
counterpartyVersion = "ics20-1"
}, true,
},
{
"max channels reached", func() {
path.EndpointA.ChannelID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1)
Expand Down Expand Up @@ -194,7 +212,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