From bbdcc8c6e965c8a2f607dfb2b61cd13712dd966a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 6 May 2024 16:31:55 +0200 Subject: [PATCH 1/2] build(deps): Bump golangci/golangci-lint-action from 5.1.0 to 5.3.0 (#6258) Bumps [golangci/golangci-lint-action](https://github.com/golangci/golangci-lint-action) from 5.1.0 to 5.3.0. - [Release notes](https://github.com/golangci/golangci-lint-action/releases) - [Commits](https://github.com/golangci/golangci-lint-action/compare/v5.1.0...v5.3.0) --- updated-dependencies: - dependency-name: golangci/golangci-lint-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/callbacks.yml | 2 +- .github/workflows/e2emodule.yml | 2 +- .github/workflows/golangci-feature.yml | 2 +- .github/workflows/golangci.yml | 2 +- .github/workflows/wasm-client.yml | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/callbacks.yml b/.github/workflows/callbacks.yml index 4dafe573838..ba5deb99eac 100644 --- a/.github/workflows/callbacks.yml +++ b/.github/workflows/callbacks.yml @@ -16,7 +16,7 @@ jobs: with: go-version: '1.21' - uses: actions/checkout@v4 - - uses: golangci/golangci-lint-action@v5.1.0 + - uses: golangci/golangci-lint-action@v5.3.0 with: version: v1.57.2 args: --timeout 5m diff --git a/.github/workflows/e2emodule.yml b/.github/workflows/e2emodule.yml index 8ac1f8cd389..5b03d2b7d4a 100644 --- a/.github/workflows/e2emodule.yml +++ b/.github/workflows/e2emodule.yml @@ -14,7 +14,7 @@ jobs: with: go-version: '1.21' - uses: actions/checkout@v4 - - uses: golangci/golangci-lint-action@v5.1.0 + - uses: golangci/golangci-lint-action@v5.3.0 with: version: v1.57.2 args: --timeout 5m diff --git a/.github/workflows/golangci-feature.yml b/.github/workflows/golangci-feature.yml index 0550ccc6bf0..f769b7384f3 100644 --- a/.github/workflows/golangci-feature.yml +++ b/.github/workflows/golangci-feature.yml @@ -25,7 +25,7 @@ jobs: go-version: '1.21' - uses: actions/checkout@v4 - name: golangci-lint - uses: golangci/golangci-lint-action@v5.1.0 + uses: golangci/golangci-lint-action@v5.3.0 with: version: v1.57.2 args: --timeout 10m diff --git a/.github/workflows/golangci.yml b/.github/workflows/golangci.yml index dcb3fb7057c..f24bb6f2abd 100644 --- a/.github/workflows/golangci.yml +++ b/.github/workflows/golangci.yml @@ -20,7 +20,7 @@ jobs: go-version: '1.21' - uses: actions/checkout@v4 - name: golangci-lint - uses: golangci/golangci-lint-action@v5.1.0 + uses: golangci/golangci-lint-action@v5.3.0 with: version: v1.57.2 args: --timeout 10m diff --git a/.github/workflows/wasm-client.yml b/.github/workflows/wasm-client.yml index 4eaf953450f..800b7757ed0 100644 --- a/.github/workflows/wasm-client.yml +++ b/.github/workflows/wasm-client.yml @@ -16,7 +16,7 @@ jobs: with: go-version: '1.21' - uses: actions/checkout@v4 - - uses: golangci/golangci-lint-action@v5.1.0 + - uses: golangci/golangci-lint-action@v5.3.0 with: version: v1.57.2 args: --timeout 10m From 3b3ecc5af1f4ad8d48685645bf996b69c9c494ad Mon Sep 17 00:00:00 2001 From: srdtrk <59252793+srdtrk@users.noreply.github.com> Date: Tue, 7 May 2024 11:55:34 +0800 Subject: [PATCH 2/2] imp(apps): allow one sided fee middleware handshakes to complete (#6253) * imp: added counter version proposal to try step * test: fixed tests * test: tested removing fee middleware * test: added ica onesided test * add changelog * imp: improved RemoveFeeMiddleware * nit: damian --------- Co-authored-by: Carlos Rodriguez --- CHANGELOG.md | 1 + .../host/keeper/handshake.go | 10 ++++- .../host/keeper/handshake_test.go | 3 +- modules/apps/29-fee/fee_test.go | 40 +++++++++++++++++++ modules/apps/29-fee/ibc_middleware.go | 7 ++-- modules/apps/29-fee/ica_test.go | 25 ++++++++++++ modules/apps/29-fee/transfer_test.go | 16 ++++++++ modules/apps/transfer/ibc_module.go | 10 +++-- modules/apps/transfer/ibc_module_test.go | 19 ++++----- testing/endpoint.go | 2 + 10 files changed, 115 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3080e4a0b9..385550813bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements * (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able. +* (apps/27-interchain-accounts, apps/tranfer, apps/29-fee) [\#6253](https://github.com/cosmos/ibc-go/pull/6253) Allow channel handshake to succeed if fee middleware is wired up on one side, but not the other. ### Features diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake.go b/modules/apps/27-interchain-accounts/host/keeper/handshake.go index a9a62c55e75..2ec9dc8afbd 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake.go @@ -30,13 +30,21 @@ func (k Keeper) OnChanOpenTry( counterparty channeltypes.Counterparty, counterpartyVersion string, ) (string, error) { + logger := k.Logger(ctx) if portID != icatypes.HostPortID { return "", errorsmod.Wrapf(icatypes.ErrInvalidHostPort, "expected %s, got %s", icatypes.HostPortID, portID) } metadata, err := icatypes.MetadataFromVersion(counterpartyVersion) if err != nil { - return "", err + // Propose the default metadata if the counterparty version is invalid + connection, err := k.channelKeeper.GetConnection(ctx, connectionHops[0]) + if err != nil { + return "", errorsmod.Wrapf(err, "failed to retrieve connection %s", connectionHops[0]) + } + + logger.Debug("counterparty version is invalid, proposing default metadata") + metadata = icatypes.NewDefaultMetadata(connection.Counterparty.ConnectionId, connectionHops[0]) } // set here the HostConnectionId in case the controller did not set it diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go index 90a5cb74874..46c653c8dc0 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go @@ -198,9 +198,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { { "invalid metadata bytestring", func() { + // the try step will propose a new valid version path.EndpointA.ChannelConfig.Version = "invalid-metadata-bytestring" }, - false, + true, }, { "unsupported encoding format", diff --git a/modules/apps/29-fee/fee_test.go b/modules/apps/29-fee/fee_test.go index ea4a619a1e1..fa36aee8093 100644 --- a/modules/apps/29-fee/fee_test.go +++ b/modules/apps/29-fee/fee_test.go @@ -5,9 +5,16 @@ import ( testifysuite "github.com/stretchr/testify/suite" + icacontroller "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller" + icacontrollertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types" + icahost "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host" + icahosttypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types" "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/types" + "github.com/cosmos/ibc-go/v8/modules/apps/transfer" + transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" + porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types" ibctesting "github.com/cosmos/ibc-go/v8/testing" ibcmock "github.com/cosmos/ibc-go/v8/testing/mock" ) @@ -55,6 +62,39 @@ func (suite *FeeTestSuite) CreateMockPacket() channeltypes.Packet { ) } +// RemoveFeeMiddleware removes: +// - Fee middleware from transfer module +// - Fee middleware from icahost submodule +// - Fee middleware from icacontroller submodule +// - The entire mock-fee module +// +// It does this by overriding the IBC router with a new router. +func RemoveFeeMiddleware(chain *ibctesting.TestChain) { + channelKeeper := chain.GetSimApp().IBCKeeper.ChannelKeeper + + // Unseal the IBC router by force + chain.GetSimApp().IBCKeeper.PortKeeper.Router = nil + + newRouter := porttypes.NewRouter() // Create a new router + // Remove Fee middleware from transfer module + chain.GetSimApp().TransferKeeper.WithICS4Wrapper(channelKeeper) + transferStack := transfer.NewIBCModule(chain.GetSimApp().TransferKeeper) + newRouter.AddRoute(transfertypes.ModuleName, transferStack) + + // Remove Fee middleware from icahost submodule + chain.GetSimApp().ICAHostKeeper.WithICS4Wrapper(channelKeeper) + icaHostStack := icahost.NewIBCModule(chain.GetSimApp().ICAHostKeeper) + newRouter.AddRoute(icahosttypes.SubModuleName, icaHostStack) + + // Remove Fee middleware from icacontroller submodule + chain.GetSimApp().ICAControllerKeeper.WithICS4Wrapper(channelKeeper) + icaControllerStack := icacontroller.NewIBCMiddleware(nil, chain.GetSimApp().ICAControllerKeeper) + newRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack) + + // Override and seal the router + chain.GetSimApp().IBCKeeper.SetRouter(newRouter) +} + // helper function func lockFeeModule(chain *ibctesting.TestChain) { ctx := chain.GetContext() diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index 4ac1e7dd989..8f5b0dca759 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -139,12 +139,13 @@ func (im IBCMiddleware) OnChanOpenAck( counterpartyChannelID string, counterpartyVersion string, ) error { - // If handshake was initialized with fee enabled it must complete with fee enabled. - // If handshake was initialized with fee disabled it must complete with fee disabled. if im.keeper.IsFeeEnabled(ctx, portID, channelID) { versionMetadata, err := types.MetadataFromVersion(counterpartyVersion) if err != nil { - return errorsmod.Wrapf(err, "failed to unmarshal ICS29 counterparty version metadata: %s", counterpartyVersion) + // we pass the entire version string onto the underlying application. + // and disable fees for this channel + im.keeper.DeleteFeeEnabled(ctx, portID, channelID) + return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion) } if versionMetadata.FeeVersion != types.Version { diff --git a/modules/apps/29-fee/ica_test.go b/modules/apps/29-fee/ica_test.go index b31ed816a21..ed121ae6e44 100644 --- a/modules/apps/29-fee/ica_test.go +++ b/modules/apps/29-fee/ica_test.go @@ -181,6 +181,31 @@ func (suite *FeeTestSuite) TestFeeInterchainAccounts() { suite.Require().Equal(preEscrowBalance.SubAmount(defaultRecvFee.AmountOf(sdk.DefaultBondDenom)), postDistBalance) } +func (suite *FeeTestSuite) TestOnesidedFeeMiddlewareICAHandshake() { + RemoveFeeMiddleware(suite.chainB) // remove fee middleware from chainB + + path := NewIncentivizedICAPath(suite.chainA, suite.chainB) + + path.SetupConnections() + + err := SetupPath(path, defaultOwnerAddress) + suite.Require().NoError(err) + + // assert the newly established channel is not fee enabled on chainB + interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID) + suite.Require().True(found) + + expVersionMetadata, err := icatypes.MetadataFromVersion(defaultICAVersion) + suite.Require().NoError(err) + + expVersionMetadata.Address = interchainAccountAddr + + expVersion := string(icatypes.ModuleCdc.MustMarshalJSON(&expVersionMetadata)) + + suite.Require().Equal(path.EndpointA.ChannelConfig.Version, expVersion) + suite.Require().Equal(path.EndpointB.ChannelConfig.Version, expVersion) +} + func buildInterchainAccountsPacket(path *ibctesting.Path, data []byte, seq uint64) channeltypes.Packet { packet := channeltypes.NewPacket( data, diff --git a/modules/apps/29-fee/transfer_test.go b/modules/apps/29-fee/transfer_test.go index 79a0405fc0d..5281927d963 100644 --- a/modules/apps/29-fee/transfer_test.go +++ b/modules/apps/29-fee/transfer_test.go @@ -163,3 +163,19 @@ func (suite *FeeTestSuite) TestTransferFeeUpgrade() { }) } } + +func (suite *FeeTestSuite) TestOnesidedFeeMiddlewareTransferHandshake() { + RemoveFeeMiddleware(suite.chainB) // remove fee middleware from chainB + + path := ibctesting.NewPath(suite.chainA, suite.chainB) + feeTransferVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version})) + path.EndpointA.ChannelConfig.Version = feeTransferVersion // this will be renegotiated by the Try step + path.EndpointB.ChannelConfig.Version = "" // this will be overwritten by the Try step + path.EndpointA.ChannelConfig.PortID = transfertypes.PortID + path.EndpointB.ChannelConfig.PortID = transfertypes.PortID + + path.Setup() + + suite.Require().Equal(path.EndpointA.ChannelConfig.Version, transfertypes.Version) + suite.Require().Equal(path.EndpointB.ChannelConfig.Version, transfertypes.Version) +} diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 4cfd55ffdef..1424260f178 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -116,15 +116,17 @@ 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) - } - // 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 } + if counterpartyVersion != types.Version { + // Propose the current version + im.keeper.Logger(ctx).Debug("invalid counterparty version, proposing current app version", "counterpartyVersion", counterpartyVersion, "version", types.Version) + return types.Version, nil + } + return types.Version, nil } diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index 878c773d2d4..4017134a999 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -132,31 +132,32 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() { "success", func() {}, nil, }, { - "max channels reached", func() { + "success: invalid counterparty version proposes new version", func() { + // transfer module will propose the default version + counterpartyVersion = "version" + }, nil, + }, + { + "failure: max channels reached", func() { path.EndpointA.ChannelID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1) }, types.ErrMaxTransferChannels, }, { - "capability already claimed", func() { + "failure: 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) }, capabilitytypes.ErrOwnerClaimed, }, { - "invalid order - ORDERED", func() { + "failure: invalid order - ORDERED", func() { channel.Ordering = channeltypes.ORDERED }, channeltypes.ErrInvalidChannelOrdering, }, { - "invalid port ID", func() { + "failure: invalid port ID", func() { path.EndpointA.ChannelConfig.PortID = ibctesting.MockPort }, porttypes.ErrInvalidPort, }, - { - "invalid counterparty version", func() { - counterpartyVersion = "version" - }, types.ErrInvalidVersion, - }, } for _, tc := range testCases { diff --git a/testing/endpoint.go b/testing/endpoint.go index e1adacafe0d..cf523ee8d38 100644 --- a/testing/endpoint.go +++ b/testing/endpoint.go @@ -342,6 +342,7 @@ func (endpoint *Endpoint) ChanOpenInit() error { // update version to selected app version // NOTE: this update must be performed after SendMsgs() endpoint.ChannelConfig.Version = endpoint.GetChannel().Version + endpoint.Counterparty.ChannelConfig.Version = endpoint.GetChannel().Version return nil } @@ -374,6 +375,7 @@ func (endpoint *Endpoint) ChanOpenTry() error { // update version to selected app version // NOTE: this update must be performed after the endpoint channelID is set endpoint.ChannelConfig.Version = endpoint.GetChannel().Version + endpoint.Counterparty.ChannelConfig.Version = endpoint.GetChannel().Version return nil }