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.

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

move ics4wrapper from keeper to ibc module #3482

Closed
wants to merge 14 commits into from
Closed
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@ var _ porttypes.Middleware = (*IBCMiddleware)(nil)
type IBCMiddleware struct {
app porttypes.IBCModule
keeper keeper.Keeper
next porttypes.ICS4Wrapper
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
}

// NewIBCMiddleware creates a new IBCMiddleware given the associated keeper and underlying application
func NewIBCMiddleware(app porttypes.IBCModule, k keeper.Keeper) IBCMiddleware {
func NewIBCMiddleware(app porttypes.IBCModule, k keeper.Keeper, next porttypes.ICS4Wrapper) IBCMiddleware {
return IBCMiddleware{
app: app,
keeper: k,
next: next,
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
suite.Require().True(ok)

if isNilApp {
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper, suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper)
}

version, err := cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
Expand Down Expand Up @@ -384,7 +384,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {
err = cbs.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelID, path.EndpointB.ChannelConfig.Version)

if isNilApp {
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper, suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper)
}

if tc.expPass {
Expand Down Expand Up @@ -507,7 +507,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanCloseConfirm() {
suite.Require().True(ok)

if isNilApp {
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper, suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper)
}

err = cbs.OnChanCloseConfirm(
Expand Down Expand Up @@ -649,7 +649,7 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
suite.Require().True(ok)

if isNilApp {
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper, suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper)
}

err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, []byte("ack"), nil)
Expand Down Expand Up @@ -742,7 +742,7 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {
suite.Require().True(ok)

if isNilApp {
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper, suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper)
}

err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, nil)
Expand Down
47 changes: 43 additions & 4 deletions modules/apps/29-fee/ibc_middleware.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package fee

import (
"fmt"
"strings"

errorsmod "cosmossdk.io/errors"
Expand All @@ -23,13 +24,15 @@ var _ porttypes.Middleware = (*IBCMiddleware)(nil)
type IBCMiddleware struct {
app porttypes.IBCModule
keeper keeper.Keeper
next porttypes.ICS4Wrapper
}

// NewIBCMiddleware creates a new IBCMiddlware given the keeper and underlying application
func NewIBCMiddleware(app porttypes.IBCModule, k keeper.Keeper) IBCMiddleware {
func NewIBCMiddleware(app porttypes.IBCModule, k keeper.Keeper, next porttypes.ICS4Wrapper) IBCMiddleware {
return IBCMiddleware{
app: app,
keeper: k,
next: next,
}
}

Expand Down Expand Up @@ -331,7 +334,7 @@ func (im IBCMiddleware) SendPacket(
timeoutTimestamp uint64,
data []byte,
) (uint64, error) {
return im.keeper.SendPacket(ctx, chanCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data)
return im.next.SendPacket(ctx, chanCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data)
}

// WriteAcknowledgement implements the ICS4 Wrapper interface
Expand All @@ -341,10 +344,46 @@ func (im IBCMiddleware) WriteAcknowledgement(
packet exported.PacketI,
ack exported.Acknowledgement,
) error {
return im.keeper.WriteAcknowledgement(ctx, chanCap, packet, ack)
if !im.keeper.IsFeeEnabled(ctx, packet.GetDestPort(), packet.GetDestChannel()) {
// ics4Wrapper may be core IBC or higher-level middleware
return im.next.WriteAcknowledgement(ctx, chanCap, packet, ack)
}

packetID := channeltypes.NewPacketID(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())

// retrieve the forward relayer that was stored in `onRecvPacket`
relayer, found := im.keeper.GetRelayerAddressForAsyncAck(ctx, packetID)
if !found {
return errorsmod.Wrapf(types.ErrRelayerNotFoundForAsyncAck, "no relayer address stored for async acknowledgement for packet with portID: %s, channelID: %s, sequence: %d", packetID.PortId, packetID.ChannelId, packetID.Sequence)
}

// it is possible that a relayer has not registered a counterparty address.
// if there is no registered counterparty address then write acknowledgement with empty relayer address and refund recv_fee.
forwardRelayer, _ := im.keeper.GetCounterpartyPayeeAddress(ctx, relayer, packet.GetDestChannel())

newAck := types.NewIncentivizedAcknowledgement(forwardRelayer, ack.Acknowledgement(), ack.Success())

im.keeper.DeleteForwardRelayerAddress(ctx, packetID)

// ics4Wrapper may be core IBC or higher-level middleware
return im.next.WriteAcknowledgement(ctx, chanCap, packet, newAck)
}

// GetAppVersion returns the application version of the underlying application
func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool) {
return im.keeper.GetAppVersion(ctx, portID, channelID)
version, found := im.next.GetAppVersion(ctx, portID, channelID)
if !found {
return "", false
}

if !im.keeper.IsFeeEnabled(ctx, portID, channelID) {
return version, true
}

var metadata types.Metadata
if err := types.ModuleCdc.UnmarshalJSON([]byte(version), &metadata); err != nil {
panic(fmt.Errorf("unable to unmarshal metadata for fee enabled channel: %w", err))
}

return metadata.AppVersion, true
}
112 changes: 112 additions & 0 deletions modules/apps/29-fee/ibc_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
fee "github.com/cosmos/ibc-go/v7/modules/apps/29-fee"
"github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v7/modules/core/24-host"
"github.com/cosmos/ibc-go/v7/modules/core/exported"
Expand Down Expand Up @@ -1080,3 +1081,114 @@ func (suite *FeeTestSuite) TestGetAppVersion() {
})
}
}

