From 58e1d5c487d1bf5b90a63b5f0e8d7ab4a0c43a8b Mon Sep 17 00:00:00 2001 From: Joe Bowman Date: Sun, 21 Jan 2024 16:51:01 +0000 Subject: [PATCH] Fix MsgTransfer acknowledgement for non-native tokens (#1056) * move transfer handler to ics middleware to properly handle multihop tokens * gofumpt * lint * rename file, because we dont implement ics4 * remove debug log; add attribution in comment --- app/keepers/keepers.go | 1 + utils/coins.go | 31 ++--- .../keeper/ibc_packet_handlers.go | 54 ++------ .../keeper/ibc_packet_handlers_test.go | 123 ++++++------------ x/interchainstaking/transfer_middleware.go | 123 ++++++++++++++++++ 5 files changed, 189 insertions(+), 143 deletions(-) create mode 100644 x/interchainstaking/transfer_middleware.go diff --git a/app/keepers/keepers.go b/app/keepers/keepers.go index ba6ceb6d4..a03cee706 100644 --- a/app/keepers/keepers.go +++ b/app/keepers/keepers.go @@ -493,6 +493,7 @@ func (appKeepers *AppKeepers) InitKeepers( packetforwardkeeper.DefaultForwardTransferPacketTimeoutTimestamp, packetforwardkeeper.DefaultRefundTransferPacketTimeoutTimestamp, ) + ibcStack = interchainstaking.NewTransferMiddleware(ibcStack, appKeepers.InterchainstakingKeeper) // Create static IBC router, add transfer route, then set and seal it ibcRouter := porttypes.NewRouter() diff --git a/utils/coins.go b/utils/coins.go index 7583876e6..c4c88045b 100644 --- a/utils/coins.go +++ b/utils/coins.go @@ -2,11 +2,12 @@ package utils import ( "fmt" + "strings" sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" - ibctransfertypes "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types" + transfertypes "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types" ) func DenomFromRequestKey(query []byte, accAddr sdk.AccAddress) (string, error) { @@ -27,18 +28,18 @@ func DenomFromRequestKey(query []byte, accAddr sdk.AccAddress) (string, error) { return denom, nil } -func DeriveIbcDenom(port, channel, denom string) string { - return DeriveIbcDenomTrace(port, channel, denom).IBCDenom() -} - -func DeriveIbcDenomTrace(port, channel, denom string) ibctransfertypes.DenomTrace { - // generate denomination prefix - sourcePrefix := ibctransfertypes.GetDenomPrefix(port, channel) - // NOTE: sourcePrefix contains the trailing "/" - prefixedDenom := sourcePrefix + denom - - // construct the denomination trace from the full raw denomination - denomTrace := ibctransfertypes.ParseDenomTrace(prefixedDenom) - - return denomTrace +// DeriveIbcDenom mirrors getDenomForThisChain from the packet-forward-middleware/v5, used under MIT License. +// See: https://github.com/strangelove-ventures/packet-forward-middleware/blob/86f045c12cc48ffc1f016ff122b89a9f6ac8ed63/router/ibc_middleware.go#L104 +func DeriveIbcDenom(port, channel, counterpartyPort, counterpartyChannel, denom string) string { + counterpartyPrefix := transfertypes.GetDenomPrefix(counterpartyPort, counterpartyChannel) + if strings.HasPrefix(denom, counterpartyPrefix) { + unwoundDenom := denom[len(counterpartyPrefix):] + denomTrace := transfertypes.ParseDenomTrace(unwoundDenom) + if denomTrace.Path == "" { + return unwoundDenom + } + return denomTrace.IBCDenom() + } + prefixedDenom := transfertypes.GetDenomPrefix(port, channel) + denom + return transfertypes.ParseDenomTrace(prefixedDenom).IBCDenom() } diff --git a/x/interchainstaking/keeper/ibc_packet_handlers.go b/x/interchainstaking/keeper/ibc_packet_handlers.go index fc3a3511c..c28e569e9 100644 --- a/x/interchainstaking/keeper/ibc_packet_handlers.go +++ b/x/interchainstaking/keeper/ibc_packet_handlers.go @@ -274,21 +274,9 @@ func (k *Keeper) HandleAcknowledgement(ctx sdk.Context, packet channeltypes.Pack return err } case "/ibc.applications.transfer.v1.MsgTransfer": - // this should be okay to fail; we'll pick it up next time around. - if !success { - return nil - } - response := ibctransfertypes.MsgTransferResponse{} - err = proto.Unmarshal(msgResponse, &response) - if err != nil { - k.Logger(ctx).Error("unable to unpack MsgTransfer response", "error", err) - return err - } + k.Logger(ctx).Debug("Received MsgTransfer acknowledgement; no action") + return nil - k.Logger(ctx).Info("MsgTranfer acknowledgement received") - if err := k.HandleMsgTransfer(ctx, msg.Msg); err != nil { - return err - } default: k.Logger(ctx).Error("unhandled acknowledgement packet", "type", reflect.TypeOf(msg.Msg).Name()) } @@ -303,46 +291,28 @@ func (*Keeper) HandleTimeout(_ sdk.Context, _ channeltypes.Packet) error { // ---------------------------------------------------------------- -func (k *Keeper) HandleMsgTransfer(ctx sdk.Context, msg sdk.Msg) error { +func (k *Keeper) HandleMsgTransfer(ctx sdk.Context, msg ibctransfertypes.FungibleTokenPacketData, ibcDenom string) error { k.Logger(ctx).Info("Received MsgTransfer acknowledgement") // first, type assertion. we should have ibctransfertypes.MsgTransfer - sMsg, ok := msg.(*ibctransfertypes.MsgTransfer) - if !ok { - k.Logger(ctx).Error("unable to cast source message to MsgTransfer") - return errors.New("unable to cast source message to MsgTransfer") - } // check if destination is interchainstaking module account (spoiler: it was) - if sMsg.Receiver != k.AccountKeeper.GetModuleAddress(types.ModuleName).String() { + if msg.Receiver != k.AccountKeeper.GetModuleAddress(types.ModuleName).String() { k.Logger(ctx).Error("msgTransfer to unknown account!") return errors.New("unexpected recipient") } - receivedCoin := sMsg.Token - - zone, found := k.GetZoneForWithdrawalAccount(ctx, sMsg.Sender) - if !found { - return fmt.Errorf("zone not found for withdrawal account %s", sMsg.Sender) + receivedAmount, ok := sdkmath.NewIntFromString(msg.Amount) + if !ok { + return fmt.Errorf("unable to marshal amount into math.Int: %s", msg.Amount) } + receivedCoin := sdk.NewCoin(ibcDenom, receivedAmount) - var channel *channeltypes.IdentifiedChannel - k.IBCKeeper.ChannelKeeper.IterateChannels(ctx, func(ic channeltypes.IdentifiedChannel) bool { - if ic.Counterparty.ChannelId == sMsg.SourceChannel && ic.Counterparty.PortId == sMsg.SourcePort && len(ic.ConnectionHops) == 1 && ic.ConnectionHops[0] == zone.ConnectionId && ic.State == channeltypes.OPEN { - channel = &ic - return true - } - return false - }) - - if channel == nil { - k.Logger(ctx).Error("channel not found for the packet", "port", sMsg.SourcePort, "channel", sMsg.SourceChannel) - return errors.New("channel not found for the packet") + zone, found := k.GetZoneForWithdrawalAccount(ctx, msg.Sender) + if !found { + return fmt.Errorf("zone not found for withdrawal account %s", msg.Sender) } - denomTrace := utils.DeriveIbcDenomTrace(channel.PortId, channel.ChannelId, receivedCoin.Denom) - receivedCoin.Denom = denomTrace.IBCDenom() - - if found && denomTrace.BaseDenom != zone.BaseDenom { + if found && msg.Denom != zone.BaseDenom { // k.Logger(ctx).Error("got withdrawal account and NOT staking denom", "rx", receivedCoin.Denom, "trace_base_denom", denomTrace.BaseDenom, "zone_base_denom", zone.BaseDenom) feeAmount := sdk.NewDecFromInt(receivedCoin.Amount).Mul(k.GetCommissionRate(ctx)).TruncateInt() rewardCoin := receivedCoin.SubAmount(feeAmount) diff --git a/x/interchainstaking/keeper/ibc_packet_handlers_test.go b/x/interchainstaking/keeper/ibc_packet_handlers_test.go index d6a9cb208..1023c9685 100644 --- a/x/interchainstaking/keeper/ibc_packet_handlers_test.go +++ b/x/interchainstaking/keeper/ibc_packet_handlers_test.go @@ -55,6 +55,12 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() { fcAmount: math.NewInt(100), withdrawalAmount: math.ZeroInt(), }, + { + name: "ibc denom denom - all goes to fc", + amount: sdk.NewCoin("transfer/channel-569/untrn", math.NewInt(100)), + fcAmount: math.NewInt(2), + withdrawalAmount: math.NewInt(98), + }, { name: "non staking denom - default (2.5%) to fc, remainder to withdrawal", amount: sdk.NewCoin("ujuno", math.NewInt(100)), @@ -81,7 +87,7 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() { channel, cfound := quicksilver.InterchainstakingKeeper.IBCKeeper.ChannelKeeper.GetChannel(ctx, "transfer", "channel-0") suite.True(cfound) - ibcDenom := utils.DeriveIbcDenom(channel.Counterparty.PortId, channel.Counterparty.ChannelId, tc.amount.Denom) + ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", channel.Counterparty.PortId, channel.Counterparty.ChannelId, tc.amount.Denom) err := quicksilver.BankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(ibcDenom, tc.amount.Amount))) suite.NoError(err) @@ -101,14 +107,14 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() { txMacc := quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName) feeMacc := quicksilver.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName) - transferMsg := ibctransfertypes.MsgTransfer{ - SourcePort: "transfer", - SourceChannel: "channel-0", - Token: tc.amount, - Sender: sender, - Receiver: quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName).String(), + transferPacket := ibctransfertypes.FungibleTokenPacketData{ + Amount: tc.amount.Amount.String(), + Denom: tc.amount.Denom, + Sender: sender, + Receiver: quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName).String(), } - suite.NoError(quicksilver.InterchainstakingKeeper.HandleMsgTransfer(ctx, &transferMsg)) + + suite.NoError(quicksilver.InterchainstakingKeeper.HandleMsgTransfer(ctx, transferPacket, utils.DeriveIbcDenom("transfer", "channel-0", channel.Counterparty.PortId, channel.Counterparty.ChannelId, tc.amount.Denom))) txMaccBalance := quicksilver.BankKeeper.GetAllBalances(ctx, txMacc) feeMaccBalance := quicksilver.BankKeeper.GetAllBalances(ctx, feeMacc) @@ -128,15 +134,6 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() { } } -func TestHandleMsgTransferBadType(t *testing.T) { - quicksilver, ctx := app.GetAppWithContext(t, true) - err := quicksilver.BankKeeper.MintCoins(ctx, ibctransfertypes.ModuleName, sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(100)))) - require.NoError(t, err) - - transferMsg := banktypes.MsgSend{} - require.Error(t, quicksilver.InterchainstakingKeeper.HandleMsgTransfer(ctx, &transferMsg)) -} - func TestHandleMsgTransferBadRecipient(t *testing.T) { recipient := addressutils.GenerateAccAddressForTest() quicksilver, ctx := app.GetAppWithContext(t, true) @@ -145,14 +142,13 @@ func TestHandleMsgTransferBadRecipient(t *testing.T) { senderAddr, err := sdk.Bech32ifyAddressBytes("cosmos", sender) require.NoError(t, err) - transferMsg := ibctransfertypes.MsgTransfer{ - SourcePort: "transfer", - SourceChannel: "channel-0", - Token: sdk.NewCoin("denom", sdk.NewInt(100)), - Sender: senderAddr, - Receiver: recipient.String(), + transferMsg := ibctransfertypes.FungibleTokenPacketData{ + Denom: "denom", + Amount: "100", + Sender: senderAddr, + Receiver: recipient.String(), } - require.Error(t, quicksilver.InterchainstakingKeeper.HandleMsgTransfer(ctx, &transferMsg)) + require.Error(t, quicksilver.InterchainstakingKeeper.HandleMsgTransfer(ctx, transferMsg, "raa")) } func (suite *KeeperTestSuite) TestHandleQueuedUnbondings() { @@ -1806,44 +1802,21 @@ func (suite *KeeperTestSuite) Test_v045Callback() { if !found { suite.Fail("unable to retrieve zone for test") } - sender := zone.WithdrawalAddress.Address - quicksilver.InterchainstakingKeeper.IBCKeeper.ChannelKeeper.SetChannel(ctx, "transfer", "channel-0", TestChannel) + val := quicksilver.InterchainstakingKeeper.GetValidatorAddresses(ctx, zone.ChainId)[0] - ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", zone.BaseDenom) - err := quicksilver.BankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(ibcDenom, sdk.NewInt(100)))) - suite.NoError(err) - - transferMsg := ibctransfertypes.MsgTransfer{ - SourcePort: "transfer", - SourceChannel: "channel-0", - Token: sdk.NewCoin(zone.BaseDenom, sdk.NewInt(100)), - Sender: sender, - Receiver: quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName).String(), - } - response := ibctransfertypes.MsgTransferResponse{ - Sequence: 1, + sendMsg := stakingtypes.MsgDelegate{ + Amount: sdk.NewCoin(zone.BaseDenom, sdk.NewInt(100)), + DelegatorAddress: zone.PerformanceAddress.Address, + ValidatorAddress: val, } + response := stakingtypes.MsgDelegateResponse{} respBytes := icatypes.ModuleCdc.MustMarshal(&response) - return []sdk.Msg{&transferMsg}, respBytes + return []sdk.Msg{&sendMsg}, respBytes }, assertStatements: func(ctx sdk.Context, quicksilver *app.Quicksilver) bool { - zone, found := quicksilver.InterchainstakingKeeper.GetZone(ctx, suite.chainB.ChainID) - if !found { - suite.Fail("unable to retrieve zone for test") - } - - txMacc := quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName) - feeMacc := quicksilver.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName) - txMaccBalance2 := quicksilver.BankKeeper.GetAllBalances(ctx, txMacc) - feeMaccBalance2 := quicksilver.BankKeeper.GetAllBalances(ctx, feeMacc) - - ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", zone.BaseDenom) - if txMaccBalance2.AmountOf(ibcDenom).Equal(sdk.ZeroInt()) && feeMaccBalance2.AmountOf(ibcDenom).Equal(sdk.NewInt(100)) { - return true - } - return false + return true }, }, { @@ -1936,45 +1909,22 @@ func (suite *KeeperTestSuite) Test_v046Callback() { if !found { suite.Fail("unable to retrieve zone for test") } - sender := zone.WithdrawalAddress.Address - quicksilver.InterchainstakingKeeper.IBCKeeper.ChannelKeeper.SetChannel(ctx, "transfer", "channel-0", TestChannel) + val := quicksilver.InterchainstakingKeeper.GetValidatorAddresses(ctx, zone.ChainId)[0] - ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", zone.BaseDenom) - err := quicksilver.BankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(ibcDenom, sdk.NewInt(100)))) - suite.NoError(err) - - transferMsg := ibctransfertypes.MsgTransfer{ - SourcePort: "transfer", - SourceChannel: "channel-0", - Token: sdk.NewCoin(zone.BaseDenom, sdk.NewInt(100)), - Sender: sender, - Receiver: quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName).String(), - } - response := ibctransfertypes.MsgTransferResponse{ - Sequence: 1, + sendMsg := stakingtypes.MsgDelegate{ + Amount: sdk.NewCoin(zone.BaseDenom, sdk.NewInt(100)), + DelegatorAddress: zone.PerformanceAddress.Address, + ValidatorAddress: val, } + response := stakingtypes.MsgDelegateResponse{} anyResponse, err := codectypes.NewAnyWithValue(&response) suite.NoError(err) - return []sdk.Msg{&transferMsg}, anyResponse + return []sdk.Msg{&sendMsg}, anyResponse }, assertStatements: func(ctx sdk.Context, quicksilver *app.Quicksilver) bool { - zone, found := quicksilver.InterchainstakingKeeper.GetZone(ctx, suite.chainB.ChainID) - if !found { - suite.Fail("unable to retrieve zone for test") - } - - txMacc := quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName) - feeMacc := quicksilver.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName) - txMaccBalance2 := quicksilver.BankKeeper.GetAllBalances(ctx, txMacc) - feeMaccBalance2 := quicksilver.BankKeeper.GetAllBalances(ctx, feeMacc) - - ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", zone.BaseDenom) - if txMaccBalance2.AmountOf(ibcDenom).Equal(sdk.ZeroInt()) && feeMaccBalance2.AmountOf(ibcDenom).Equal(sdk.NewInt(100)) { - return true - } - return false + return true }, }, { @@ -2047,6 +1997,7 @@ func (suite *KeeperTestSuite) Test_v046Callback() { Data: packetBytes, } + ctx = ctx.WithContext(context.WithValue(ctx.Context(), utils.ContextKey("connectionID"), "connection-0")) suite.NoError(quicksilver.InterchainstakingKeeper.HandleAcknowledgement(ctx, packet, icatypes.ModuleCdc.MustMarshalJSON(&acknowledgement))) suite.True(test.assertStatements(ctx, quicksilver)) diff --git a/x/interchainstaking/transfer_middleware.go b/x/interchainstaking/transfer_middleware.go new file mode 100644 index 000000000..f66d9b2c4 --- /dev/null +++ b/x/interchainstaking/transfer_middleware.go @@ -0,0 +1,123 @@ +package interchainstaking + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + + transfertypes "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types" + channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types" + porttypes "github.com/cosmos/ibc-go/v5/modules/core/05-port/types" + ibcexported "github.com/cosmos/ibc-go/v5/modules/core/exported" + + "github.com/quicksilver-zone/quicksilver/utils" + "github.com/quicksilver-zone/quicksilver/x/interchainstaking/keeper" + "github.com/quicksilver-zone/quicksilver/x/interchainstaking/types" +) + +var _ porttypes.IBCModule = &TransferMiddleware{} + +// IBCModule implements the ICS26 interface for interchain accounts controller chains. +type TransferMiddleware struct { + app porttypes.IBCModule + keeper *keeper.Keeper +} + +// NewIBCModule creates a new IBCModule given the keeper. +func NewTransferMiddleware(app porttypes.IBCModule, k *keeper.Keeper) TransferMiddleware { + return TransferMiddleware{ + app: app, + keeper: k, + } +} + +// OnChanOpenInit implements the IBCModule interface. +func (im TransferMiddleware) OnChanOpenInit( + ctx sdk.Context, + order channeltypes.Order, + connectionHops []string, + portID string, + channelID string, + chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, + version string, +) (string, error) { + return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version) +} + +// OnChanOpenTry implements the IBCModule interface. +func (im TransferMiddleware) OnChanOpenTry( + ctx sdk.Context, + order channeltypes.Order, + connectionHops []string, + portID, channelID string, + chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, + counterpartyVersion string, +) (version string, err error) { + return im.app.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, counterpartyVersion) +} + +// OnChanOpenAck implements the IBCModule interface. +func (im TransferMiddleware) OnChanOpenAck( + ctx sdk.Context, + portID, channelID string, + counterpartyChannelID string, + counterpartyVersion string, +) error { + return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion) +} + +// OnChanOpenConfirm implements the IBCModule interface. +func (im TransferMiddleware) OnChanOpenConfirm(ctx sdk.Context, portID, channelID string) error { + return im.app.OnChanOpenConfirm(ctx, portID, channelID) +} + +// OnChanCloseInit implements the IBCModule interface. +func (im TransferMiddleware) OnChanCloseInit(ctx sdk.Context, portID, channelID string) error { + return im.app.OnChanCloseInit(ctx, portID, channelID) +} + +// OnChanCloseConfirm implements the IBCModule interface. +func (im TransferMiddleware) OnChanCloseConfirm(ctx sdk.Context, portID, channelID string) error { + return im.app.OnChanCloseConfirm(ctx, portID, channelID) +} + +// OnRecvPacket checks implements the IBCModule interface. +func (im TransferMiddleware) OnRecvPacket( + ctx sdk.Context, + packet channeltypes.Packet, + relayer sdk.AccAddress, +) ibcexported.Acknowledgement { + var data transfertypes.FungibleTokenPacketData + if err := transfertypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { + return channeltypes.NewErrorAcknowledgement(err) + } + + _, found := im.keeper.GetZoneForWithdrawalAccount(ctx, data.Sender) + if found { + if data.Receiver == im.keeper.AccountKeeper.GetModuleAddress(types.ModuleName).String() { + im.keeper.Logger(ctx).Info("msgTransfer to ics module account from withdrawal address") + err := im.keeper.HandleMsgTransfer(ctx, data, utils.DeriveIbcDenom(packet.DestinationPort, packet.DestinationChannel, packet.SourcePort, packet.SourceChannel, data.Denom)) + if err != nil { + im.keeper.Logger(ctx).Error("unable to disperse rewards", "error", err.Error()) + } + } + } + + return im.app.OnRecvPacket(ctx, packet, relayer) +} + +// OnAcknowledgementPacket implements the IBCModule interface. +func (im TransferMiddleware) OnAcknowledgementPacket( + ctx sdk.Context, + packet channeltypes.Packet, + acknowledgement []byte, + relayer sdk.AccAddress, +) error { + return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer) +} + +// OnTimeoutPacket implements the IBCModule interface. +func (im TransferMiddleware) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress) error { + return im.app.OnTimeoutPacket(ctx, packet, relayer) +}