func (suite *FeeTestSuite) TestWriteAcknowledgementAsync() {
testCases := []struct {
name string
malleate func()
expPass bool
}{
{
"success",
func() {
suite.chainB.GetSimApp().IBCFeeKeeper.SetRelayerAddressForAsyncAck(suite.chainB.GetContext(), channeltypes.NewPacketID(suite.path.EndpointB.ChannelConfig.PortID, suite.path.EndpointB.ChannelID, 1), suite.chainA.SenderAccount.GetAddress().String())
suite.chainB.GetSimApp().IBCFeeKeeper.SetCounterpartyPayeeAddress(suite.chainB.GetContext(), suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), suite.path.EndpointB.ChannelID)
},
true,
},
{
"relayer address not set for async WriteAcknowledgement",
func() {},
false,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()

// open incentivized channels
// setup pathAToC (chainA -> chainC) first in order to have different channel IDs for chainA & chainB
suite.coordinator.Setup(suite.pathAToC)
// setup path for chainA -> chainB
suite.coordinator.Setup(suite.path)

// build packet
timeoutTimestamp := ^uint64(0)
packet := channeltypes.NewPacket(
[]byte("packetData"),
1,
suite.path.EndpointA.ChannelConfig.PortID,
suite.path.EndpointA.ChannelID,
suite.path.EndpointB.ChannelConfig.PortID,
suite.path.EndpointB.ChannelID,
clienttypes.ZeroHeight(),
timeoutTimestamp,
)

ack := channeltypes.NewResultAcknowledgement([]byte("success"))
chanCap := suite.chainB.GetChannelCapability(suite.path.EndpointB.ChannelConfig.PortID, suite.path.EndpointB.ChannelID)

// malleate test case
tc.malleate()

module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), ibctesting.MockFeePort)
suite.Require().NoError(err)

cbs, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

feeModule := cbs.(fee.IBCMiddleware)

err = feeModule.WriteAcknowledgement(suite.chainB.GetContext(), chanCap, packet, ack)
if tc.expPass {
suite.Require().NoError(err)
_, found := suite.chainB.GetSimApp().IBCFeeKeeper.GetRelayerAddressForAsyncAck(suite.chainB.GetContext(), channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1))
suite.Require().False(found)

expectedAck := types.NewIncentivizedAcknowledgement(suite.chainB.SenderAccount.GetAddress().String(), ack.Acknowledgement(), ack.Success())
commitedAck, _ := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetPacketAcknowledgement(suite.chainB.GetContext(), packet.DestinationPort, packet.DestinationChannel, 1)
suite.Require().Equal(commitedAck, channeltypes.CommitAcknowledgement(expectedAck.Acknowledgement()))
} else {
suite.Require().Error(err)
}
})
}
}

func (suite *FeeTestSuite) TestWriteAcknowledgementAsyncFeeDisabled() {
// open incentivized channel
suite.coordinator.Setup(suite.path)
suite.chainB.GetSimApp().IBCFeeKeeper.DeleteFeeEnabled(suite.chainB.GetContext(), suite.path.EndpointB.ChannelConfig.PortID, "channel-0")

// build packet
timeoutTimestamp := ^uint64(0)
packet := channeltypes.NewPacket(
[]byte("packetData"),
1,
suite.path.EndpointA.ChannelConfig.PortID,
suite.path.EndpointA.ChannelID,
suite.path.EndpointB.ChannelConfig.PortID,
suite.path.EndpointB.ChannelID,
clienttypes.ZeroHeight(),
timeoutTimestamp,
)

ack := channeltypes.NewResultAcknowledgement([]byte("success"))
chanCap := suite.chainB.GetChannelCapability(suite.path.EndpointB.ChannelConfig.PortID, suite.path.EndpointB.ChannelID)

module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), ibctesting.MockFeePort)
suite.Require().NoError(err)

cbs, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

feeModule := cbs.(fee.IBCMiddleware)

err = feeModule.WriteAcknowledgement(suite.chainB.GetContext(), chanCap, packet, ack)
suite.Require().NoError(err)

packetAck, _ := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetPacketAcknowledgement(suite.chainB.GetContext(), packet.DestinationPort, packet.DestinationChannel, 1)
suite.Require().Equal(packetAck, channeltypes.CommitAcknowledgement(ack.Acknowledgement()))
}
5 changes: 1 addition & 4 deletions modules/apps/29-fee/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
"github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types"
ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
)

Expand All @@ -27,7 +26,6 @@ type Keeper struct {
cdc codec.BinaryCodec

authKeeper types.AccountKeeper
ics4Wrapper porttypes.ICS4Wrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to do a similar change for ics 27 controller keeper, right? That's it, we can remove the ics4Wrapper field.

channelKeeper types.ChannelKeeper
portKeeper types.PortKeeper
bankKeeper types.BankKeeper
Expand All @@ -36,13 +34,12 @@ type Keeper struct {
// NewKeeper creates a new 29-fee Keeper instance
func NewKeeper(
cdc codec.BinaryCodec, key storetypes.StoreKey,
ics4Wrapper porttypes.ICS4Wrapper, channelKeeper types.ChannelKeeper,
channelKeeper types.ChannelKeeper,
portKeeper types.PortKeeper, authKeeper types.AccountKeeper, bankKeeper types.BankKeeper,
) Keeper {
return Keeper{
cdc: cdc,
storeKey: key,
ics4Wrapper: ics4Wrapper,
channelKeeper: channelKeeper,
portKeeper: portKeeper,
authKeeper: authKeeper,
Expand Down
75 changes: 0 additions & 75 deletions modules/apps/29-fee/keeper/relay.go

This file was deleted.

Loading